On Fri, Jun 10 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> When ci/print-test-failures.sh was last in this file before 08dccc8fc1f >> (ci: make it easier to find failed tests' logs in the GitHub workflow, >> 2022-05-21) there was no "name" field, that's an unrelated change that >> shouldn't be part of a narrow regression fix. >> >>> + if: failure() && env.FAILED_TEST_ARTIFACTS != '' >> >> We likewise just had "if failure()" then, is the distinction different >> in all these cases? >> >>> + shell: bash >> >> ...and you've made every single one of them run with "bash" instead of >> the default shell, which is another "change while at it" that isn't >> discussed. > > If it is so important to support all the other shells in the GitHub > workflows environment, we can discuss fix-up patches on top or > replacement patches, but does that really matter? If this were main > Makefile or ci/*.sh that are supposed to be usable by places other > than GitHub Actions environment we use for the CI there, of course > it would be worth to try being extra portable, but it may be even > beneficial to "fix" .github/workflows/* stuff, so that we won't have > to be affected by mistaken use of non-portable shell construct > written there, perhaps? It just looks like a mistake. The Windows sections need an explicit "bash" shell, but nothing else does, and the Windows sections had explicit names for somes stuff, but the other ones did not. So I think thas was just a case of copy/pasting the first section(s) rather than bringing back the pre-image. I think just bringing back the old behavior makes sense for a regression fix in a re-roll. Aside from that I think it's very useful to not rely on bash, for future directions of being able to use this tooling more portably, c.f. what I did in my series where you can run "like CI" locally, which I'd like to do on Solaris, AIX & whatever else without it being a portability hassle.
