fix(resolve): Improve multi-MSRV workspaces #14569

epage · 2024-09-19T21:14:56Z

What does this PR try to resolve?

We do this by resolving for a package version that is compatible
with the most number of MSRVs within a workspace.

If a version requirement is just right, every package will get the
highest MSRV-compatible dependency.

If its too high, packages will get MSRV-incompatible dependency
versions.
This will happen regardless of what we do due to the nature of
"fallback".

If its too low, packages with higher MSRVs will get older-than-necessary
dependency versions.
This is similar to the "some with and without MSRV" workspaces.
When locking dependencies, we do report to users when newer MSRV/SemVer
compatible dependencies are available to help guide them to upgrading if
desired.

Fixes #14414

How should we test and review this PR?

Is this the right behavior?

This feature is unstable and letting people try it is one way to determine that
A concern was raised within the Cargo team about new users with workspace member MSRVs all set to latest having someone ask them to lower an MSRV and them dealing with staler-than-required dependencies
- At this point, there seems to be agreement on MSRV resolver has surprising behaviour in workspaces with mixed MSRV #14414 being a problem, the resolver magically fixing this is unlikely to happen for the foreseeable future, and this does fix it with the potential for user confusion. From my understanding of those conversations, they are mostly in the area of UX, like with feat(resolve): Direct people to working around less optimal MSRV-resolver results #14543. Rather than blocking on that discussion, this moves forward with the implementation.

Additional information

For workflows like `cargo info`, this call will live on regardless of what we do with the resolver.

We do this by resolving for a package version that is compatible with the most number of MSRVs within a workspace. If a version requirement is just right, every package will get the highest MSRV-compatible dependency. If its too high, packages will get MSRV-incompatible dependency versions. This will happen regardless of what we do due to the nature of `"fallback"`. If its too low, packages with higher MSRVs will get older-than-necessary dependency versions. This is similar to the "some with and without MSRV" workspaces. When locking dependencies, we do report to users when newer MSRV/SemVer compatible dependencies are available to help guide them to upgrading if desired. Fixes rust-lang#14414

rustbot · 2024-09-19T21:15:02Z

r? @ehuss

rustbot has assigned @ehuss.
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

weihanglo · 2024-09-23T20:22:31Z

src/cargo/core/resolver/version_prefs.rs

Implementation-wise looks good. I'd like to hear back from @VorpalBlade for their opinion on this new behavior.

It seems like a step in the right direction, as I understand it, the majority wins, which would solve my issue.

(However, if you go ahead and open a pr in the middle of the night, and then merge it before morning there is little timely feedback I can give. Given timezones, work hours etc it is not reasonable to expect feedback in less than 24 hours.)

Yes, that was little time for feedback before merging but they noted elsewhere that your feedback didn't have to be blocking for merge as this is unstable. Its still important for stabilization!

weihanglo · 2024-09-23T20:22:44Z

src/cargo/core/resolver/version_prefs.rs

@@ -59,7 +59,7 @@ impl VersionPreferences {
    ///
    /// Sort order:
    /// 1. Preferred packages
-    /// 2. [`VersionPreferences::max_rust_version`]
+    /// 2. Most compatible [`VersionPreferences::rust_versions`]


The unstable doc might need an update according, for the algorithm of determining workspace MSRV. However, from a user perspective, it is a bit hard to understand. I am not sure if we want to document it.

It is also hard to determine from a human eye, if it is a large workspace and people are adding/removing members a lot that changes the workspace-wide MSRV.

I've updated the text. The unstable docs offer us an opportunity to evaluate the wording. Even once its stabilized, we can iterate as we see how people are dealing with it.

weihanglo

Going to merge this as this unstable feature is still in the phase of collecting feedback. Thank you for fixing this :)

weihanglo · 2024-09-24T02:16:48Z

@bors r+

bors · 2024-09-24T02:16:51Z

📌 Commit 94db932 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-09-24T02:17:57Z

⌛ Testing commit 94db932 with merge 5e44da4...

bors · 2024-09-24T02:46:19Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 5e44da4 to master...

