mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
Cc:, Junio C Hamano <>,
	Johannes Schindelin <>
Subject: Re: [PATCH v2] CI: don't explicitly pick "bash" shell outside of Windows, fix regression
Date: Thu, 08 Dec 2022 13:20:01 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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/"
>> 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/" step that
>> was present for the Windows job to all "ci/"
>> steps. The Windows steps specified "shell: bash", but the non-Windows
>> ones did not.
>> This broke the "ci/print/" 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

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

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

      reply	other threads:[~2022-12-08 12:34 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-07  3:15 [PATCH] CI: don't explicitly pick "bash" shell outside of Windows, fix regression Ævar Arnfjörð Bjarmason
2022-12-07 12:03 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-12-08 10:09   ` Phillip Wood
2022-12-08 12:20     ` Ævar Arnfjörð Bjarmason [this message]

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:

  List information:

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

  git send-email \ \ \ \ \ \ \

* 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

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