Skip to content

Commit ff25e35

Browse files
committed
Ensure the Windows check-msrv job uses the intended toolchain
This fixes an edge case in the Windows MSRV job, by using `bash` for script steps on all platforms. The MSRV workflow had alredy implicitly used `bash` on Ubuntu, which implicitly passed `-e` (as also happens when `shell: bash` is used explicitly). But it had implicitly used `pwsh` on Windows. Although GHA runners make an effort to fail `pwsh` script steps when a command has failed, PowerShell (whether `pwsh` or `powershell`) does not have an analogue of POSIX `-e`. Script steps with `pwsh` do not always fail fast, nor even always fail at all, when a command fails that is not the last command in the script. Recent experiments confirm that running an implicit `pwsh` script step with two commands, each of which run external programs, where the first program exits normally but indicating failure (such as with an exit code of 1) but the second program does not fail, will run both commands, then report success for the whole step. This is to say that the kind of bug that 4f2ab5b (GitoxideLabs#1559) fixed in the Windows `test-fast` job in `ci.yml` also existed in the Windows `check-msrv` job in `msrv.yml` (which GitoxideLabs#1559 did not fix). Fortunately, this version of the bug is far less severe, in that the circumstnaces under which a failure would be concealed appear to have been unlikely. However, at least one such concealed-failure mode was plausible. If the `rustup toolchain install ...` command failed, thereby causing `rustup default...` to fail, then the cargo update command could still succeed, masking those two failures. Error messages for the failures would still be shown in the log, but the step would have reported success. Then the `gix check` commands run via `just ci-check-msrv` would check using the wrong toolchain, i.e. a typically newer toolchain than the MSRV, present in the `windows-2022` runner image. This commit fixes that bug by setting a default of `shell: bash` for all script steps not specifying `shell:`. This implicitly passes `-e` to the `bash` interpreter. Although `-e` has its own intricacies, for simple commands such as those shown here, it has the intuitive fail-fast behavior. (The alternative of splitting the steps up so each step directly runs only a single command, as was done in GitoxideLabs#1559, was considered. But the code is less readable if written that way. It may make sense to organize the commands here into more steps, but most likely some steps will continue to have two or more commands.) This commit also rewords a conceptually related comment in `ci.yml` for clarity, and so that differences in wording between comments on `shell: bash` in various workflows reflect differences in circumstances.
1 parent fa25109 commit ff25e35

File tree

2 files changed

+7
-1
lines changed

2 files changed

+7
-1
lines changed

.github/workflows/ci.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,8 @@ jobs:
433433

434434
defaults:
435435
run:
436-
shell: bash # Use bash even on Windows, if we ever reenable windows-latest for testing.
436+
# Use `bash` even on Windows, if we ever reenable `windows-latest` for testing.
437+
shell: bash
437438

438439
steps:
439440
- uses: actions/checkout@v4

.github/workflows/msrv.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ jobs:
3333
# IMPORTANT: adjust etc/msrv-badge.svg as well
3434
rust_version: 1.75.0
3535

36+
defaults:
37+
run:
38+
# Use `bash` even in the Windows job, so any failing command fails its step (due to `-e`).
39+
shell: bash
40+
3641
steps:
3742
- uses: actions/checkout@v4
3843
- uses: extractions/setup-just@v3

0 commit comments

Comments
 (0)