{"thread":{"id":"58913","subject":"[PATCH] CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression","startedAt":"2022-12-07T03:15:30Z","lastAt":"2022-12-08T12:34:23Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468678","messageId":"patch-1.1-08dc682926a-20221207T031459Z-avarab@gmail.com","threadId":"58913","inReplyTo":null,"subject":"[PATCH] CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T03:15:19Z","receivedAt":"2022-12-07T03:15:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the \"js/ci-github-workflow-markup\" topic was originally merged in\n[1] it included a change to get rid of the \"ci/print-test-failures.sh\"\nstep[2]. This was then brought back in [3] as part of a fix-up patches\non top[4].\n\nThe problem was that [3] was not a revert of the relevant parts of\n[2], but rather copy/pasted the \"ci/print-test-failures.sh\" step that\nwas present for the Windows job to all \"ci/print-test-failures.sh\"\nsteps. The Windows steps specified \"shell: bash\", but the non-Windows\nones did not.\n\nThis broke the \"ci/print/test-failures.sh\" step for the \"linux-musl\"\njob, where we don't have a \"bash\" shell, just a \"/bin/sh\" (a\n\"dash\"). This breakage was reported at the time[5], but hadn't been\nfixed.\n\nIt would be sufficient to change this only for \"linux-musl\", but let's\nchange this for both \"regular\" and \"dockerized\" to omit the \"shell\"\nline entirely, as we did before [2].\n\nLet's also change undo the \"name\" change that [3] made while\ncopy/pasting the \"print test failures\" step for the Windows job. These\nsteps are now the same as they were before [2], except that the \"if\"\nincludes the \"env.FAILED_TEST_ARTIFACTS\" test.\n\n1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)\n2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the\n   GitHub workflow, 2022-05-21)\n3. 5aeb145780f (ci(github): bring back the 'print test failures' step,\n   2022-06-08)\n4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)\n5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/\n---\n .github/workflows/main.yml | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 9afacfa0b33..1a86f6a8ce1 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,10 +265,8 @@ jobs:\n     - uses: actions/checkout@v2\n     - run: ci/install-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - name: print test failures\n+    - run: ci/print-test-failures.sh\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n-      shell: bash\n-      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v2\n@@ -297,10 +295,8 @@ jobs:\n     - uses: actions/checkout@v1\n     - run: ci/install-docker-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - name: print test failures\n+    - run: ci/print-test-failures.sh\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n-      shell: bash\n-      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v1\n-- \n2.39.0.rc2.1019.gce04d262ea9\n\n"},{"id":"468707","messageId":"patch-v2-1.1-c34fd06623a-20221207T120220Z-avarab@gmail.com","threadId":"58913","inReplyTo":"patch-1.1-08dc682926a-20221207T031459Z-avarab@gmail.com","subject":"[PATCH v2] CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T12:03:15Z","receivedAt":"2022-12-07T12:03:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the \"js/ci-github-workflow-markup\" topic was originally merged in\n[1] it included a change to get rid of the \"ci/print-test-failures.sh\"\nstep[2]. This was then brought back in [3] as part of a fix-up patches\non top[4].\n\nThe problem was that [3] was not a revert of the relevant parts of\n[2], but rather copy/pasted the \"ci/print-test-failures.sh\" step that\nwas present for the Windows job to all \"ci/print-test-failures.sh\"\nsteps. The Windows steps specified \"shell: bash\", but the non-Windows\nones did not.\n\nThis broke the \"ci/print/test-failures.sh\" step for the \"linux-musl\"\njob, where we don't have a \"bash\" shell, just a \"/bin/sh\" (a\n\"dash\"). This breakage was reported at the time[5], but hadn't been\nfixed.\n\nIt would be sufficient to change this only for \"linux-musl\", but let's\nchange this for both \"regular\" and \"dockerized\" to omit the \"shell\"\nline entirely, as we did before [2].\n\nLet's also change undo the \"name\" change that [3] made while\ncopy/pasting the \"print test failures\" step for the Windows job. These\nsteps are now the same as they were before [2], except that the \"if\"\nincludes the \"env.FAILED_TEST_ARTIFACTS\" test.\n\n1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)\n2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the\n   GitHub workflow, 2022-05-21)\n3. 5aeb145780f (ci(github): bring back the 'print test failures' step,\n   2022-06-08)\n4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)\n5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nI forgot the Signed-off-by in v1, sorry.\n\nRange-diff against v1:\n1:  08dc682926a ! 1:  c34fd06623a CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression\n    @@ Commit message\n         4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)\n         5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/\n     \n    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    +\n      ## .github/workflows/main.yml ##\n     @@ .github/workflows/main.yml: jobs:\n          - uses: actions/checkout@v2\n\n .github/workflows/main.yml | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 9afacfa0b33..1a86f6a8ce1 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,10 +265,8 @@ jobs:\n     - uses: actions/checkout@v2\n     - run: ci/install-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - name: print test failures\n+    - run: ci/print-test-failures.sh\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n-      shell: bash\n-      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v2\n@@ -297,10 +295,8 @@ jobs:\n     - uses: actions/checkout@v1\n     - run: ci/install-docker-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - name: print test failures\n+    - run: ci/print-test-failures.sh\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n-      shell: bash\n-      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v1\n-- \n2.39.0.rc2.1019.gce04d262ea9\n\n"},{"id":"468775","messageId":"271d275e-df40-76e5-3930-89ab63961fb2@dunelm.org.uk","threadId":"58913","inReplyTo":"patch-v2-1.1-c34fd06623a-20221207T120220Z-avarab@gmail.com","subject":"Re: [PATCH v2] CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-08T10:09:38Z","receivedAt":"2022-12-08T10:09:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 07/12/2022 12:03, Ævar Arnfjörð Bjarmason wrote:\n> When the \"js/ci-github-workflow-markup\" topic was originally merged in\n> [1] it included a change to get rid of the \"ci/print-test-failures.sh\"\n> step[2]. This was then brought back in [3] as part of a fix-up patches\n> on top[4].\n> \n> The problem was that [3] was not a revert of the relevant parts of\n> [2], but rather copy/pasted the \"ci/print-test-failures.sh\" step that\n> was present for the Windows job to all \"ci/print-test-failures.sh\"\n> steps. The Windows steps specified \"shell: bash\", but the non-Windows\n> ones did not.\n> \n> This broke the \"ci/print/test-failures.sh\" step for the \"linux-musl\"\n> job, where we don't have a \"bash\" shell, just a \"/bin/sh\" (a\n> \"dash\"). This breakage was reported at the time[5], but hadn't been\n> fixed.\n> \n> It would be sufficient to change this only for \"linux-musl\", but let's\n> change this for both \"regular\" and \"dockerized\" to omit the \"shell\"\n> line entirely, as we did before [2].\n> \n> Let's also change undo the \"name\" change that [3] made while\n> copy/pasting the \"print test failures\" step for the Windows job. These\n> steps are now the same as they were before [2], except that the \"if\"\n> includes the \"env.FAILED_TEST_ARTIFACTS\" test.\n\nWhat's the motivation for this part of the change (which is completely \nunrelated to the choice of shell)? Having the test failures under \"Print \ntest failures\" makes it easy for new contributors to see where to click \nto see the full output for test failures. Now they will appear under \n\"Run ci/print-test-failures.sh\" which while not terrible is not as clear.\n\nBest Wishes\n\nPhillip\n\n> 1. fc5a070f591 (Merge branch 'js/ci-github-workflow-markup', 2022-06-07)\n> 2. 08dccc8fc1f (ci: make it easier to find failed tests' logs in the\n>     GitHub workflow, 2022-05-21)\n> 3. 5aeb145780f (ci(github): bring back the 'print test failures' step,\n>     2022-06-08)\n> 4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)\n> 5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> I forgot the Signed-off-by in v1, sorry.\n> \n> Range-diff against v1:\n> 1:  08dc682926a ! 1:  c34fd06623a CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression\n>      @@ Commit message\n>           4. d0d96b8280f (Merge branch 'js/ci-github-workflow-markup', 2022-06-17)\n>           5. https://lore.kernel.org/git/220725.86sfmpneqp.gmgdl@evledraar.gmail.com/\n>       \n>      +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>      +\n>        ## .github/workflows/main.yml ##\n>       @@ .github/workflows/main.yml: jobs:\n>            - uses: actions/checkout@v2\n> \n>   .github/workflows/main.yml | 8 ++------\n>   1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index 9afacfa0b33..1a86f6a8ce1 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -265,10 +265,8 @@ jobs:\n>       - uses: actions/checkout@v2\n>       - run: ci/install-dependencies.sh\n>       - run: ci/run-build-and-tests.sh\n> -    - name: print test failures\n> +    - run: ci/print-test-failures.sh\n>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n> -      shell: bash\n> -      run: ci/print-test-failures.sh\n>       - name: Upload failed tests' directories\n>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n>         uses: actions/upload-artifact@v2\n> @@ -297,10 +295,8 @@ jobs:\n>       - uses: actions/checkout@v1\n>       - run: ci/install-docker-dependencies.sh\n>       - run: ci/run-build-and-tests.sh\n> -    - name: print test failures\n> +    - run: ci/print-test-failures.sh\n>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n> -      shell: bash\n> -      run: ci/print-test-failures.sh\n>       - name: Upload failed tests' directories\n>         if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n>         uses: actions/upload-artifact@v1\n"},{"id":"468781","messageId":"221208.86sfhq6pmg.gmgdl@evledraar.gmail.com","threadId":"58913","inReplyTo":"271d275e-df40-76e5-3930-89ab63961fb2@dunelm.org.uk","subject":"Re: [PATCH v2] CI: don't explicitly pick \"bash\" shell outside of Windows, fix regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-08T12:20:01Z","receivedAt":"2022-12-08T12:34:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 08 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 07/12/2022 12:03, Ævar Arnfjörð Bjarmason wrote:\n>> When the \"js/ci-github-workflow-markup\" topic was originally merged in\n>> [1] it included a change to get rid of the \"ci/print-test-failures.sh\"\n>> step[2]. This was then brought back in [3] as part of a fix-up patches\n>> on top[4].\n>> The problem was that [3] was not a revert of the relevant parts of\n>> [2], but rather copy/pasted the \"ci/print-test-failures.sh\" step that\n>> was present for the Windows job to all \"ci/print-test-failures.sh\"\n>> steps. The Windows steps specified \"shell: bash\", but the non-Windows\n>> ones did not.\n>> This broke the \"ci/print/test-failures.sh\" step for the \"linux-musl\"\n>> job, where we don't have a \"bash\" shell, just a \"/bin/sh\" (a\n>> \"dash\"). This breakage was reported at the time[5], but hadn't been\n>> fixed.\n>> It would be sufficient to change this only for \"linux-musl\", but\n>> let's\n>> change this for both \"regular\" and \"dockerized\" to omit the \"shell\"\n>> line entirely, as we did before [2].\n>> Let's also change undo the \"name\" change that [3] made while\n>> copy/pasting the \"print test failures\" step for the Windows job. These\n>> steps are now the same as they were before [2], except that the \"if\"\n>> includes the \"env.FAILED_TEST_ARTIFACTS\" test.\n>\n> What's the motivation for this part of the change (which is completely\n> unrelated to the choice of shell)?\n\nThat the stated aim of [3] was to bring back the code prematurely\nremoved in [2], this is merely an attempt to bring us back to that\npre-image.\n\n> Having the test failures under\n> \"Print test failures\" makes it easy for new contributors to see where\n> to click to see the full output for test failures. Now they will\n> appear under \"Run ci/print-test-failures.sh\" which while not terrible\n> is not as clear.\n\nMaybe that's worth doing, I just thought it was as unintentional as the\n\"bash\" change, [2] didn't advocate for changing the \"name\" fields, or to\nadd a \"shell\". Just to bring back [3]. It didn't, and this change does\nthat.\n\nMaybe we should add \"name\" fields to everything, but I think that's best\ndone as a follow-up, and not argued on this regression fix (which is in\n\"next\" already).\n\nBut just to comment on the substance of that:\n\nI don't think it's worth it to do so for the *nix recipes.\n\nUnlike e.g. \"bundle artifact tar\" in the Windows recipe -- which really\nbenefits from that \"name\", rather than dumping some opaque command at\nthe user -- the *nix ones are self-descriptive.\n\nWe're not adding any new information by giving\n\"ci/{install-dependencies,run-build-and-tests,print-test-failures}.sh\"\nan explicit title. The current implicit title gives you a description\n*and* reminds you of what script in ci/* is driving that step.\n\nBut this change really was not intended to take a stance on that\nquestion, just to complete the incomplete revert in [3].\n\nI really don't feel strongly about it. But if we are adding titles to\nthe *nix ones, let's add it to all of them, just as with the Windows\nrecipe. That wasn't done in [3], which along with the lack of commit\nmessage mention of this change is why I inferred that it wasn't\nintentional.\n"}]}