Update cargo 19 commits in eaee77dc1584be45949b75e4c4c9a841605e3a4b..80d82ca22abbee5fb7b51fa1abeb1ae34e99e88a 2024-09-19 21:10:23 +0000 to 2024-09-27 17:56:01 +0000 - Update cc to 1.1.22 (rust-lang/cargo#14607) - feat: lockfile path implies --locked on cargo install (rust-lang/cargo#14556) - feat(toml): Add `autolib` (rust-lang/cargo#14591) - fix: correct error count for `cargo check --message-format json` (rust-lang/cargo#14598) - test: relax panic output assertion (rust-lang/cargo#14602) - feat(timings): support dark color scheme in HTML output (rust-lang/cargo#14588) - feat: add CARGO_MANIFEST_PATH env variable (rust-lang/cargo#14404) - fix(config): Don't double-warn about `$CARGO_HOME/config` (rust-lang/cargo#14579) - fix(cargo-rustc): give trailing flags higher precedence on nightly (rust-lang/cargo#14587) - feat: make lockfile v4 the default (rust-lang/cargo#14595) - perf: Improve quality of completion performance traces (rust-lang/cargo#14592) - test: Remove completion tests (rust-lang/cargo#14590) - feat: Add support for completing `cargo update <TAB>` (rust-lang/cargo#14552) - test: Migrate remaining with_stdout/with_stderr calls (rust-lang/cargo#14577) - fix(resolve): Improve multi-MSRV workspaces (rust-lang/cargo#14569) - chore: Bump MSRV to 1.81 (rust-lang/cargo#14585) - Add a `--dry-run` flag to the `install` command (rust-lang/cargo#14280) - fix(resolve): Don't list transitive, incompatible dependencies as available (rust-lang/cargo#14568) - feat(complete): Upgrade clap_complete (rust-lang/cargo#14573)

epage added 7 commits September 19, 2024 13:19

refactor(ws): Clarify what rust-version is used

5f07992

For workflows like `cargo info`, this call will live on regardless of what we do with the resolver.

test(resolver): Have unit test for multi-msrv

698f26b

test(msrv): Clarify version numbers

6b79647

test(msrv): Clarify role of deps

3a47885

test(msrv): Clarify the differences in deps

892ad41

test(msrv): Show msrv resolver issue

64abeb2

rustbot assigned ehuss Sep 19, 2024

epage force-pushed the msrv-max branch 2 times, most recently from 6fac2ab to 7d8007c Compare September 22, 2024 01:25

weihanglo reviewed Sep 23, 2024

View reviewed changes

epage force-pushed the msrv-max branch from 7d8007c to 7d32467 Compare September 24, 2024 00:51

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Sep 24, 2024

epage force-pushed the msrv-max branch from 7d32467 to 94db932 Compare September 24, 2024 01:43

weihanglo approved these changes Sep 24, 2024

View reviewed changes

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 Sep 24, 2024

bors merged commit 5e44da4 into rust-lang:master Sep 24, 2024
22 checks passed

bors mentioned this pull request Sep 24, 2024

fix(update): don't reuse a previously locked dependency if multiple major versions are compatible #14582

Open

epage deleted the msrv-max branch September 24, 2024 13:54

weihanglo mentioned this pull request Sep 27, 2024

Update cargo rust-lang/rust#130929

Merged

rustbot added this to the 1.83.0 milestone Sep 28, 2024

epage mentioned this pull request Oct 3, 2024

MSRV-dependent dependency version resolution #9930

Open

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolve): Improve multi-MSRV workspaces #14569

fix(resolve): Improve multi-MSRV workspaces #14569

epage commented Sep 19, 2024

rustbot commented Sep 19, 2024

weihanglo Sep 23, 2024

VorpalBlade Sep 24, 2024

epage Sep 24, 2024

weihanglo Sep 23, 2024

epage Sep 24, 2024

weihanglo left a comment

weihanglo commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 24, 2024

fix(resolve): Improve multi-MSRV workspaces #14569

fix(resolve): Improve multi-MSRV workspaces #14569

Conversation

epage commented Sep 19, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Sep 19, 2024

weihanglo Sep 23, 2024

Choose a reason for hiding this comment

VorpalBlade Sep 24, 2024

Choose a reason for hiding this comment

epage Sep 24, 2024

Choose a reason for hiding this comment

weihanglo Sep 23, 2024

Choose a reason for hiding this comment

epage Sep 24, 2024

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 24, 2024