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.

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

Jump to bottom

fix check-msrv ci job by resolving to latest compatible dependencies #4972

Merged
merged 6 commits into from
Mar 13, 2025
Merged

fix check-msrv ci job by resolving to latest compatible dependencies #4972

merged 6 commits into from
Mar 13, 2025
+28 −31

Conversation

bschoenmaeckers
Copy link
Member

Once_cell 1.21 only supports rust 1.70. So forcing a lower version using the nox set_msrv_package_versions job.

davidhewitt
davidhewitt approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Mar 12, 2025
@davidhewitt davidhewitt enabled auto-merge March 12, 2025 09:22
@Tpt
Copy link
Contributor

Tpt commented Mar 12, 2025 •
edited
Loading

Aside: A possible way to fix this kind of issues once and for all is to set resolver.incompatible-rust-versions = "fallback" into .config/cargo.toml then use Rust 1.85+ to run cargo update. The resolver will then only use versions compatible with MSRV (this assume all crates uses correct MSRV annotations)

@davidhewitt
Copy link
Member

That sounds like a great idea, we can also do that with an env var on the command-line inside the job, to avoid affecting non-MSRV jobs:

$ CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback cargo update

auto-merge was automatically disabled March 12, 2025 09:35

Head branch was pushed to by a user without write access

@bschoenmaeckers
fix check-msrv ci job by resolving to latest compatible version
7b3a754 
using `resolver.incompatible-rust-versions = "fallback"`
@bschoenmaeckers bschoenmaeckers changed the title fix check-msrv ci job by pinning once_cell fix check-msrv ci job by resolving to latest compatible dependencies Mar 12, 2025
@davidhewitt davidhewitt enabled auto-merge March 12, 2025 09:58
@davidhewitt
Copy link
Member

Looks like that works!

@Tpt
Copy link
Contributor

Tpt commented Mar 12, 2025

Looks like that works!

Amazing! I guess it might be possible to drop all the hardcoded versions and simplify the script significantly?

@davidhewitt davidhewitt added this pull request to the merge queue Mar 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2025
@bschoenmaeckers
Copy link
Member Author

@davidhewitt Could you enable the full pipeline so I can tests the ci before merging?

@bschoenmaeckers bschoenmaeckers force-pushed the fix-check-msrv branch 5 times, most recently from 411d75a to 9b6edf4 Compare March 12, 2025 13:40
@bschoenmaeckers bschoenmaeckers force-pushed the fix-check-msrv branch 4 times, most recently from 99a3ad0 to cd75ec8 Compare March 12, 2025 14:18
Tpt
Tpt reviewed Mar 12, 2025
View reviewed changes
noxfile.py Outdated
# Set the rust-version in the Cargo.toml the generate a compatible lockfile
with open(f"{project}/Cargo.toml", "r") as f:
cargo_toml = toml.load(f)
cargo_toml["package"]["rust-version"] = msrv
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of editing this field dynamically, should we just change the Cargo.toml files?

Copy link
Member Author

Choose a reason for hiding this comment

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

These examples are also used as template for new projects. So I don't think we should recommend this setting to users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'd be up for setting our MSRV in the examples to match our MSRV. Users can then choose to bump their MSRV higher.

Not modifying the Cargo.toml makes this much more convenient to run locally (which can be useful to reproduce issues with MSRV).

@bschoenmaeckers
Copy link
Member Author

Amazing! I guess it might be possible to drop all the hardcoded versions and simplify the script significantly?

Unfortunately this does not work because some crates do not correctly set their MSRV.

@davidhewitt
Copy link
Member

Thanks for this, I pushed some additional commits to simplify and move this forward to unblock CI 😃

@davidhewitt davidhewitt enabled auto-merge March 13, 2025 19:03
@davidhewitt davidhewitt added this pull request to the merge queue Mar 13, 2025
Merged via the queue into PyO3:main with commit 980268d Mar 13, 2025
88 of 89 checks passed
@davidhewitt davidhewitt mentioned this pull request Mar 13, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tpt Tpt Tpt left review comments

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
CI-build-full CI-skip-changelog Skip checking changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bschoenmaeckers @Tpt @davidhewitt