On Tue, Jul 09, 2024 at 11:02:01PM -0400, Eric Sunshine wrote: > On Tue, Jul 9, 2024 at 9:09 PM Jeff King <peff@xxxxxxxx> 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". > 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. > > - 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. > > 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
