On Thu, Dec 08 2022, Phillip Wood wrote:
> Hi Ævar
>
> On 07/12/2022 12:03, Ævar Arnfjörð Bjarmason wrote:
>> When the "js/ci-github-workflow-markup" topic was originally merged in
>> [1] it included a change to get rid of the "ci/print-test-failures.sh"
>> step[2]. This was then brought back in [3] as part of a fix-up patches
>> on top[4].
>> The problem was that [3] was not a revert of the relevant parts of
>> [2], but rather copy/pasted the "ci/print-test-failures.sh" step that
>> was present for the Windows job to all "ci/print-test-failures.sh"
>> steps. The Windows steps specified "shell: bash", but the non-Windows
>> ones did not.
>> This broke the "ci/print/test-failures.sh" step for the "linux-musl"
>> job, where we don't have a "bash" shell, just a "/bin/sh" (a
>> "dash"). This breakage was reported at the time[5], but hadn't been
>> fixed.
>> It would be sufficient to change this only for "linux-musl", but
>> let's
>> change this for both "regular" and "dockerized" to omit the "shell"
>> line entirely, as we did before [2].
>> Let's also change undo the "name" change that [3] made while
>> copy/pasting the "print test failures" step for the Windows job. These
>> steps are now the same as they were before [2], except that the "if"
>> includes the "env.FAILED_TEST_ARTIFACTS" test.
>
> What's the motivation for this part of the change (which is completely
> unrelated to the choice of shell)?
That the stated aim of [3] was to bring back the code prematurely
removed in [2], this is merely an attempt to bring us back to that
pre-image.
> Having the test failures under
> "Print test failures" makes it easy for new contributors to see where
> to click to see the full output for test failures. Now they will
> appear under "Run ci/print-test-failures.sh" which while not terrible
> is not as clear.
Maybe that's worth doing, I just thought it was as unintentional as the
"bash" change, [2] didn't advocate for changing the "name" fields, or to
add a "shell". Just to bring back [3]. It didn't, and this change does
that.
Maybe we should add "name" fields to everything, but I think that's best
done as a follow-up, and not argued on this regression fix (which is in
"next" already).
But just to comment on the substance of that:
I don't think it's worth it to do so for the *nix recipes.
Unlike e.g. "bundle artifact tar" in the Windows recipe -- which really
benefits from that "name", rather than dumping some opaque command at
the user -- the *nix ones are self-descriptive.
We're not adding any new information by giving
"ci/{install-dependencies,run-build-and-tests,print-test-failures}.sh"
an explicit title. The current implicit title gives you a description
*and* reminds you of what script in ci/* is driving that step.
But this change really was not intended to take a stance on that
question, just to complete the incomplete revert in [3].
I really don't feel strongly about it. But if we are adding titles to
the *nix ones, let's add it to all of them, just as with the Windows
recipe. That wasn't done in [3], which along with the lack of commit
message mention of this change is why I inferred that it wasn't
intentional.
