git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git List <git@vger.kernel.org>, Brandon Casey <drafnel@gmail.com>
Subject: Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Date: Wed, 05 Sep 2018 13:07:16 +0200	[thread overview]
Message-ID: <877ek0rymz.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CAPig+cRrwW662Hzp8V1sH51x8qYxPyqG=VHpOkfd-=RwUSSZbw@mail.gmail.com>


On Wed, Sep 05 2018, Eric Sunshine wrote:

> On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@gmail.com/

>>     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

  reply	other threads:[~2018-09-05 11:07 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-23  9:14 [PATCH] tests: fix and add lint for non-portable head -c N Ævar Arnfjörð Bjarmason
2018-08-23  9:56 ` Test failures on OpenBSD Ævar Arnfjörð Bjarmason
2018-08-23 15:53   ` Junio C Hamano
2018-08-23 15:25 ` [PATCH v2 1/2] tests: fix and add lint for non-portable head -c N Ævar Arnfjörð Bjarmason
2018-08-23 16:11   ` Jeff King
2018-08-23 15:25 ` [PATCH v2 2/2] tests: fix and add lint for non-portable seq Ævar Arnfjörð Bjarmason
2018-08-23 16:08   ` Junio C Hamano
2018-08-23 17:20     ` Ævar Arnfjörð Bjarmason
2018-08-23 20:35   ` [PATCH v3 0/5] OpenBSD & AIX etc. portability fixes Ævar Arnfjörð Bjarmason
2018-08-23 20:36   ` [PATCH v3 1/5] tests: fix and add lint for non-portable head -c N Ævar Arnfjörð Bjarmason
2018-08-23 20:36   ` [PATCH v3 2/5] tests: fix and add lint for non-portable seq Ævar Arnfjörð Bjarmason
2018-08-23 20:36   ` [PATCH v3 3/5] tests: use shorter here-docs in chainlint.sed for AIX sed Ævar Arnfjörð Bjarmason
2018-08-23 20:42     ` Junio C Hamano
2018-08-23 20:56     ` Eric Sunshine
2018-08-24 15:20       ` [PATCH v4 0/6] OpenBSD & AIX etc. portability fixes Ævar Arnfjörð Bjarmason
2018-08-24 15:20       ` [PATCH v4 1/6] tests: fix and add lint for non-portable head -c N Ævar Arnfjörð Bjarmason
2018-08-24 15:20       ` [PATCH v4 2/6] tests: fix and add lint for non-portable seq Ævar Arnfjörð Bjarmason
2018-08-24 15:20       ` [PATCH v4 3/6] tests: fix comment syntax in chainlint.sed for AIX sed Ævar Arnfjörð Bjarmason
2018-08-24 20:52         ` Eric Sunshine
2018-08-24 15:20       ` [PATCH v4 4/6] tests: use shorter here-docs " Ævar Arnfjörð Bjarmason
2018-08-24 21:29         ` Eric Sunshine
2018-08-28 20:14           ` Ævar Arnfjörð Bjarmason
2018-08-28 20:17             ` Eric Sunshine
2018-08-27 19:36         ` Junio C Hamano
2018-09-04 22:36         ` What's cooking in git.git (Sep 2018, #01; Tue, 4) Junio C Hamano
2018-09-05  9:14           ` Eric Sunshine
2018-09-05 17:45             ` Junio C Hamano
2018-09-05 18:44             ` Eric Sunshine
2018-09-05  9:50           ` Eric Sunshine
2018-09-05 20:31             ` Jeff King
2018-09-05 21:56               ` Junio C Hamano
2018-09-05 13:38           ` jc/rebase-in-c-9-fixes, was " Johannes Schindelin
2018-09-05 16:41             ` Junio C Hamano
     [not found]           ` <xmqqbm9b6gxs.fsf@gitster-ct.c.googlers.com>
2018-09-05 16:48             ` Duy Nguyen
2018-09-05 18:18               ` SZEDER Gábor
2018-09-05  8:29         ` Ævar Arnfjörð Bjarmason
2018-09-05  8:59           ` Eric Sunshine
2018-09-05 11:07             ` Ævar Arnfjörð Bjarmason [this message]
2018-11-24 19:33               ` Ævar Arnfjörð Bjarmason
2018-11-25  4:28                 ` Torsten Bögershausen
2018-11-25  8:21                   ` Torsten Bögershausen
2018-11-25 14:14                     ` Ævar Arnfjörð Bjarmason
2018-09-05 16:45             ` Junio C Hamano
2018-08-24 15:20       ` [PATCH v4 5/6] tests: fix version-specific portability issue in Perl JSON Ævar Arnfjörð Bjarmason
2018-08-24 20:41         ` Eric Sunshine
2018-08-24 15:20       ` [PATCH v4 6/6] tests: fix and add lint for non-portable grep --file Ævar Arnfjörð Bjarmason
2018-08-23 20:36   ` [PATCH v3 4/5] tests: fix version-specific portability issue in Perl JSON Ævar Arnfjörð Bjarmason
2018-08-23 20:36   ` [PATCH v3 5/5] tests: fix and add lint for non-portable grep --file Ævar Arnfjörð Bjarmason
2018-08-23 20:44     ` Junio C Hamano
2018-08-24 13:49       ` Derrick Stolee
2018-08-23 15:42 ` [PATCH] tests: fix and add lint for non-portable head -c N Junio C Hamano
2018-08-23 17:24   ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877ek0rymz.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=drafnel@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).