From 6cbc91613659993b18d30d9b76369fbfdca46f15 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 8 May 2025 18:11:06 -0400 Subject: [PATCH 1/8] Idea for expanded and clarified `ci-check-msrv` recipe doc 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. --- justfile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/justfile b/justfile index 2a9255e07f0..73e6a3369b3 100755 --- a/justfile +++ b/justfile @@ -238,7 +238,11 @@ cross-test-android: (cross-test 'armv7-linux-androideabi' '--no-default-features check-size: etc/check-package-size.sh -# Check the minimal support Rust version, with the currently installed Rust version +# Assume the current default toolchain is the Minimal Supported Rust Version and check against it +# +# This is run on CI in `msrv.yml`, after the MSRV toolchain is installed and set as default, and +# after dependencies in `Cargo.lock` are downgraded to the latest MSRV-compatible versions. +# Only if those or similar steps are done first does this recipe really validate the MSRV. ci-check-msrv: rustc --version cargo check -p gix From af70bfa7b5ef382981f4947132649d1e09ca81d9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 8 May 2025 18:17:35 -0400 Subject: [PATCH 2/8] Rewrite expanded `ci-check-msrv` doc so `just --list` works 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. --- justfile | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/justfile b/justfile index 73e6a3369b3..e22a81a17f3 100755 --- a/justfile +++ b/justfile @@ -238,11 +238,12 @@ cross-test-android: (cross-test 'armv7-linux-androideabi' '--no-default-features check-size: etc/check-package-size.sh -# Assume the current default toolchain is the Minimal Supported Rust Version and check against it +# This assumes the current default toolchain is the Minimal Supported Rust Version and checks +# against it. This is run on CI in `msrv.yml`, after the MSRV toolchain is installed and set as +# default, and after dependencies in `Cargo.lock` are downgraded to the latest MSRV-compatible +# versions. Only if those or similar steps are done first does this work to validate the MSRV. # -# This is run on CI in `msrv.yml`, after the MSRV toolchain is installed and set as default, and -# after dependencies in `Cargo.lock` are downgraded to the latest MSRV-compatible versions. -# Only if those or similar steps are done first does this recipe really validate the MSRV. +# Check the MSRV, *if* the toolchain is set and `Cargo.lock` is downgraded (used on CI) ci-check-msrv: rustc --version cargo check -p gix From 510847b04b8330b1ec332ed691b8f0b2b00305e2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 8 May 2025 21:05:33 -0400 Subject: [PATCH 3/8] 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 (#1559) fixed in the Windows `test-fast` job in `ci.yml` also existed in the Windows `check-msrv` job in `msrv.yml` (which #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 #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. --- .github/workflows/ci.yml | 3 ++- .github/workflows/msrv.yml | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a55e80c820..8728d06747f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -433,7 +433,8 @@ jobs: defaults: run: - shell: bash # Use bash even on Windows, if we ever reenable windows-latest for testing. + # Use `bash` even on Windows, if we ever reenable `windows-latest` for testing. + shell: bash steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index 9d96b3db67c..9e0383fcc7f 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -33,6 +33,11 @@ jobs: # IMPORTANT: adjust etc/msrv-badge.svg as well rust_version: 1.75.0 + defaults: + run: + # Use `bash` even in the Windows job, so any failing command fails its step (due to `-e`). + shell: bash + steps: - uses: actions/checkout@v4 - uses: extractions/setup-just@v3 From 4af6ef92ca57ddfceda514938a7b73c223184707 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 8 May 2025 21:04:44 -0400 Subject: [PATCH 4/8] Make the `check-msrv` jobs more robust and documented - 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. --- .github/workflows/msrv.yml | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index 9e0383fcc7f..a6de1e0834b 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -31,7 +31,7 @@ jobs: env: # dictated by `firefox` to support the `helix` editor, but now probably effectively be controlled by `jiff`, which also aligns with `regex`. # IMPORTANT: adjust etc/msrv-badge.svg as well - rust_version: 1.75.0 + RUST_VERSION: 1.75.0 defaults: run: @@ -41,8 +41,15 @@ jobs: steps: - uses: actions/checkout@v4 - uses: extractions/setup-just@v3 - - run: | - rustup toolchain install ${{ env.rust_version }} nightly --profile minimal --no-self-update - rustup default ${{ env.rust_version }} - cargo +nightly update -Zminimal-versions - - run: just ci-check-msrv + - name: Set up ${{ env.RUST_VERSION }} (MSRV) and nightly toolchains + run: | + rustup toolchain install ${{ env.RUST_VERSION }} nightly --profile minimal --no-self-update + rustup default ${{ env.RUST_VERSION }} + - name: Downgrade locked dependencies to lowest allowed versions + run: | + cargo +nightly update -Zminimal-versions # TODO(msrv): Use non-`-Z` way when available. + git add Cargo.lock # Stage for a later `git diff` check. + - name: Run some `cargo check` commands on `gix` + run: just ci-check-msrv + - name: Check `cargo` didn't have to change the versions + run: git diff --exit-code -- Cargo.lock From f10f18d21eeb25da199a88f778484ac1e33c7a98 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 8 May 2025 22:12:44 -0400 Subject: [PATCH 5/8] Use `--locked` instead of checking `Cargo.lock` afterwards 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. --- .github/workflows/msrv.yml | 3 --- Makefile | 4 ++-- justfile | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index a6de1e0834b..e0ae4908eca 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -48,8 +48,5 @@ jobs: - name: Downgrade locked dependencies to lowest allowed versions run: | cargo +nightly update -Zminimal-versions # TODO(msrv): Use non-`-Z` way when available. - git add Cargo.lock # Stage for a later `git diff` check. - name: Run some `cargo check` commands on `gix` run: just ci-check-msrv - - name: Check `cargo` didn't have to change the versions - run: git diff --exit-code -- Cargo.lock diff --git a/Makefile b/Makefile index fea4c7229ec..3f9fd3bbfdd 100644 --- a/Makefile +++ b/Makefile @@ -126,8 +126,8 @@ bench-gix-config: check-msrv-on-ci: ## Check the minimal support rust version for currently installed Rust version rustc --version - cargo check --package gix - cargo check --package gix --no-default-features --features async-network-client,max-performance + cargo check --locked --package gix + cargo check --locked --package gix --no-default-features --features async-network-client,max-performance ##@ Maintenance diff --git a/justfile b/justfile index e22a81a17f3..9cd3f930359 100755 --- a/justfile +++ b/justfile @@ -246,8 +246,8 @@ check-size: # Check the MSRV, *if* the toolchain is set and `Cargo.lock` is downgraded (used on CI) ci-check-msrv: rustc --version - cargo check -p gix - cargo check -p gix --no-default-features --features async-network-client,max-performance + cargo check --locked -p gix + cargo check --locked -p gix --no-default-features --features async-network-client,max-performance # Enter a nix-shell able to build on macOS nix-shell-macos: From eaecc9b270269536308359c94d774ab388849f18 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 9 May 2025 00:00:07 -0400 Subject: [PATCH 6/8] Further split `check-msrv` steps; let Windows use `pwsh` again 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 (#1559). So this commit also removes `shell: bash` in `msrv.yml` (but keeps the comment clarification in `ci.yml`). --- .github/workflows/msrv.yml | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index e0ae4908eca..07fbdd21018 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -33,20 +33,16 @@ jobs: # IMPORTANT: adjust etc/msrv-badge.svg as well RUST_VERSION: 1.75.0 - defaults: - run: - # Use `bash` even in the Windows job, so any failing command fails its step (due to `-e`). - shell: bash - steps: - uses: actions/checkout@v4 - uses: extractions/setup-just@v3 - name: Set up ${{ env.RUST_VERSION }} (MSRV) and nightly toolchains - run: | - rustup toolchain install ${{ env.RUST_VERSION }} nightly --profile minimal --no-self-update - rustup default ${{ env.RUST_VERSION }} + run: rustup toolchain install ${{ env.RUST_VERSION }} nightly --profile minimal --no-self-update + - name: Set ${{ env.RUST_VERSION }} (MSRV) as default + run: rustup default ${{ env.RUST_VERSION }} - name: Downgrade locked dependencies to lowest allowed versions run: | - cargo +nightly update -Zminimal-versions # TODO(msrv): Use non-`-Z` way when available. + # TODO(msrv): Use `cargo update --minimal-versions` when `--minimal-versions` is available. + cargo +nightly update -Zminimal-versions - name: Run some `cargo check` commands on `gix` run: just ci-check-msrv From dc1d271d48b7c41134eb842ff9bcbcd1adeb7330 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 9 May 2025 00:57:24 -0400 Subject: [PATCH 7/8] doc: Fix accessibility bug in MSRV badge 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.) --- .github/workflows/msrv.yml | 5 +++-- etc/msrv-badge.svg | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index 07fbdd21018..985c79c07f6 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -29,8 +29,9 @@ jobs: runs-on: ${{ matrix.os }} env: - # dictated by `firefox` to support the `helix` editor, but now probably effectively be controlled by `jiff`, which also aligns with `regex`. - # IMPORTANT: adjust etc/msrv-badge.svg as well + # This is dictated by `firefox` to support the `helix` editor, but now probably effectively + # be controlled by `jiff`, which also aligns with `regex`. + # IMPORTANT: When adjusting, change all occurrences in `etc/msrv-badge.svg` as well. RUST_VERSION: 1.75.0 steps: diff --git a/etc/msrv-badge.svg b/etc/msrv-badge.svg index 184a0688c61..3d42373fe68 100644 --- a/etc/msrv-badge.svg +++ b/etc/msrv-badge.svg @@ -1,4 +1,4 @@ - + rustc: 1.75.0+ From 3a63c68d079ee79e5e2c8ef3846ef11d2c1c7891 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 9 May 2025 00:06:41 -0400 Subject: [PATCH 8/8] Actually build `gix` to check MSRV 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: https://github.com/GitoxideLabs/gitoxide/issues/1808#issuecomment-2614260268 However, this does not uncover any new breakages. And there has been further improvement on #1808, including in the commits leading up to this, as well as earlier, in 569c186 (#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. #1808 is most likely not yet fully fixed. --- .github/workflows/msrv.yml | 2 +- Makefile | 4 ++-- justfile | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml index 985c79c07f6..2d7043d113a 100644 --- a/.github/workflows/msrv.yml +++ b/.github/workflows/msrv.yml @@ -45,5 +45,5 @@ jobs: run: | # TODO(msrv): Use `cargo update --minimal-versions` when `--minimal-versions` is available. cargo +nightly update -Zminimal-versions - - name: Run some `cargo check` commands on `gix` + - name: Run some `cargo build` commands on `gix` run: just ci-check-msrv diff --git a/Makefile b/Makefile index 3f9fd3bbfdd..431c041979d 100644 --- a/Makefile +++ b/Makefile @@ -126,8 +126,8 @@ bench-gix-config: check-msrv-on-ci: ## Check the minimal support rust version for currently installed Rust version rustc --version - cargo check --locked --package gix - cargo check --locked --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 ##@ Maintenance diff --git a/justfile b/justfile index 9cd3f930359..7f5c71dd24a 100755 --- a/justfile +++ b/justfile @@ -246,8 +246,8 @@ check-size: # Check the MSRV, *if* the toolchain is set and `Cargo.lock` is downgraded (used on CI) ci-check-msrv: rustc --version - cargo check --locked -p gix - cargo check --locked -p gix --no-default-features --features async-network-client,max-performance + cargo build --locked -p gix + cargo build --locked -p gix --no-default-features --features async-network-client,max-performance # Enter a nix-shell able to build on macOS nix-shell-macos: