| Message ID | patch-v2-1.1-c34fd06623a-20221207T120220Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d8b21a0fe2bfeaccc5a359d9df8f0fc57e928ccb |
| Headers | show |
| Series | [v2] CI: don't explicitly pick "bash" shell outside of Windows, fix regression | expand |
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
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.
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
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(-)