-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(add): Stabilize MSRV-aware version req selection #13608
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
This is part of rust-lang#9930 for rust-lang/rfcs#3537 This will make it easier to maintain an MSRV-compliant `Cargo.toml` but leaves validation up to the user as the resolver will pick newer versions. This helps the MSRV-aware workflows enumerated in rust-lang/rfcs#3537 though it could be confusing to the workflow with an MSRV-compatible lockfile. PR rust-lang#13561 at least raises awareness of that discrepancy. There is an unresolved question on differences in the resolver vs `cargo add` for dealing with an unset `rust-version`. However, we are only stabilizing the `cargo add` side which is a very light two-way door as this is a UX-focused command and not a programmatic command. This hasn't gotten much end-user testing but, as its UX focused, that seems fine. As such, this seems like it is ready to stabilize.
|
Major launchpad outage according to https://status.canonical.com/ |
While the stabilization changes still need a review, I figure we could do the high level "should we stabilize" conversation in parallel. Before, when a user ran With this change, we still give priority to the version req the user selected. However, if they did not specify one, instead of always picking the latest version to make a version req from, instead we'll filter out versions incompatible with This does not include Differences with the resolver
This is part of RFC #3537. being tracked in #9930. The intention with the RFC is that we could stabilize each individual piece, rather than waiting for everything to be ready. @rfcbot fcp merge |
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good.
To whom wants to test the change, simply run:
zsh
fpath+=$PWD/src/etc
autoload -Uz compinit
compinit
cargo add --i<tab>
cargo add --<tab> # to review the flag help text
bash
. ./src/etc/cargo.bashcomp.sh
cargo add --i<tab>
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I'm not quite clear on how this works with the resolver. What happens if:
Does it add |
This is independent of the resolver. The version requirement will be MSRV-compatible ( For users who don't track MSRV-comptible dependencies in their lockfile, this is what they expect and things will be fine (until we change the resolver). For users who do track MSRV-compatible dependencies, this removes one extra step for them. They will have a valid So while this isn't a complete solution for everyone, until the resolver is stable, it does improvements for everyone. |
@ehuss this does get me thinking that maybe we could update the
message to
Depending on which resolve behavior is being invoked. |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
The FCP is complete. Thank you to everyone involved. @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 11 commits in 28e7b2bc0a812f90126be30f48a00a4ada990eaa..74fd5bc730b828dbc956335b229ac34ba47f7ef7 2024-04-05 19:31:01 +0000 to 2024-04-10 18:40:49 +0000 - chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#13731) - fix(cargo-fix): dont apply same suggestion twice (rust-lang/cargo#13728) - refactor: make `resolve_with_previous` clearer (rust-lang/cargo#13727) - fix(package): Normalize paths in `Cargo.toml` (rust-lang/cargo#13729) - refactor: Track when MSRV is explicitly set, either way (rust-lang/cargo#13732) - [fix]:Build script not rerun when target rustflags change (rust-lang/cargo#13560) - feat(add): Stabilize MSRV-aware version req selection (rust-lang/cargo#13608) - Fix github fast path redirect. (rust-lang/cargo#13718) - Add release notes for 1.77.1 (rust-lang/cargo#13717) - doc(semver): remove mention of deprecated tool rust-semverver (rust-lang/cargo#13715) - chore: fix some typos (rust-lang/cargo#13714) r? ghost
What does this PR try to resolve?
This is part of #9930 for rust-lang/rfcs#3537
This will make it easier to maintain an MSRV-compliant
Cargo.toml
but leaves validation up to the user as the resolver will pick newer versions.This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537
How should we test and review this PR?
As for determining if this is ready for stabilization:
By stabilizing this without the MSRV-aware resolver, this could be confusing to the workflow with an MSRV-compatible lockfile.
PR #13561 at least raises awareness of that discrepancy.
In general there was interest in the RFC discussions to stabilize this ASAP, regardless of what resolver direction we went.
There is an unresolved question on differences in the resolver vs
cargo add
for dealing with an unsetrust-version
(noted in the tracking issue). However, we are only stabilizing thecargo add
side which is a very light two-way door as this is a UX-focused command and not a programmatic command.This hasn't gotten much end-user acceptance testing but, as its UX focused, that seems fine (light, two way door)
As such, this seems like it is ready to stabilize.
Additional information