Make MSRV checks and updates easier and more thorough #2026

EliahKagan · 2025-05-25T20:59:57Z

This PR could be regarded as a sequel to #2003. Prior to this PR, the MSRV had to be updated manually in Cargo.toml for the gix crate, in the msrv.yml workflow, and in the four occurrences appearing in msrv-badge.svg. No mechanism was in place to automate or verify that this had been done correctly.

The changes in this PR make the rust-version in Cargo.toml for the gix crate the single source of truth for the MSRV. This pertains to how the MSRV is validated and the actions that are undertaken to change it, so no change to STABILITY.md is made or required; this also does not change how modifying the MSRV will sometimes involve adjusting rust-version in other gix-* crates.

In the justfile, besides makes some minor adjustments for robustness and refactoring for clarity, this makes three external-facing recipe changes:

A new msrv recipe uses cargo metadata and jq to retrieve the rust-version of gix and report it in major.minor.patch format.
A new msrv-badge recipe calls the msrv recipe and writes a badge with the current MSRV to etc/msrv-badge.svg.

This is supported by a new MSRV badge template with the literal text {MSRV} in place of the MSRV added in etc; the msrv-badge recipe substitutes those placeholders with the actual MSRV.
The old ci-check-msrv recipe was confusing, because it didn't do anything MSRV-specific: it checked that gix could build in a couple of ways using the current default toolchain. (Comment changes in #2003 attempted to make this clearer, but I think they managed to lessen the confusion only slightly, if at all.)

This confusion is mostly fixed here, by renaming the recipe to check-rust-version and having it require a rust-version argument. Regardless of whatever toolchain is configured as the default, it uses the toolchain specified by the rust-version argument (which in ordinary use will be a version number) for the commands it runs.

This facilitates local use, such as running just msrv-badge to update the MSRV badge after modifying gix/Cargo.toml, but the more important effect is to support changes in the msrv.yml workflow:

The hard-coded MSRV environment variable in msrv.yml is removed. The check-msrv job definition in that workflow instead uses just msrv to find out what MSRV toolchain to preinstall, and what argument to pass in just check-rust-version. Passing this argument to the check-rust-version recipe eliminates the need to set a default toolchain.

