On Wed, Sep 05 2018, Eric Sunshine wrote:
> On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
>> that the chainlint.sed implementation in 2.19.0 has more sed portability
>> issues.
>>
>> First, whoever implemented the /bin/sed on Solaris apparently read the
>> POSIX requirements for a max length label of 8 to mean that 8 characters
>> should include the colon, so a bunch of things fail because of that, but
>> are fixed with a shorter 7 character label.
>
> I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but
> that's neither here nor there.
>
>> Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
>> teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
>> added a "grep -q" invocation. The /bin/grep on that version of Solaris
>> doesn't have -q.
>
> I knew that '-q' was potentially problematic on some platforms, so I
> checked and saw that existing tests were already using it, thus went
> ahead and used it. Dropping '-q' here and redirecting stderr to
> /dev/null is a perfectly fine alternative.
>
>> We fixed a similar issue way back in 80700fde91
>> ("t/t1304: make a second colon optional in the mask ACL check",
>> 2010-03-15) by redirecting to /dev/null instead.
>>
>> A bunch of other tests in the test suite rely on "grep -q", but nothing
>> as central as chainlint, so it makes everything break. Do we want to
>> away with "grep -q" entirely because of old Solaris /bin/grep?
>
> I count 132 instances in existing tests (though, I may have missed some).
>
>> At this point those familiar with Solaris are screaming ("why are you
>> using anything in /bin!"). Okey fine, but it mostly worked before, so
>> are we OK with breaking it? "Mostly" here is "test suite would fail
>> 20-30 tests for various reasons, but at least no failures in test-lib.sh
>> and the like".
>>
>> However, if as config.mak.uname does we run the tests with
>> PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
>> character labels [...]
>
> So, if you run the tests via 'make test' (or whatever), then you get
> /usr/xpg?/bin in PATH, but if you run an individual test script (and
> haven't set your PATH appropriately), then you encounter the problems
> you report above?
You need to manually set the PATH before running the tests, the
SANE_TOOL_PATH just affects git-sh-setup. We should probably fix that,
i.e. now if you compile git our shellscripts will use the fixed paths,
but not the tests.
>> [...] but starts complaining about this (also in
>> chainlint.sed):
>>
>> sed: Too many commands, last: s/\n//
>
> Erm. Any additional context to help narrow this down?
I tried playing around with it a bit, but honestly don't understand
enough of this advanced sed syntax to make any headway, it's complaining
about e.g. the "check for multi-line double-quoted string" rule, but
then if you remove the s/\n// from there it complains about the next
rule in that format.
If you want to dig into this yourself I think the best way forward is
the last two paragraphs of
https://public-inbox.org/git/20180824152016.20286-1-avarab@xxxxxxxxx/
>> diff --git a/config.mak.uname b/config.mak.uname
>> @@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
>> BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
>> + # t/chainlint.sed is hopelessly broken all known (tested
>> + # Solaris 10 & 11) versions of Solaris, both /bin/sed and
>> + # /usr/xpg4/bin/sed
>> + GIT_TEST_CHAIN_LINT = 0
>> endif
>>
>> It slightly sucks to not have chainlint on
>> Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
>> (there were some big improvements). So I think the patch above is the
>> best way forward, especially since we're on rc2. What do you think?
>
> Keeping in mind that the main goal of 'chainlint' is to prevent _new_
> breakage from entering the test suite, disabling 'chainlint' on
> Solaris is an entirely reasonable way forward. In present day, it
> seems quite unlikely that we'll see someone develop new tests on
> Solaris, so having 'chainlint' disabled there isn't likely to be a big
> deal. Moreover, if someone does write a new test on Solaris which has
> a broken &&-chain in a subshell, that breakage will be caught very
> quickly once the test is run by anyone on Linux or MacOS (or Windows
> or BSD or AIX), so it's not like the broken test will make it into the
> project undetected.
Since writing my initial message I see that the CSW package packaging
git on Solaris just uses GNU userland:
https://sourceforge.net/p/gar/code/HEAD/tree/csw/mgar/pkg/git/trunk/Makefile
I.e. it puts /opt/csw/gnu in its $PATH, so this whole thing is probably
a non-issue. I.e. if the near-as-you-can-get "official" package just
compiles git against GNU tooling, perhaps we should stop caring about
Solaris-native tooling.
That does also mean that something like my patch above isn't OK, since
we shouldn't be disabling GIT_TEST_CHAIN_LINT based on the uname, if it
then turns out that GNU tooling is being used.
So I think for the purposes of v2.19.0-rc2..v2.19.0 it's fine to just
leave this as it is in master right now.
It's somewhat of a shame that we're drifting to not supporing some of
these more obscure POSIX systems "natively" though, but maybe that's the
right move at this point, and maybe it isn't.
When I was porting C code to Solaris ~10 years ago I'd often get SunCC
turning up some interesting issues that were *real* issues, same with
compiling with IBM xlc on AIX.
Now when I'm re-visiting these systems much later I've yet to turn up
some issue like that, just boring compatibility issues with their old
tooling. E.g. one outstanding issue is that some test of ours fail on
AIX because we use "readlink", but that command isn't in POSIX (only the
C function is), so AIX doesn't implement it.
SunCC used to be ahead of GCC & Clang when it came to certain classes of
warnings, but e.g. now everything it complains about is because it
doesn't understand C as well, e.g. we have quite a few compile warnings
due to code like this, which it claims is unreachable (but isn't):
https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
