Consider rust-version when selecting packages for cargo add #12078

cassaundra · 2023-05-02T23:56:24Z

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.

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:

warning: selecting older version of `fancy-dep` to satisfy the minimum supported rust version
note: version 0.1.2 of `fancy-dep` has an MSRV of 1.72, which is greater than this package's MSRV of 1.69

Implements #10653.

r? @epage

epage

Thanks for moving this forward!

src/cargo/ops/cargo_add/mod.rs

tests/testsuite/cargo_add/mod.rs

tests/testsuite/cargo_add/rust_version/stderr.log

cassaundra · 2023-05-04T23:43:11Z

Made changes as requested, including the new unstable --ignore-rust-version flag for cargo add. I'll post a few review comments for things that stood out to me in a second...

src/cargo/ops/cargo_add/mod.rs

cassaundra · 2023-05-04T23:46:30Z

src/cargo/ops/cargo_add/mod.rs

@@ -518,8 +531,10 @@ fn get_existing_dependency(
 }

 fn get_latest_dependency(
+    spec: &Package,
    dependency: &Dependency,
    _flag_allow_prerelease: bool,


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?

Yeah, that is a vestige of an old cargo-edit version of cargo-add and can be removed.

src/cargo/ops/cargo_add/mod.rs

epage · 2023-05-05T01:47:27Z

src/bin/cargo/commands/add.rs

+    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"


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

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

fail_if_stable_opt encourages -Zunstable-opt rather than letting us use -Zmsrv-policy, right?

fail_if_stable_opt does encourage -Zunstable-options, but it does also link to an issue that includes information about the unstable flag.

src/bin/cargo/commands/add.rs

tests/testsuite/cargo_add/rust_version_older/stderr.log

cassaundra · 2023-05-15T22:17:35Z

Apologies for the late update, I got pretty sick. In any case, here's what's new:

Don't assume rust-version minor is present (probably against spec, but to ensure compatibility).
Select the latest version with the lowest rust-version, not the first version. Update the rust_version_older test to reflect this.
Provide more information in the error/warning messages.
Move large closure to a rust_version_incompat_error function.
Document the --ignore-rust-version flag.
Gate --ignore-rust-version behind -Zunstable-options instead of -Zmsrv-policy.
Other cleanups.

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.

epage · 2023-05-17T20:59:57Z

src/cargo/ops/cargo_add/mod.rs

+                                // 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)


nit: Can we do req >= (major, minor, patch)?

epage · 2023-05-17T21:03:35Z

src/cargo/ops/cargo_add/mod.rs

+                            "ignoring `{dependency}` {latest_version} to satisfy this package's \
+                             rust-version of {rust_version}",


At minimum, we need to include the rust-version for latest

epage · 2023-05-17T21:07:31Z

tests/testsuite/cargo_add/rust_version_older/mod.rs

+use crate::cargo_add::init_registry;
+use cargo_test_support::curr_dir;
+
+#[cargo_test(nightly, reason = "-Zmsrv-policy is unstable")]


Why are we filtering these tests for nightly?

ah I misunderstood what #[cargo_test(nightly)] was actually for (external nightly features), will fix

epage · 2023-05-17T21:09:46Z

tests/testsuite/cargo_add/rust_version_older/stderr.log

@@ -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)


Huh, just had a thought. We could specify crates as rust-version-user@0.2.1. That is the command-line syntax.

Thoughts?

epage · 2023-05-17T21:13:08Z

src/bin/cargo/commands/add.rs

+    if ignore_rust_version {
+        config
+            .cli_unstable()
+            .fail_if_stable_opt("ignore-rust-version", 0)?;


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 · 2023-05-18T23:01:13Z

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 · 2023-05-23T18:07:25Z

Updated to now use unstable error message as suggested.

epage · 2023-05-23T18:53:17Z

@bors r+

bors · 2023-05-23T18:53:19Z

📌 Commit 8df391b has been approved by epage

It is now in the queue for this repository.

bors · 2023-05-23T18:53:26Z

⌛ Testing commit 8df391b with merge 64fb38c...

bors · 2023-05-23T19:45:17Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 64fb38c to master...

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)

rustbot assigned epage May 2, 2023

rustbot added Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2023

cassaundra changed the title ~~Cargo add rust version~~ Consider rust-version when selecting packages for cargo add May 2, 2023

cassaundra force-pushed the cargo-add-rust-version branch from 6efaaf0 to 7707867 Compare May 3, 2023 00:27

epage reviewed May 3, 2023

View reviewed changes

cassaundra force-pushed the cargo-add-rust-version branch from 7707867 to 4ae83b9 Compare May 4, 2023 23:41

rustbot added the A-cli Area: Command-line interface, option parsing, etc. label May 4, 2023

cassaundra commented May 4, 2023

View reviewed changes

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved