All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH] ci(github): bring back the 'print test failures' step
Date: Fri, 10 Jun 2022 19:32:07 +0200	[thread overview]
Message-ID: <220610.86edzws9q0.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqtu8sfp52.fsf@gitster.g>


On Fri, Jun 10 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.


  reply	other threads:[~2022-06-10 17:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-08 10:43 [PATCH] ci(github): bring back the 'print test failures' step Johannes Schindelin via GitGitGadget
2022-06-08 23:14 ` Junio C Hamano
2022-06-09 13:06 ` Ævar Arnfjörð Bjarmason
2022-06-10 16:40   ` Junio C Hamano
2022-06-10 17:32     ` Ævar Arnfjörð Bjarmason [this message]
2022-07-25 20:00       ` Ævar Arnfjörð Bjarmason

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=220610.86edzws9q0.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.