Skip to content
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.

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

Check crate MSRVs #1026

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

Conversation

fuzzypixelz
Copy link
Member

This add a check for the MSRV of all crates in the workspace. Each crate has its own MSRV specified through package.rust-version.

@fuzzypixelz fuzzypixelz marked this pull request as draft May 14, 2024 12:19
@fuzzypixelz fuzzypixelz marked this pull request as draft May 14, 2024 12:19
@fuzzypixelz fuzzypixelz added enhancement Existing things could work better internal Changes not included in the changelog labels May 24, 2024
@fuzzypixelz fuzzypixelz changed the title Check MSRV Check crate MSRVs May 27, 2024
@Mallets
Copy link
Member

Mallets commented May 28, 2024

@fuzzypixelz what's the status of this PR? Is it ready for review?

@fuzzypixelz
Copy link
Member Author

@fuzzypixelz what's the status of this PR? Is it ready for review?

Right now it seems to report incorrect MSRVs. I would have to investigate that first.

@fuzzypixelz fuzzypixelz self-assigned this Jun 10, 2024
@fuzzypixelz
Copy link
Member Author

fuzzypixelz commented Jun 10, 2024

The issue wasn't that the job was reporting incorrect MSRVs. But that the MSRV of the crate zenohd depends on the target platform it's compiled for.

On the ubuntu-latest runner (i.e. x86_64-unknown-linux-gnu) the MSRV is 1.70. But on my Apple Silicon machine (i.e. aarch64-apple-darwin) the MSRV is 1.72.

This MSRV business is more complicated than I first thought...

@fuzzypixelz
Copy link
Member Author

The problem now is handling platform-specific MSRVs. Right now we check that the rust-version field is equal to the MSRV.

If crate C1 on platform P1 has an MSRV of V1 and an MSRV of V2 on platform P2. Then the checks on platforms P1 and P2 cannot both succeed.

We cannot tolerate a rust-version lower than the MSRV (here MSRV means the minimum version that builds). So we should instead tolerate rust-versions higher than the MSRV and emit a warning for good measure.

@fuzzypixelz
Copy link
Member Author

The other problem is that cargo-msrv fails to install toolchains on windows-latest with error:

error: could not create link from 'C:\\Users\\runneradmin\\.cargo\\bin\\rustup.exe' to 'C:\\Users\\runneradmin\\.cargo\\bin\\cargo.exe'

It seemingly cannot create a (symbolic? hard?) link. This is very odd since the check job installs the Stable toolchain successfully on windows-latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants