fix(resolver): De-prioritize no-rust-version in MSRV resolver #13066

epage · 2023-11-28T20:27:27Z

What does this PR try to resolve?

This is a corner case without a good answer.
As such, this change leans on some happy-path entries existing and
preferring those.

How should we test and review this PR?

Additional information

This was originally discussed around the time of #12950 but was held off.

When working on this, I was considering other heuristics like

If a future version has an MSRV, assume that it applies also to the current version
- This can be added in the future
- We likely would want to consider an alternative value, like inferring the rust-version from the manifest or the rust-version used from publish
Sort no-MSRV versions of a package by minimal versions
- The lower the version, the more likely it is to be compatible
- This likely could apply to incompatible MSRVs (or we could reverse-sort those by rust-version) but those will error anyways without --ignore-rust-version, so I decided against these
- I realized this was a backdoor to minimal versions for dependencies without a MSRV and that the community support isn't there for that yet to be a high enough quality of an experience

This is a corner case without a good answer. As such, this change leans on some happy-path entries existing and preferring those.

rustbot · 2023-11-28T20:27:32Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

epage · 2023-11-28T20:33:02Z

r? @Eh2406

src/cargo/core/resolver/version_prefs.rs

Eh2406 · 2023-11-28T20:46:47Z

src/cargo/core/resolver/version_prefs.rs

+                match (a.rust_version(), b.rust_version()) {
+                    // Fallback
+                    (None, None) => {}
+                    (Some(a), Some(b)) if a == b => {}


Can be combined with the previous as (a, b) if a == b =>

The compiler doesn't recognize that None == None and complains about the match being non-exhaustive

Eh2406 · 2023-11-28T21:00:25Z

@bors r+

bors · 2023-11-28T21:00:28Z

📌 Commit 1b97a5c has been approved by Eh2406

It is now in the queue for this repository.

bors · 2023-11-28T21:01:35Z

⌛ Testing commit 1b97a5c with merge 5dfe9bf...

bors · 2023-11-28T21:32:37Z

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 5dfe9bf to master...

Update cargo 25 commits in 26333c732095d207aa05932ce863d850fb309386..58fb23140972092a12f7011d17a7db1d99e3eacf 2023-11-28 20:07:39 +0000 to 2023-12-02 14:15:16 +0000 - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066) r? ghost

Update cargo 27 commits in 26333c732095d207aa05932ce863d850fb309386..623b788496b3e51dc2f9282373cf0f6971a229b5 2023-11-28 20:07:39 +0000 to 2023-12-02 18:10:03 +0000 - docs(book): make old title anchorable (rust-lang/cargo#13102) - Revert "chore(deps): update rust crate openssl to 0.10.60 [security]" (rust-lang/cargo#13101) - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066)

We last tweaked this logic in rust-lang#13066. However, we noticed this was inconsistent with `cargo add` in automatically selecting version requirements. It looks like this is a revert of rust-lang#13066, taking us back to the behavior in rust-lang#12950. In rust-lang#12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely. There are no right answes here, only which wrong answer is ok enough. - Do we treat lack of rust version as `rust-version = "*"` as some people expect or do we try to be smart? - If a user adds or removes `rust-version`, how should that affect the priority? One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with `rust-version = some-value-representing-the-current-toolchain>`.

fix(resolver): Treat unset MSRV as compatible ### What does this PR try to resolve? Have the resolver treat no-MSRV as `rust-version = "*"`, like `cargo add` does for version-requirement selection ### How should we test and review this PR? We last tweaked this logic in #13066. However, we noticed this was inconsistent with `cargo add` in automatically selecting version requirements. It looks like this is a revert of #13066, taking us back to the behavior in #12950. In #12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely. There are no right answes here, only which wrong answer is ok enough. - Do we treat lack of rust version as `rust-version = "*"` as some people expect or do we try to be smart? - If a user adds or removes `rust-version`, how should that affect the priority? One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with `rust-version = some-value-representing-the-current-toolchain>`. See also #9930 (comment) r? `@Eh2406` ### Additional information

epage added 2 commits November 28, 2023 13:57

test(resolver): Add more MSRV test cases

defe789

fix(resolver): De-prioritize no-rust-version in MSRV resolver

1b97a5c

This is a corner case without a good answer. As such, this change leans on some happy-path entries existing and preferring those.

rustbot assigned ehuss Nov 28, 2023

rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2023

rustbot assigned Eh2406 and unassigned ehuss Nov 28, 2023

Eh2406 reviewed Nov 28, 2023

View reviewed changes

src/cargo/core/resolver/version_prefs.rs Outdated Show resolved Hide resolved

Eh2406 reviewed Nov 28, 2023

View reviewed changes

epage force-pushed the msrv branch from 3d68234 to 1b97a5c Compare November 28, 2023 20:52

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 Nov 28, 2023

bors merged commit 5dfe9bf into rust-lang:master Nov 28, 2023
18 checks passed

epage deleted the msrv branch November 28, 2023 21:53

epage mentioned this pull request Nov 30, 2023

MSRV-dependent dependency version resolution #9930

Open

25 tasks

weihanglo mentioned this pull request Dec 2, 2023

Update cargo rust-lang/rust#118541

Merged

ehuss added this to the 1.76.0 milestone Dec 6, 2023

epage mentioned this pull request Apr 23, 2024

fix(resolver): Treat unset MSRV as compatible #13791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolver): De-prioritize no-rust-version in MSRV resolver #13066

fix(resolver): De-prioritize no-rust-version in MSRV resolver #13066

epage commented Nov 28, 2023

rustbot commented Nov 28, 2023

epage commented Nov 28, 2023

Eh2406 Nov 28, 2023

epage Nov 28, 2023

Eh2406 commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

fix(resolver): De-prioritize no-rust-version in MSRV resolver #13066

fix(resolver): De-prioritize no-rust-version in MSRV resolver #13066

Conversation

epage commented Nov 28, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Nov 28, 2023

epage commented Nov 28, 2023

Eh2406 Nov 28, 2023

Choose a reason for hiding this comment

epage Nov 28, 2023

Choose a reason for hiding this comment

Eh2406 commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023