[PATCH] CI: don't explicitly pick "bash" shell outside of Windows, fix regression

[PATCH] CI: don't explicitly pick "bash" shell outside of Windows, fix regression

[Ævar Arnfjörð Bjarmason]

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.

1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)
2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the
   GitHub workflow, 2022-05-21)
3. 5aeb145780f (ci(github): bring back the 'print test failures' step,
   2022-06-08)
4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)
5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/
---
 .github/workflows/main.yml | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 9afacfa0b33..1a86f6a8ce1 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,10 +265,8 @@ jobs:
     - uses: actions/checkout@v2
     - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
-    - name: print test failures
+    - run: ci/print-test-failures.sh
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
-      shell: bash
-      run: ci/print-test-failures.sh
     - name: Upload failed tests' directories
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
       uses: actions/upload-artifact@v2
@@ -297,10 +295,8 @@ jobs:
     - uses: actions/checkout@v1
     - run: ci/install-docker-dependencies.sh
     - run: ci/run-build-and-tests.sh
-    - name: print test failures
+    - run: ci/print-test-failures.sh
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
-      shell: bash
-      run: ci/print-test-failures.sh
     - name: Upload failed tests' directories
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
       uses: actions/upload-artifact@v1
-- 
2.39.0.rc2.1019.gce04d262ea9

[PATCH v2] CI: don't explicitly pick "bash" shell outside of Windows, fix regression

[Ævar Arnfjörð Bjarmason]

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.

1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)
2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the
   GitHub workflow, 2022-05-21)
3. 5aeb145780f (ci(github): bring back the 'print test failures' step,
   2022-06-08)
4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)
5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I forgot the Signed-off-by in v1, sorry.

Range-diff against v1:
1:  08dc682926a ! 1:  c34fd06623a CI: don't explicitly pick "bash" shell outside of Windows, fix regression
    @@ Commit message
         4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)
         5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/
     
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +
      ## .github/workflows/main.yml ##
     @@ .github/workflows/main.yml: jobs:
          - uses: actions/checkout@v2

 .github/workflows/main.yml | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 9afacfa0b33..1a86f6a8ce1 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,10 +265,8 @@ jobs:
     - uses: actions/checkout@v2
     - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
-    - name: print test failures
+    - run: ci/print-test-failures.sh
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
-      shell: bash
-      run: ci/print-test-failures.sh
     - name: Upload failed tests' directories
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
       uses: actions/upload-artifact@v2
@@ -297,10 +295,8 @@ jobs:
     - uses: actions/checkout@v1
     - run: ci/install-docker-dependencies.sh
     - run: ci/run-build-and-tests.sh
-    - name: print test failures
+    - run: ci/print-test-failures.sh
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
-      shell: bash
-      run: ci/print-test-failures.sh
     - name: Upload failed tests' directories
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
       uses: actions/upload-artifact@v1
-- 
2.39.0.rc2.1019.gce04d262ea9

Re: [PATCH v2] CI: don't explicitly pick "bash" shell outside of Windows, fix regression

[Phillip Wood]

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

Best Wishes

Phillip

> 1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)
> 2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the
>     GitHub workflow, 2022-05-21)
> 3. 5aeb145780f (ci(github): bring back the 'print test failures' step,
>     2022-06-08)
> 4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)
> 5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> I forgot the Signed-off-by in v1, sorry.
> 
> Range-diff against v1:
> 1:  08dc682926a ! 1:  c34fd06623a CI: don't explicitly pick "bash" shell outside of Windows, fix regression
>      @@ Commit message
>           4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)
>           5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/
>       
>      +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      +
>        ## .github/workflows/main.yml ##
>       @@ .github/workflows/main.yml: jobs:
>            - uses: actions/checkout@v2
> 
>   .github/workflows/main.yml | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 9afacfa0b33..1a86f6a8ce1 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -265,10 +265,8 @@ jobs:
>       - uses: actions/checkout@v2
>       - run: ci/install-dependencies.sh
>       - run: ci/run-build-and-tests.sh
> -    - name: print test failures
> +    - run: ci/print-test-failures.sh
>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''
> -      shell: bash
> -      run: ci/print-test-failures.sh
>       - name: Upload failed tests' directories
>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''
>         uses: actions/upload-artifact@v2
> @@ -297,10 +295,8 @@ jobs:
>       - uses: actions/checkout@v1
>       - run: ci/install-docker-dependencies.sh
>       - run: ci/run-build-and-tests.sh
> -    - name: print test failures
> +    - run: ci/print-test-failures.sh
>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''
> -      shell: bash
> -      run: ci/print-test-failures.sh
>       - name: Upload failed tests' directories
>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''
>         uses: actions/upload-artifact@v1

Re: [PATCH v2] CI: don't explicitly pick "bash" shell outside of Windows, fix regression

[Ævar Arnfjörð Bjarmason]

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.