Re: [PATCH] chainlint.pl: recognize test bodies defined via heredoc

From: Jeff King <peff@peff.net>
Date: Jul 10, 2024, 07:06 UTC
Message-ID: <20240710070647.GA2048777@coredump.intra.peff.net>
In-Reply-To: <CAPig+cRXkOesS_ctvxY2X=rwesTzgrBB0=5fvQLQsG3hZVY9TQ@mail.gmail.com>

On Tue, Jul 09, 2024 at 11:02:01PM -0400, Eric Sunshine wrote:

Show 11 quoted lines

> On Tue, Jul 9, 2024 at 9:09 PM Jeff King <peff@peff.net> wrote:
> > The chainlint.pl parser chokes on CRLF line endings. So Windows CI
> > produces:
> >
> >   runneradmin@fv-az1390-742 MINGW64 /d/a/git/git/t
> >   # perl chainlint.pl chainlint/for-loop.test
> >   'nternal error scanning character '
> 
> As far as I understand, chainlint is disabled in the Windows CI. Did
> you manually re-enable it for testing? Or are you just running it
> manually in the Windows CI?

Neither. As far as I can tell, we still run the "check-chainlint" target as part of "make test", and that's what I saw fail. For instance:

  https://github.com/peff/git/actions/runs/9856301557/job/27213352807

Every one of the "win test" jobs failed, with the same outcome: running check-chainlint triggered the "internal scanning error".

Show 9 quoted lines

> Assuming you manually re-enabled chaintlint in the Windows CI for this
> testing or are running it manually, it may be the case that
> chainlint.pl has never been run in the Windows CI. Specifically,
> chainlint in Windows CI was disabled by a87e427e35 (ci: speed up
> Windows phase, 2019-01-29) which predates the switchover from
> chainlint.sed to chainlint.pl by d00113ec34 (t/Makefile: apply
> chainlint.pl to existing self-tests, 2022-09-01). So, it's quite
> possible that chainlint.pl has never run in Windows CI. But, perhaps
> I'm misunderstanding or missing some piece of information.

I think that commit would prevent it from running as part of the actual test scripts. But we'd still do check-chainlint to run the chainlint self-tests. And because it only sets "--no-chain-lint" in GIT_TEST_OPTS and not GIT_TEST_CHAIN_LINT=0, I think that the bulk run of chainlint.pl by t/Makefile is still run (and then ironically, when that is run the Makefile manually suppresses the per-script runs, so that --no-chain-lint option is truly doing nothing).

And I think is true even with the ci/run-test-slice.sh approach that the Windows tests use. They still drive it through "make", and just override the $(T) variable.

Show 9 quoted lines

> >   - why doesn't "PERLIO=:crlf make check-chainlint" work? It seems that
> >     perl spawned from "make" behaves differently. More mingw weirdness?
> 
> That could indeed be an msys2 issue. It will automatically convert
> colon ":" to semicolon ";" in environment variables since the PATH
> separator on Windows is ";", not ":" as it is on Unix. Moreover, the
> ":" to ";" switcheroo logic is not restricted only to PATH since there
> are other PATH-like variables in common use, so it's applied to all
> environment variables.

Ah, good thinking. I'm not sure if that's it, though. Just PERLIO=crlf should behave the same way (the ":" is technically a separator, and it is only a style suggestion that you prepend one). Likewise a space is supposed to be OK, too, so PERLIO="unix crlf" should work. But neither seems to work for me. So I'm still puzzled.

Show 14 quoted lines

> > I'm tempted to just do this:
> >
> >         while (my $path = $next_script->()) {
> >                 $nscripts++;
> >                 my $fh;
> > -               unless (open($fh, "<", $path)) {
> > +               unless (open($fh, "<:unix:crlf", $path)) {
> >
> > It feels like a hack, but it makes the parser's assumptions explicit,
> > and it should just work everywhere.
> 
> Yep, if this makes it work, then it seems like a good way forward,
> especially since I don't think there's any obvious way to work around
> the ":" to ";" switcheroo performed by msys2.

OK, I'll add that to my series, then. The fact that we weren't really _intending_ to run chainlint there makes me tempted to just punt and disable it. But AFAICT we have been running it for a while, and it could benefit people on Windows (though it is a bit funky that we do a full check-chainlint in each slice). And I suspect disabling it reliably might be a trickier change than what I wrote above anyway. ;)

-Peff