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

@Byron Byron May 9, 2025

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

@EliahKagan EliahKagan May 9, 2025

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

@Byron Byron May 9, 2025

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

@EliahKagan EliahKagan May 9, 2025

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.

Copy link
Member Author

@EliahKagan EliahKagan May 25, 2025

Choose a reason for hiding this comment

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

Or maybe casey/just#626 will be completed--for example, maybe casey/just#1562 will be picked back up--and then they could all be moved over to the justfile.

Copy link
Member

@Byron Byron May 26, 2025

Choose a reason for hiding this comment

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

That would be nice.

My general feeling about just was that it was supposed to remain a sequential task runner though, and thus far the community around it didn't feel strongly enough about it to fork and merge in their desired solution. So it's probably nothing more than a wildcard at this point.

Copy link
Member Author

@EliahKagan EliahKagan Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed that casey/just#626 has very recently been implemented in casey/just#2803 in the form of a new [parallel] attribute. My understanding is that, if a recipe $Y$ declares recipes $X_1$, $X_2$, and $X_3$ as dependencies and $Y$ is marked [parallel], then running $Y$ causes just to run $X_1$, $X_2$, and $X_3$ in parallel rather than sequentially.

This is not equivalent to -j for make. However, at least for make stress, it seems to me that [parallel] does exactly what we want, and that we are currently using make -j as a workaround for not having such a feature. The stress recipe runs various commands with time, but the first command it runs is only to set things up for the subsequent commands, and it does not use time, nor any other shell features.

gitoxide/Makefile

Line 98 in c7af04d

$(MAKE) -j3 $(linux_repo) $(rust_repo) release-lean

That first command runs three other recipes with $(MAKE) -j3 so they run in parallel, and the recipe declares no dependencies. Therefore, conceptually it has the three recipes identified in the first command as dependencies, but it runs them through a command with $(MAKE) instead, to be able to pass -j3.

Furthermore, as far as I can tell, while stress runs various recipes directly and indirectly, this is the only place where -j parallelism is involved. Even if one runs make -jN stress with one's chosen N, I don't think one gets greater parallelism, nor do I think greater parallelism is intended when running the stress test.

Per casey/just#626 (comment), [parallel] will be available starting in the next release of just. It seems to me that whether this will let us move everything from the Makefile into the justfile depends on how accurate the note at the top of the make help text is:

Note: Make is only for specific functionality used by CI - run just for developer targets

Assuming that is (still) true, it seems to me that [parallel] will let us move everything into the justfile (or, if preferred, a separate justfile, though I don't think that would be preferred). This is because, at least going by what I see when I inspect the output of git grep -Fwn make, the only use of make on CI--looking only at uses that that involve our Makefile--is to run make stress in cron.yml.

I believe there are other indirect implicit uses of make in building native code dependencies, as occurs when some features of some crates are enabled, and for this reason we do install make in containers that may not have it, in some other CI jobs. But these would not have anything to do with the Makefile in this project.

However, if make -jN is also being used manually for local operations, possibly explicitly specifying multiple recipes on the command line, then that is a different story. There may also be other relevant subtleties.

Although I don't know that we should do it regularly, I think it would be useful to be able to run the stress test on Windows. I expect that eliminating the Makefile would ease portability, since various interesting things can happen with Makefiles on Windows. But it may not be necessary.

(As an aside, torvalds/linux is one of the test repositories used there, and just cloning that on Windows is very slow, even when it is a bare clone. However, since the test uses a bare clone, it should at least be able to run, since the usual problems such as case clashes and conflicts with the reserved DOS device name AUX should not apply.)

Copy link
Member

@Byron Byron Jul 8, 2025

Choose a reason for hiding this comment

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

Thanks a lot for sharing!

After reading this I arrive at the same conclusion and can confirm that with [parallel] our Makefile wouldn't be needed anymore. I also think it's valuable to not have a Makefile when just would now do in every way.

Thus I very much welcome this transition to happen whenever possible, and whenever you have some time.


##@ 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