Skip to content

Improve MSRV check and fix aria-label in MSRV badge #2003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 9, 2025
Merged

Improve MSRV check and fix aria-label in MSRV badge #2003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6cbc916
Idea for expanded and clarified `ci-check-msrv` recipe doc
EliahKagan May 8, 2025
af70bfa
Rewrite expanded `ci-check-msrv` doc so `just --list` works
EliahKagan May 8, 2025
510847b
Ensure the Windows `check-msrv` job uses the intended toolchain
EliahKagan May 9, 2025
4af6ef9
Make the `check-msrv` jobs more robust and documented
EliahKagan May 9, 2025
f10f18d
Use `--locked` instead of checking `Cargo.lock` afterwards
EliahKagan May 9, 2025
eaecc9b
Further split `check-msrv` steps; let Windows use `pwsh` again
EliahKagan May 9, 2025
dc1d271
doc: Fix accessibility bug in MSRV badge
EliahKagan May 9, 2025
3a63c68
Actually build `gix` to check MSRV
EliahKagan May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 13 additions & 7 deletions .github/workflows/msrv.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,21 @@ 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
rust_version: 1.75.0
# 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:
- 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 }}
- name: Set up ${{ env.RUST_VERSION }} (MSRV) and nightly toolchains
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: |
# TODO(msrv): Use `cargo update --minimal-versions` when `--minimal-versions` is available.
cargo +nightly update -Zminimal-versions
- run: just ci-check-msrv
- name: Run some `cargo build` commands on `gix`
run: just ci-check-msrv
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 build --locked --package gix
cargo build --locked --package gix --no-default-features --features async-network-client,max-performance
Comment on lines -129 to +130
Copy link
Member Author

@EliahKagan EliahKagan May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


##@ Maintenance

Expand Down
2 changes: 1 addition & 1 deletion etc/msrv-badge.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 8 additions & 3 deletions justfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,11 +238,16 @@ 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
# 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.
#
# 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 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:
Expand Down
Loading