feat(resolver): Very preliminary MSRV resolver support #12560

epage · 2023-08-25T19:24:00Z

What does this PR try to resolve?

A bare bones implementation of an MSRV resolver that is good enough for people running on nightly when they really need it but is not ready for general use.

Current limitations

Does not honor --ignore-version
Gives terrible error messages
Nothing is done yet regarding cargo install
Doesn't inform the user when choosing non-latest

These will be noted in #9930 on merge.

Implementation wise, this is yet another hack (sorry @Eh2406). Our expectation to get this GA is to refactor the resolver to make the cargo/resolver boundary look a little more like the cargo/pubgrub boundary so we can better control policy without any of these hacks which will also make having all of the policy we need for this easier to maintain.

This is a part of #9930

How should we test and review this PR?

Per commit

This was kept separate to show that the prior commit didn't change anything for stable users.

rustbot · 2023-08-25T19:24:04Z

r? @ehuss

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

Eh2406 · 2023-08-25T20:43:11Z

🙈

epage · 2023-08-26T01:19:02Z

src/cargo/core/compiler/standard_lib.rs

@@ -153,6 +154,7 @@ pub fn resolve_std<'cfg>(
        &specs,
        HasDevUnits::No,
        crate::core::resolver::features::ForceAllTargets::No,
+        max_rust_version,


This is one case I'm not entirely sure of but since its another unstable feature, I figure doing a best effort isn't the end of the world

I did not follow the point you were trying to make here.

I'm assuming this is build-std and we do a separate resolve pass for that. What should we consider max_rust_version to be in this case?

epage · 2023-08-26T01:20:04Z

src/cargo/core/resolver/mod.rs

+    if !config
+        .map(|c| c.cli_unstable().msrv_policy)
+        .unwrap_or(false)
+    {
+        max_rust_version = None;
+    }


I centralized the feature check to this one spot

I am somewhat tempted to do something similar for honor_rust_version and have the config initialized with it and just check the config here.

epage · 2023-08-26T01:21:01Z

tests/testsuite/cargo_add/rust_version_ignore/mod.rs

-        .success()
+        .code(101)


This will be undone when we get --ignore-rust-version working

epage · 2023-08-26T01:21:23Z

tests/testsuite/rust_version.rs

+        // This should have a better error message
+        .with_stderr(
+            "\
+[UPDATING] `dummy-registry` index
+[ERROR] failed to select a version for the requirement `bar = \"^1.0.0\"`
+candidate versions found which didn't match: 1.6.0
+location searched: `dummy-registry` index (which is replacing registry `crates-io`)
+required by package `foo v0.0.1 ([CWD])`
+perhaps a crate was updated and forgotten to be re-vendored?
+",
+        )


Example of the bad error message

epage · 2023-08-26T01:21:46Z

tests/testsuite/rust_version.rs

+    p.cargo("check --ignore-rust-version")
+        .arg("-Zmsrv-policy")
+        .masquerade_as_nightly_cargo(&["msrv-policy"])
+        // This should pick 1.6.0
+        .with_stderr(
+            "\
+[UPDATING] `dummy-registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v1.5.0 (registry `dummy-registry`)
+[CHECKING] bar v1.5.0


Another example of --ignore-rust-version not working yet

epage · 2023-08-26T02:01:51Z

🙈

r? @Eh2406

;)

Eh2406 · 2023-08-28T16:30:56Z

There was discussion about modeling this as a dependency so that the resolver did not explicitly need to know about this new kind of requirement. What happened with that idea?

With this being part of the resolver is it possible to or is there value in adding this to proptest test generation?

epage · 2023-08-28T16:43:36Z

There was discussion about modeling this as a dependency so that the resolver did not explicitly need to know about this new kind of requirement. What happened with that idea?

This is exclusively meant as an unstable hack to

Unblock people from getting this feature even if its not to the quality of stable
Allow us to iterate on other parts of the design while working on the resolver refactor

The resolver factor to get us to the state of this being a dependency is doing to be a bit of work and I figured this can allow us to decouple parts of the process for this feature.

With this being part of the resolver is it possible to or is there value in adding this to proptest test generation?

As this implementation is only ever intended for unstable use, I would hold off but we can call this out in the issue as remaining work in case anything changes about the plan to ensure we have it before stabilization.

Eh2406 · 2023-08-28T19:13:09Z

As a reviewer I don't quite know what to say then. I can confirm that it is an unholy hack. But, doing things properly is not in scope for this PR. The way it's implemented it should not corrupt internal data structures or cause other similar problems. What other feedback would you find useful from your reviewer?

Is there other work you would like to do before this hack gets Merged?

epage · 2023-08-28T19:29:22Z

I am fine with it being merged as-is but with this touching the resolver, I wanted you to both review the work and get your Ok on doing this unholy hack.

Eh2406 · 2023-08-28T20:48:53Z

@bors r=🙈eh2406🙈

bors · 2023-08-28T20:48:55Z

📌 Commit 4abd05e has been approved by 🙈eh2406🙈

It is now in the queue for this repository.

bors · 2023-08-28T21:11:24Z

⌛ Testing commit 4abd05e with merge 0c2b370...

bors · 2023-08-28T21:55:03Z

☀️ Test successful - checks-actions
Approved by: 🙈eh2406🙈
Pushing 0c2b370 to master...

Eh2406 · 2023-08-29T14:45:56Z

Having slept on this I think I was too harsh. I have done worse things this code base, some of which I have not succeeded at removing. It is a hack, but "unholy" was unfair of me.

Update cargo 18 commits in 925280f028db3a322935e040719a0754703947cf..96fe1c9e1aecd8f57063e3753969bb6418fd2fd5 2023-08-25 21:16:44 +0000 to 2023-08-29 20:10:34 +0000 - fix(lints): Fail when overriding inherited lints (rust-lang/cargo#12584) - cargo install: suggest --git when package name is url (rust-lang/cargo#12575) - chore: remove unstable-options for logout (rust-lang/cargo#12588) - Improve logout message for asymmetric tokens (rust-lang/cargo#12587) - fix(update): Remove references to -p in help (rust-lang/cargo#12586) - fix(update): Make `-p` more convenient by being positional (rust-lang/cargo#12545) - Set tracing target for networking messages. (rust-lang/cargo#12582) - Retry docs (rust-lang/cargo#12583) - feat(resolver): **Very** preliminary MSRV resolver support (rust-lang/cargo#12560) - Update git2 (rust-lang/cargo#12580) - Explain how `version` works for `git` dependencies (rust-lang/cargo#12270) - Improve deserialization errors of untagged enums (rust-lang/cargo#12574) - Add support for `target.'cfg(..)'.linker` (rust-lang/cargo#12535) - Improve resolver version mismatch warning (rust-lang/cargo#12573) - Stabilize `--keep-going` (rust-lang/cargo#12568) - Define {{command}} for use in src/doc/man/includes (rust-lang/cargo#12570) - Update serde (rust-lang/cargo#12569) - chore: add missing `windows-sys` features back (rust-lang/cargo#12563) r? ghost

MSRV-aware resolver has been implemented in rust-lang#12560. Based on that effort, generating lockfile can now respect `rust-version` so that a package with an old MSRV never generate a incompatiable lockfile, even using the latest Cargo.

epage added 3 commits August 25, 2023 09:59

test(resolver): Demonstrate existing behavior

10196aa

feat(resolver): Respect MSRV

4820624

test(resolver): Enable msrv-policy

4abd05e

This was kept separate to show that the prior commit didn't change anything for stable users.

rustbot assigned ehuss Aug 25, 2023

epage force-pushed the msrv branch from ff0f1c8 to 4abd05e Compare August 26, 2023 01:13

rustbot added the A-testing-cargo-itself Area: cargo's tests label Aug 26, 2023

epage commented Aug 26, 2023

View reviewed changes

epage marked this pull request as ready for review August 26, 2023 02:01

rustbot assigned Eh2406 and unassigned ehuss Aug 26, 2023

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

bors merged commit 0c2b370 into rust-lang:master Aug 28, 2023
19 checks passed

epage deleted the msrv branch August 28, 2023 22:54

epage mentioned this pull request Aug 29, 2023

MSRV-dependent dependency version resolution #9930

Open

25 tasks

weihanglo mentioned this pull request Aug 29, 2023

Update cargo rust-lang/rust#115365

Merged

epage mentioned this pull request Sep 18, 2023

Tag 0.3? eminence/terminal-size#59

Closed

ehuss added this to the 1.74.0 milestone Sep 30, 2023

epage mentioned this pull request Oct 19, 2023

feat: stabilize lockfile v4 #12852

Merged

weihanglo mentioned this pull request Oct 20, 2023

feat: respect rust-version when generating lockfile #12861

Merged

polarathene mentioned this pull request Oct 27, 2023

Command failing on crate json5 taiki-e/cargo-minimal-versions#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(resolver): Very preliminary MSRV resolver support #12560

feat(resolver): Very preliminary MSRV resolver support #12560

epage commented Aug 25, 2023 •

edited

Loading

rustbot commented Aug 25, 2023

Eh2406 commented Aug 25, 2023

epage Aug 26, 2023

Eh2406 Aug 28, 2023

epage Aug 28, 2023

epage Aug 26, 2023

epage Aug 26, 2023

epage Aug 26, 2023

epage Aug 26, 2023

epage commented Aug 26, 2023

Eh2406 commented Aug 28, 2023

epage commented Aug 28, 2023

Eh2406 commented Aug 28, 2023

epage commented Aug 28, 2023

Eh2406 commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

Eh2406 commented Aug 29, 2023

feat(resolver): **Very** preliminary MSRV resolver support #12560

feat(resolver): **Very** preliminary MSRV resolver support #12560

Conversation

epage commented Aug 25, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Aug 25, 2023

Eh2406 commented Aug 25, 2023

epage Aug 26, 2023

Choose a reason for hiding this comment

Eh2406 Aug 28, 2023

Choose a reason for hiding this comment

epage Aug 28, 2023

Choose a reason for hiding this comment

epage Aug 26, 2023

Choose a reason for hiding this comment

epage Aug 26, 2023

Choose a reason for hiding this comment

epage Aug 26, 2023

Choose a reason for hiding this comment

epage Aug 26, 2023

Choose a reason for hiding this comment

epage commented Aug 26, 2023

Eh2406 commented Aug 28, 2023

epage commented Aug 28, 2023

Eh2406 commented Aug 28, 2023

epage commented Aug 28, 2023

Eh2406 commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

Eh2406 commented Aug 29, 2023

feat(resolver): Very preliminary MSRV resolver support #12560

feat(resolver): Very preliminary MSRV resolver support #12560

epage commented Aug 25, 2023 •

edited

Loading