Improve MSRV check and fix aria-label in MSRV badge
#2003
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Probably it should not be in quite this form in the `justfile`, though, because only the `#` line immediately preceding the first line that is formally part of the recipe is shown in `just --list`. This only changes the `justfile`. It does not modify the `##` comment in the corresponding `Makefile` rule.
This rewrites the newly expanded multi-line comment explaining what the `ci-check-msrv` recipe in the `justfile` does, so that it is still readable from top to bottom, but so that the one-line summary is the last line. This is needed because `just --list` only treats a single `#` line, preceding the formal beginning of the recipe, as its documentation. (This differs from most other uses of multi-line comments, such as in a Rust `///` or `//!` comment, where the first line could be a short summary, and subsequent paragraphs are regarded subordinate.) This continues to change only the `justfile` and not to modify the `##` comment in the corresponding `Makefile` rule.
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 seems like it will be less readable if written that way. It may make sense to organize the commands here into more steps, but likely with some steps having 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.
- Split into more steps, logically related. - Identify both toolchains in the step name. - Name the `just` step so it's clear what it does. - Add a TODO for switching to `--minimal-versions`. - Check that `cargo` didn't have to further modify `Cargo.toml`. - Capitalize `RUST_VERSION`, as it is an environment variable.
The verification being done here is exactly what `--locked` is for. Allowing the check to proceed even if it changes the dependencies could have the benefit of showing more information. But it is not obvious that the information it shows would be relevant to the situation being investigated. (If that information would usually be relevant, then `cargo +nightly update -Zminimal-versions` may be too strong, and `cargo +nightly update -Zdirect-minimal-versions` could be considered. Even better than either of those *might* be, somehow, to temporarily downgrade locked dependencies only as much as necessary to make them all MSRV-compatible.) In any case, `--locked` is more readable, and considerably simpler. In addition to making complementary changes to the `msrv.yml` workflow and to the `justfile` recipe it uses, this also changes the `Makefile` rule corresponding to the `justfile` recipe, so they continue to do the same thing if used in local experiments.
This commit splits the commands in the steps of the `check-msrv` job definition to be one per step, documenting each step. In 510847b I predicted that it would not make sense to split the steps to one command per step in `msrv.yml`. The change made there of using `bash` even on Windows allowed for experimenting with how the workflow logs look when reorganized in various ways. But in view of other changes -- and to better clarify that two toolchains were installed, but the MSRV toolchain is set default, so it is used by all subsequent operations *except* the `-Zminimal-versions` dependency downgrade that currently requires `nightly` -- it now looks like having one command per step is better after all. When running only one (simple) command per step, the main change in 510847b is no longer needed. That is, this now avoids that problem in the same way it has been avoided in `test-fast` in `ci.yml` since 4f2ab5b (GitoxideLabs#1559). So this commit also removes `shell: bash` in `msrv.yml` (but keeps the comment clarification in `ci.yml`).
The SVG badge showing the project's MSRV, `etc/msrv-badge.svg`, has four occurrences of the MSRV in it. Three had the current intended MSRV. But the top-level `svg` element's `aria-label` attribute still had the value `rustc: 1.70.0+`, having not been updated the most recent two times the badge SVG was adjusted for a new MSRV. That would, at least potentially, result in screen readers and possibly other software saying/outputting 1.70.0 instead of 1.75.0. This changes the `aria-label` value to `rustc: 1.75.0+`, as intended. This also rephrases the comment in `msrv.yml` about updating the badge when changing the MSRV, to mention the need to change "all occurrences". (We may want to add a CI check to make sure the badge is fully consistent with the current MSRV, but that isn't attempted here.)
This changes `cargo check` commands to `cargo build` in the `ci-check-msrv` recipe in the `justfile`, which the `check-msrv` CI jobs in `msrv.yml` use. (It updates a CI step name accordingly.) The idea is to make sure at least `gix`, with the two combinations of features tested, actually *builds* under the MSRV toolchain. As in f10f18d, this also updates the `Makefile` rule corresponding to that `justfile` recipe. The idea of actually building was suggested in: GitoxideLabs#1808 (comment) However, this does not uncover any new breakages. And there has been further improvement on GitoxideLabs#1808, including in the commits leading up to this, as well as earlier, in 569c186 (GitoxideLabs#1909). Nonetheless, it seems likely that some problems remain with some combinations of crates and features that are not currently exercised in the MSRV check. GitoxideLabs#1808 is most likely not yet fully fixed.
EliahKagan
commented
May 9, 2025
Comment on lines
-129
to
+130
| cargo check --package gix | ||
| cargo check --package gix --no-default-features --features async-network-client,max-performance | ||
| cargo build --locked --package gix | ||
| cargo build --locked --package gix --no-default-features --features async-network-client,max-performance |
There was a problem hiding this comment.
I don't know why we have a check-msrv-on-ci rule in the Makefile, equivalent to the ci-check-msrv recipe in the justfile that the msrv.yml workflow uses. I've expanded the comment only in the justfile. But I've updated the command in the Makefile, rather than only in the justfile, so that their behavior can remain equivalent as long as we keep duplicating functionality across both.
There was a problem hiding this comment.
Thanks for bringing this up. It also seems like this target isn't used at all, so I think it's better to remove it.
There was a problem hiding this comment.
Thanks. I'll remove it from Makefile in a subsequent PR.
More broadly, I'm not clear on what is supposed to go in the Makefile and what is supposed to go in the justfile. Is the Makefile just left over from before the justfile was introduced, such that everything there is there is for historical reasons only? Should its contents be migtated to the justfile? Or are there reason to prefer that some things be in the Makefile instead of the justfile?
There was a problem hiding this comment.
It's a left-over, and the port to justfile was never completed.
make stress is still called, and it could be that it leverages the natural parallelism of make which is harder to do in a justfile.
There was a problem hiding this comment.
Ah, I see. I'll remember to consider the effect on parallelism if and when moving things, especially if they are not already duplicated, from the Makefile to the justfile. The check-msrv-on-ci rule/target probably doesn't benefit from that, though.
Byron
reviewed
May 9, 2025
Comment on lines
-129
to
+130
| cargo check --package gix | ||
| cargo check --package gix --no-default-features --features async-network-client,max-performance | ||
| cargo build --locked --package gix | ||
| cargo build --locked --package gix --no-default-features --features async-network-client,max-performance |
There was a problem hiding this comment.
Thanks for bringing this up. It also seems like this target isn't used at all, so I think it's better to remove it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes several improvements to the MSRV check. Most are to make clearer what is going on, in the
msrv.ymlworkflow and elsewhere, by modifying and expanding comments, and by slightly reorganizing the commands. But this also improves the robustness of the check, by:rustupcommands were to fail, but only on Windows, then the third command that was in that same step with them could cause the whole step to fail--sincepwshis used by default on Windows and it doesn't have anything analogous to-e--whereupon the usually more recent toolchain preinstalled in thewindows-2022runner would be tested with, instead of the MSRV. I don't think this has actually happened in any runs so far, but I think it's worth preventing entirely.--lockedto thecargocommands in theci-check-msrvrecipe, so they fail if they would have to further modifyCargo.lock, rather than modifying it and continuing. This is because the effects of-Zminimal-versionsare non-obvious and subject to change (--minimal-versionshas not been stabilized), and also to try to account at least partially for possible disagreement in what the minimal dependency versions are between thenightlytoolchain that is used to downgrade the locked dependencies (because onlynightlyhas-Zminimal-versions) and the old stable toolchain versioned at the MSRV.cargo buildinstead ofcargo checkin theci-check-msrvrecipe, as suggested in #1808 (comment). (Please note, however, that #1808 is still probably not completely fixed. My guess is that the build problems noted there occurred, and could still occur, in some features, or some crates, that are not covered by the commands in the recipe.)This also fixes an accessibility bug in the MSRV badge, where the
aria-labelattribute of the top-levelsvgelement had the valuerustc: 1.70.0+, instead of the correct and intended valuerustc: 1.75.0+that was already present in the other places in the SVG code where the version appears (dc1d271).