(The main reason the entire MSRV check is not just added as a check-msrv recipe in the justfile, with the idea that the check-msrv jobs could call that, is that check-msrv in msrv.yml has an intermediate step that downgrades versions in Cargo.lock. It might be possible to take a different approach, or to add a justfile recipe for that as well, but it's not something that should be done locally by just commands that don't clearly advertise it, and it also feels like it doesn't belong in the scope of this PR.)
A new check-msrv-badge job is added to msrv.yml. This verifies that etc/msrv-badge.svg is correct for the current MSRV, by calling just msrv-badge, checking if there are any changes, and failing if so.

This should make it so that the badge cannot get out of date from forgetting to update it, as well as avoid problems like the aria-label bug fixed in dc1d271 (#2003) where the badge was in an inconsistent state presenting different MSRVs depending on how it was examined.

(This is its own job rather than a step in check-msrv because it is conceptually separate from that, and also because there are two check-msrv jobs--it uses a matrix strategy--but there should only be one check-msrv-badge job.)
An msrv-pass job, which we may or may not keep, the considerations for which are expounded below.

This also removes the disused check-msrv-on-ci rule from the Makefile (#2003 (comment)).

I believe the check-msrv-badge job ought to block PR auto-merge--as the check-msrv jobs do--since changes to the MSRV may be made in PRs and any resulting inconsistency is most useful to catch immediately when it arises. Currently, the check-msrv jobs are listed as required checks (which is not something I can change, via a PR or otherwise).

To support doing that without making branch protection rules more complicated, I've also added an msrv-pass job to msrv.yml that depends on the jobs in that workflow in the same way that the tests-pass job in ci.yml depends on most of the other jobs there. (I think it's possible to do this across workflows, but more complicated, and it doesn't seem like it would be worthwhile to do for gitoxide's workflows at this time. That is something I am hoping to set up for GitPython, though.) So one option is to remove the current required check(s) for msrv.yml and add msrv-pass as a required check.

But an alternative is to get rid of the new msrv-pass job, and instead merge the msrv.yml workflow into ci.yml--that is, to put the check-msrv and check-msrv-badge job definitions in ci.yml. I think the workflows have had different triggers in the past, with msrv.yml having run on more branches than ci.yml. But even if so, that is no longer the case. The ci.yml workflow is about 500 lines long, so even if it doubles in size, I don't think it will be too long to read and work in easily. It also contains a wide variety of checks, not limited to running the test suite. Accordingly, it's not obvious that the MSRV checks are more different from the checks that in ci.yml than the various checks already in ci.yml are from each other.

Because it was equivalent to the `ci-check-msrv` recipe in the `justfile`, and only the latter is used. For details, see: GitoxideLabs#2003 (comment)

The journey tests need to know where the target directory is. For robustness, we have not been assuming it is `target`, but instead attempting to allow it to be in any location, and querying cargo metadata to find out where it is. (Doing so has the effect of taking care of resolving it to an absolute path, too.) This preserves that, and continues to use `justfile` logic to find it, but does so in a more streamlined way: - Pass `--no-deps` to `cargo metadata`, since dependencies should not affect the location of the target directory. - In the `jq` command that processes `cargo metadata` output, pass `-e`/`--exit-status` so `jq` reports a failing status if the query fails (such as by attempting to look up a missing key, due to `cargo metadata` having failed or otherwise not producing the expected kind of output). - For `jq`, express `-r` as `--raw-output (and `-e` as `--exit-status`) so that the meaning is more readily clear. - Extract a generalized `cargo metadata ... | jq ...` recipe with the flags we're using on both commands. This is parameterized on the `jq` query argument only. This is to make the code easier to read, and also in anticipation of other uses that would also want those flags and no others but with different query strings (such as to extract the MSRV). And removes checks that are now unnecessary: - Don't do a separate check for empty `jq` output anymore, since the goal is to check for an absent key (or totally empty input), and `-e`/`--exit-status` takes care of that. - Don't repeat `dbg` as a prerequisite for the journey test recipes that rely on it, instead allowing the recipes to proceed until they get to where they really use it (which is by interpolation), because `--no-deps` should avoid any major delays at this point as well as making failure unlikely (but if it does fail, that is still checked and will still fail the recipe).

This makes the `rust-version` in the manifest file for the `gix` crate the single source of truth for the MSRV. CI checks use this rather than relying on a separate copy of the MSRV. The MSRV badge still hard-codes the MSRV (in four places in its SVG code), but a recipe is added to regenerate it based on the MSRV, and a CI check is added to verify that it agrees with the MSRV. Specifically, the changes are: 1. Add an `msrv` recipe to the `justfile` that extracts the MSRV from the value of `rust-version` in the `gix` package, using `cargo metadata` and `jq`, and that formats it in the X.Y.Z form (since that form is slightly more widely usable than the X.Y form even when Z is 0). 2. Remove the hard-coded MSRV in `msrv.yml` (in `env.RUST_VERSION`) and instead have the `check-msrv` job in `msrv.yml` get it from the `gix` manifest by callig `just msrv`. This fixes the problem that it was possible to forget to update the MSRV in the workflow. 3. Replace the `ci-check-msrv` recipe in the `justfile` with a `check-rust-version` recipe taking a `rust-version` argument. Instead of implicitly using the current default toolchain and relying on that being the MSRV, this explicitly runs its `rustc` and `cargo` commands with the specified `rust-version`. The `check-msrv` CI job now calls this, passing the MSRV. The reason to have a `check-rust-version` recipe instead of a `check-msrv` recipe (or both recipes with the latter delegating to the former) is that CI usage, as well as other anticipated usage, involves performing additional operations between finding out the MSRV and attempting to build with it: - At least on CI, it is clearer to install the needed toolchains before using them, even though specifying them explicitly in `rustc` or `cargo` commands will try to install them. - More importantly, on CI and locally, the `check-rust-version` recipe is not currently expected to work with the MSRV when the committed version of `Cargo.lock` is used, because some locked dependencies may have later MSRVs. Depending on precisely what one is testing, one could temporarily remove `Cargo.lock` and regenerate it using the MSRV toolchain, or (as done on CI) temporarily downgrade the versions in `Cargo.lock`. 4. Add an `msrv-badge` recipe to the `justfile` that regenerates the MSRV badge `etc/msrv-badge.yml` based on the MSRV from the `gix` manifest, as obtained via `just msrv`. This uses a template file, added alongside the badge, where each place where the MSRV should appear has a literal placeholder `{MSRV}` instead. `just msrv-badge` copies the template, with the actual MSRV value subtituted for the placeholders, to the badge, overwriting whatever is there. 5. Add a `check-msrv-badge` job to `msrv.yml` that checks out the code, runs `just msrv-badge`, and checks if there are any changes. If so, the committed badge is out of date. 6. Add an `msrv-pass` job to `msrv.yml` that depends on the `check-msrv` and `check-msrv-badge` jobs. This is analogous to the `tests-pass` job in `ci.yml`. It is so that, if having the MSRV badge out of date should block PR auto-merge, then that can be achieved without `msrv.yml` having to contribute more than one required check to the branch protection rules. (This also temporarily adds a `check-msrv` job for `macos-15`, but that is just to verify that recent `justfile` changes are portable. It will be removed shortly.)

This is clearer than the inline shell script previously used, and also more robust.

`cargo metadata` already validates the version, checking that it's a string satisfying one of a small number of recognized formats. So the only validation `jq` needs to do is to make sure it is really only appending `.0` to the end if it is of the form `X.Y` for nonempty numeric `X` and `Y` (with no other characters anywhere). This shortens the `jq` query considerably, removing the unnecessary validation and also using regular expression replacement to replace the empty string that follows the a *full* `X.Y` line with `.0`.

Because regular expressions in a `jq` query are strings, their `\` escapes must be written as `\\`. But the `get-metadata` recipe in the `justfile` is strangely intolerant of `\\` in a cross-platform setting. On Windows, even though `sh` is used on both, and even though the `quote()` function is supposed to work the same way on both, the `\\` gets folded into `\` in the actual query argument passed to `jq`, and `just msrv` fails with: cargo metadata --format-version 1 --no-deps | jq --exit-status --raw-output -- '.packages[] | select(.name == "gix") | .rust_version | sub("^\\d+\\.\\d+\\K$"; ".0")' jq: error: Invalid escape at line 1, column 4 (while parsing '"\d"') at <top-level>, line 1: .packages[] | select(.name == "gix") | .rust_version | sub("^\d+\.\d+\K$"; ".0") jq: error: Invalid escape at line 1, column 6 (while parsing '"\.\d"') at <top-level>, line 1: .packages[] | select(.name == "gix") | .rust_version | sub("^\d+\.\d+\K$"; ".0") jq: error: Invalid escape at line 1, column 4 (while parsing '"\K"') at <top-level>, line 1: .packages[] | select(.name == "gix") | .rust_version | sub("^\d+\.\d+\K$"; ".0") jq: 3 compile errors error: Recipe `get-metadata` failed on line 191 with exit code 3 A Windows-specific workaround is to write `\\\\` instead of `\\`. This works on Windows, but of course breaks on other platforms where it represents a subpattern of `\\` and thus matches only a literal `\` (which is not present and shouldn't be matched if it were). In this case, it so happens that the pattern was also potentially confusing for other reasons, due to the way it matched a zero-width string after a `\K`-discarded sub-match that did most of the work. Because `\\d` and `\\.` are easily replaced with character classes that use no `\`, the `\\K` was the only use of `\` in the pattern string that needed `\`. So this replaces that technique with a different more readable one that does not require writing any occurrences of `\\`. (It uses a `\`, but no `\\`, and `\` by itself seems to cause no problems.) Instead of discarding a sub-match and replacing an empty string, this matches on the entire string (if in MAJOR.MINOR form) and replaces the whole thing with itself followed by `.0`. This does make the recipe longer. The `jq` query argument is accordingly now split over multiple lines. (Although this is done using a `'''` string instead of a `'` string, that does not make a difference to the misinterpretation of `\\`, which occurs after the string has been received by the `get-metadata` recipe.)

- Rename `get-metadata` to the more accurate name `query-meta`. Since it is being used to do more processing than just dumping values as they literally appear in the JSON metadata. - Catch if `cargo metadata ...` fails and don't even run `jq`. This shouldn't increase memory requirements much, since `jq` without `--stream` always reads its full input anyway. (A middle ground could be to use `set -o pipefail`, but while that has recently been added to POSIX, `sh` is `dash` on Debian and Ubuntu, and `dash` doesn't yet support `-o pipefail`.) - Simplify `dbg` recipe to have `query-meta` concatenate `/debug`.

- Remove the recently added `macos-15` MSRV check CI job. We don't typically automatically run the MSRV check on macOS CI. That job was temporarily added to help verify that new `justfile` recipes are portable. (They are.) This removes that job. - Show the MSRV in the `check-msrv` "Read the MSRV" step. Show the line being written to set an `MSRV` environment variable for subsequent steps, so it's easy to see the MSRV in the log. - Distinguish MSRV badge changes from unanticipated other changes. In case something goes wrong in the checkout or some other step. - Reword a comment for clarity.

.github/workflows/msrv.yml

EliahKagan · 2025-05-25T21:11:17Z

justfile

+query-meta jq-query:
+    meta="$(cargo metadata --format-version 1 --no-deps)" && \
+        printf '%s\n' "$meta" | jq --exit-status --raw-output -- {{ quote(jq-query) }}


This seems useful enough that it might make sense to remove [private] so it appears in the output of just --list. But I've refrained from that for now, because I'm not confident that --no-deps will be suitable for future uses. (As detailed at the end of the a8476e1 commit message, it seems somewhat important to have --no-deps here. But I have verified that querying for gix still returns only the intended workspace-member gix metadata, even if a dependency on another gix version, as gix-testtools used to have, is reintroduced.)

This moves the `check-msrv` and `check-msrv-badge` job definitions from `msrv.yml` to `ci.yml`, where they are renamed `msrv` and `msrv-badge`, respectively. This includes those jobs among the PR-blocking checks in `ci.yml` (i.e., `tests-pass` depends on them). This lets failure of `msrv` continue blocking PR auto-merge (`check-msrv` had done so), and causes `msrv-badge` to begin blocking PRs (`check-msrv-badge` had not done so). The `msrv.yml` workflow is accordingly removed. See GitoxideLabs#2026 for context and relevant discussion.

This moves the `check-msrv` and `check-msrv-badge` job definitions from `msrv.yml` to `ci.yml`, where they are renamed `msrv` and `msrv-badge`, respectively. This includes those jobs among the PR-blocking checks in `ci.yml` (i.e., `tests-pass` depends on them). This lets failure of `msrv` continue blocking PR auto-merge (`check-msrv` had done so), and causes `msrv-badge` to begin blocking PRs (`check-msrv-badge` had not done so). The `msrv.yml` workflow is accordingly removed. (See GitoxideLabs#2026 and GitoxideLabs#2027 for context and relevant discussion.)

Byron · 2025-05-26T03:00:07Z

Thanks a lot for cleaning this up and particularly for setting up a single source of truth for the MSRV.

To support doing that without making branch protection rules more complicated, I've also added an msrv-pass job to msrv.yml that depends on the jobs in that workflow in the same way that the tests-pass job in ci.yml depends on most of the other jobs there.

I have adjusted the requirements accordingly, thank you.

But an alternative is to get rid of the new msrv-pass job, and instead merge the msrv.yml workflow into ci.yml

I see that this could be useful, with a strong argument due to ci.yml containing all kinds of tests by now. I'd like to leave it to your judgement if and when to do that, since you are the one dealing with the files more than me.

PS: If I missed anything or there are open questions left, please let me know.

EliahKagan · 2025-05-26T03:03:12Z

But an alternative is to get rid of the new msrv-pass job, and instead merge the msrv.yml workflow into ci.yml

I see that this could be useful, with a strong argument due to ci.yml containing all kinds of tests by now. I'd like to leave it to your judgement if and when to do that, since you are the one dealing with the files more than me.

I do lean slightly toward moving the MSRV checks into the ci.yml workflow, and I've opened #2027 to propose that.

EliahKagan added 8 commits May 24, 2025 11:02

Remove check-msrv-on-ci rule in Makefile

a943e36

Because it was equivalent to the `ci-check-msrv` recipe in the `justfile`, and only the latter is used. For details, see: GitoxideLabs#2003 (comment)

Use jq to validate and format the rust-version

efeb6e6

This is clearer than the inline shell script previously used, and also more robust.

EliahKagan commented May 25, 2025

View reviewed changes

EliahKagan marked this pull request as ready for review May 25, 2025 21:13

EliahKagan enabled auto-merge May 25, 2025 21:14

EliahKagan merged commit 40f5a56 into GitoxideLabs:main May 25, 2025
24 checks passed

EliahKagan deleted the run-ci/check-msrv-next branch May 25, 2025 21:15

EliahKagan mentioned this pull request May 25, 2025

Move MSRV jobs into main ci.yml workflow #2027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Make MSRV checks and updates easier and more thorough #2026

Make MSRV checks and updates easier and more thorough #2026

Uh oh!

EliahKagan commented May 25, 2025 •

edited

Loading

Uh oh!

Uh oh!

EliahKagan May 25, 2025

Uh oh!

Uh oh!

Byron commented May 26, 2025 •

edited

Loading

Uh oh!

EliahKagan commented May 26, 2025

Uh oh!

Uh oh!

Uh oh!

Make MSRV checks and updates easier and more thorough #2026

Make MSRV checks and updates easier and more thorough #2026

Uh oh!

Conversation

EliahKagan commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

EliahKagan May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Byron commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EliahKagan commented May 26, 2025

Uh oh!

Uh oh!

EliahKagan commented May 25, 2025 •

edited

Loading

Byron commented May 26, 2025 •

edited

Loading