Consider rust-version when selecting packages for cargo add #12078
Conversation
rustbot
added
Command-add
S-waiting-on-review
cassaundra
changed the title
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
6efaaf0
to
7707867
Compare
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
7707867
to
4ae83b9
Compare
|
Made changes as requested, including the new unstable |
| @@ -518,8 +531,10 @@ fn get_existing_dependency( | |||
| } | |||
|
|
|||
| fn get_latest_dependency( | |||
| spec: &Package, | |||
| dependency: &Dependency, | |||
| _flag_allow_prerelease: bool, | |||
There was a problem hiding this comment.
side note: no clue what this _flag_allow_prerelease is. doesn't appear to ever have been used in cargo. I'm guessing a holdover from cargo-edit?
There was a problem hiding this comment.
Yeah, that is a vestige of an old cargo-edit version of cargo-add and can be removed.
src/bin/cargo/commands/add.rs
Outdated
| if !config.cli_unstable().msrv_policy && !honor_rust_version { | ||
| return Err(CliError::new( | ||
| anyhow::format_err!( | ||
| "the `ignore-rust-version` flag is unstable, pass -Z msrv-policy to enable it" |
There was a problem hiding this comment.
nit: I'd suggest something more like
"`--ignore-rust-version` is unstable; pass `-Zmsrv-policy` to enable support for it"or look to https://docs.rs/cargo/latest/src/cargo/core/features.rs.html#1034-1057 to see what other unstable errors are like
There was a problem hiding this comment.
fail_if_stable_opt seems more suitable. Should we open a tracking issue separate from the #12043 PR so that it can be referenced in the message?
edit: same goes for the flag documentation as well
There was a problem hiding this comment.
fail_if_stable_opt encourages -Zunstable-opt rather than letting us use -Zmsrv-policy, right?
There was a problem hiding this comment.
fail_if_stable_opt does encourage -Zunstable-options, but it does also link to an issue that includes information about the unstable flag.
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
4ae83b9
to
00f2658
Compare
rustbot
added
A-cli-help
|
Apologies for the late update, I got pretty sick. In any case, here's what's new:
Note: |
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
00f2658
to
cbab596
Compare
src/cargo/ops/cargo_add/mod.rs
Outdated
| // Determine whether or not the rust-versions are compatible by | ||
| // comparing the lowest possible versions each of them could | ||
| // represent. | ||
| (req.0, req.1, req.2) >= (major, minor, patch) |
There was a problem hiding this comment.
nit: Can we do req >= (major, minor, patch)?
src/cargo/ops/cargo_add/mod.rs
Outdated
| "ignoring `{dependency}` {latest_version} to satisfy this package's \ | ||
| rust-version of {rust_version}", |
There was a problem hiding this comment.
At minimum, we need to include the rust-version for latest
| use crate::cargo_add::init_registry; | ||
| use cargo_test_support::curr_dir; | ||
|
|
||
| #[cargo_test(nightly, reason = "-Zmsrv-policy is unstable")] |
There was a problem hiding this comment.
Why are we filtering these tests for nightly?
There was a problem hiding this comment.
ah I misunderstood what #[cargo_test(nightly)] was actually for (external nightly features), will fix
| @@ -0,0 +1,3 @@ | |||
| Updating `dummy-registry` index | |||
| warning: ignoring `rust-version-user` 0.2.1 to satisfy this package's rust-version of 1.70 (use `--ignore-rust-version` to override) | |||
There was a problem hiding this comment.
Huh, just had a thought. We could specify crates as rust-version-user@0.2.1. That is the command-line syntax.
Thoughts?
src/bin/cargo/commands/add.rs
Outdated
| if ignore_rust_version { | ||
| config | ||
| .cli_unstable() | ||
| .fail_if_stable_opt("ignore-rust-version", 0)?; |
There was a problem hiding this comment.
Note: fail_if_stable_opt currently has its tracking issue set to 0. We can either pick an issue for that, or just write our own error message that does not include a link to an issue. Of course, don't merge until this has been resolved.
One option is that this PR doesn't close out #10653 but we keep it open to track to stabilization, so it acts as its own tracking issue.
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
cbab596
to
53797b6
Compare
|
Fixed some of the issues raised (sans tracking issue), and cleaned things up a bit. |
When `-Zmsrv-policy` is enabled, try to select dependencies which satisfy the target package's `rust-version` field (if present). If the selected version is not also the latest, emit a warning to the user about this discrepancy. Dependency versions without a `rust-version` are considered compatible by default. Implements rust-lang#10653.
cassaundra
force-pushed
the
cargo-add-rust-version
branch
from
53797b6
to
8df391b
Compare
|
Updated to now use unstable error message as suggested. |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 10 commits in 09276c703a473ab33daaeb94917232e80eefd628..64fb38c97ac4d3a327fc9032c862dd28c8833b17 2023-05-16 21:43:35 +0000 to 2023-05-23 18:53:23 +0000 - Consider rust-version when selecting packages for cargo add (rust-lang/cargo#12078) - fix(lints): Switch to -Zlints so stable projects can experiment (rust-lang/cargo#12168) - Automatically inherit workspace fields when running cargo new/init (rust-lang/cargo#12069) - ci: check if any version bump needed for member crates (rust-lang/cargo#12126) - feat: `lints` feature (rust-lang/cargo#12148) - fix: pass `-C debuginfo` after weakening if explicitly set (rust-lang/cargo#12165) - Tweak build help to clarify role of --bin (rust-lang/cargo#12157) - fix: Pass CI on nightly (rust-lang/cargo#12160) - docs(source): doc comments for Source and its impls (rust-lang/cargo#12159) - docs(source): doc comments for `Source` and friends (rust-lang/cargo#12153) r? `@ghost`
Update cargo 10 commits in 09276c703a473ab33daaeb94917232e80eefd628..64fb38c97ac4d3a327fc9032c862dd28c8833b17 2023-05-16 21:43:35 +0000 to 2023-05-23 18:53:23 +0000 - Consider rust-version when selecting packages for cargo add (rust-lang/cargo#12078) - fix(lints): Switch to -Zlints so stable projects can experiment (rust-lang/cargo#12168) - Automatically inherit workspace fields when running cargo new/init (rust-lang/cargo#12069) - ci: check if any version bump needed for member crates (rust-lang/cargo#12126) - feat: `lints` feature (rust-lang/cargo#12148) - fix: pass `-C debuginfo` after weakening if explicitly set (rust-lang/cargo#12165) - Tweak build help to clarify role of --bin (rust-lang/cargo#12157) - fix: Pass CI on nightly (rust-lang/cargo#12160) - docs(source): doc comments for Source and its impls (rust-lang/cargo#12159) - docs(source): doc comments for `Source` and friends (rust-lang/cargo#12153) r? `@ghost`
refactor: Pull out cargo-add MSRV code for reuse ### What does this PR try to resolve? #12078 added MSRV code in `cargo add`. Our assumption when writing it is that we'd need to generalize the code before reusing it in other places, like `cargo install`. This PR focused purely on that refactor because I'm hopeful it will be useful for other work I'm doing. Despite not having a user for this yet, I think the `cargo install` case is inevitable and I feel this does a bit to clean up MSRV related code by using a more specific type everywhere. ### How should we test and review this PR? Each commit gradually progresses things along
Remove useless parameters ### What does this PR try to resolve? From #12078 (comment)
When
-Zmsrv-policyis enabled, try to select dependencies which satisfy the target package'srust-versionfield (if present). If the selected version is not also the latest, emit a warning to the user about this discrepancy.Dependency versions without a
rust-versionare considered compatible by default.One remaining question is whether we should go into more detail when explaining the discrepancy and ways to resolve it to the user. For example:
Implements #10653.
r? @epage