feat(add): Stabilize MSRV-aware version req selection #13608

epage · 2024-03-20T16:15:52Z

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 unset rust-version (noted in the tracking issue). 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 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

rustbot · 2024-03-20T16:15:57Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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.

epage · 2024-03-20T16:32:00Z

Err:10 https://ppa.launchpadcontent.net/ubuntu-toolchain-r/test/ubuntu jammy InRelease
  503  Service Unavailable [IP: 185.125.190.80 443]

epage · 2024-03-20T17:14:21Z

Major launchpad outage according to https://status.canonical.com/

epage · 2024-03-22T18:24:55Z

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 cargo add foo (no version req specified), we'd always make a version req out of the latest version.

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 package.rust-version (falling back to rustc -V if unspecified). If we select a non-latest version, we'll warn the user. If we can't select a version, we will error. The user can force the add to work by specifying the version req or passing --ignore-rust-version.

This does not include --update-rust-version. I was planning on implementing that for both the resolver and cargo-add at the same time.

Differences with the resolver

This gives a hard error, rather than falling back to an incompatible version. For an interactive command, I think this workflow is likely better.
This treats dependencies without a package.rust-version as compatible. Being interactive, I think this is likely the best choice. This is also a light two way door and we might want to consider changing the resolver to this behavior. This discrepancy is being tracked in MSRV-dependent dependency version resolution #9930.

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

rfcbot · 2024-03-22T18:24:57Z

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.

weihanglo

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>

src/bin/cargo/commands/add.rs

rfcbot · 2024-03-27T20:20:35Z

🔔 This is now entering its final comment period, as per the review above. 🔔

ehuss · 2024-03-27T21:01:57Z

I'm not quite clear on how this works with the resolver. What happens if:

foo 1.0.0 is compatible with my msrv
foo 1.0.1 is not
run cargo add foo

Does it add foo = "1.0.0"? What happens with Cargo.lock, is it 1.0.0 or 1.0.1?

epage · 2024-03-27T21:20:01Z

This is independent of the resolver. The version requirement will be MSRV-compatible (foo = "1.0.0") while Cargo.lock will be set to 1.0.1.

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 Cargo.toml and will just need to work on their Cargo.lock. They already know what to lock to for the first dependency. They then only need to search the docs for any transitive dependencies. They can also choose to use nightly to resolve as they want (either cargo nightly -Zmsrv-policy add ... or git checkout HEAD -- Cargo.lock && cargo nightly check -Zmsrv-policy).

So while this isn't a complete solution for everyone, until the resolver is stable, it does improvements for everyone.

epage · 2024-03-28T14:29:35Z

@ehuss this does get me thinking that maybe we could update the

Locking 3 packages

message to

Locking 3 packages to latest versions
Locking 3 packages to latest rust-version compatible versions
Locking 3 packages to lowest versions

Depending on which resolve behavior is being invoked.

rfcbot · 2024-04-06T20:24:59Z

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.

weihanglo · 2024-04-08T15:19:03Z

The FCP is complete. Thank you to everyone involved.

@bors r

bors · 2024-04-08T15:19:07Z

📌 Commit 6e4e31b has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-04-08T15:20:14Z

⌛ Testing commit 6e4e31b with merge 3d16d65...

bors · 2024-04-08T15:53:01Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 3d16d65 to master...

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

epage added the T-cargo Team: Cargo label Mar 20, 2024

rustbot assigned weihanglo Mar 20, 2024

rustbot added A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2024

epage mentioned this pull request Mar 20, 2024

MSRV-dependent dependency version resolution #9930

Open

25 tasks

epage added 2 commits March 20, 2024 11:19

feat(cli): Add shell completions for 'cargo add -ignore-rust-version'

80124ad

epage force-pushed the msrv-add branch from 7805845 to 329ee0c Compare March 20, 2024 16:19

rustbot added the A-completions Area: shell completions label Mar 20, 2024

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Mar 22, 2024

weihanglo approved these changes Mar 25, 2024

View reviewed changes

weihanglo reviewed Mar 25, 2024

View reviewed changes

src/bin/cargo/commands/add.rs Show resolved Hide resolved

rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Mar 27, 2024

epage force-pushed the msrv-add branch from 329ee0c to 6e4e31b Compare April 6, 2024 16:05

rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 6, 2024

rfcbot added the to-announce label Apr 6, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2024

bors merged commit 3d16d65 into rust-lang:master Apr 8, 2024
21 checks passed

epage deleted the msrv-add branch April 10, 2024 15:41

weihanglo mentioned this pull request Apr 10, 2024

Update cargo rust-lang/rust#123745

Merged

rustbot added this to the 1.79.0 milestone Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(add): Stabilize MSRV-aware version req selection #13608

feat(add): Stabilize MSRV-aware version req selection #13608

epage commented Mar 20, 2024

rustbot commented Mar 20, 2024

epage commented Mar 20, 2024

epage commented Mar 20, 2024

epage commented Mar 22, 2024 •

edited

Loading

rfcbot commented Mar 22, 2024 •

edited by ehuss

Loading

weihanglo left a comment

rfcbot commented Mar 27, 2024

ehuss commented Mar 27, 2024

epage commented Mar 27, 2024

epage commented Mar 28, 2024

rfcbot commented Apr 6, 2024

weihanglo commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

feat(add): Stabilize MSRV-aware version req selection #13608

feat(add): Stabilize MSRV-aware version req selection #13608

Conversation

epage commented Mar 20, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Mar 20, 2024

epage commented Mar 20, 2024

epage commented Mar 20, 2024

epage commented Mar 22, 2024 • edited Loading

rfcbot commented Mar 22, 2024 • edited by ehuss Loading

weihanglo left a comment

Choose a reason for hiding this comment

rfcbot commented Mar 27, 2024

ehuss commented Mar 27, 2024

epage commented Mar 27, 2024

epage commented Mar 28, 2024

rfcbot commented Apr 6, 2024

weihanglo commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

epage commented Mar 22, 2024 •

edited

Loading

rfcbot commented Mar 22, 2024 •

edited by ehuss

Loading