feat(resolver): `-Zdirect-minimal-versions` #11688

epage · 2023-02-07T22:44:06Z

This is an alternative to -Zminimal-versions as discussed in #5657.

Problems with -Zminimal-versions includes

Requires the root most dependencies to verify it and we then percolate that up the stack. This requires a massive level of cooperation to accomplish and so far there have been mixed results with it to the point that cargo's unstable
documentation discourages its use.
Users expect cargo check -Zminimal-versions to force resolving to minimal but it doesn't as the default maximal resolve is compatible and requires cargo update -Zminimal-versions
Different compatible versions might be selected, breaking interop between crates, changing feature unification, and breaking -sys crates without bad links

-Zdirect-minimal-versions instead only applies this rule to your
direct dependencies, allowing anyone in the stack to immediately adopt
it, independent of everyone else.

Special notes

Living up to the name and the existing design, this ignores yanked
crates. This makes sense for ^1.1 version requirements but might
look weird for ^1.1.1 version requirements as it could select
1.1.2.
This will error if an indirect dependency requires a newer version.
Your version requirement will need to capture what you use and all
of you dependencies. An alternative design would have tried to merge
the result of minimum versions for direct dependencies and maximum
versions for indirect dependencies. This would have been complex and
led to weird corner cases, making it harder to predict. I also suspect
the value gained would be relatively low as you can't verify that
version requirement in any other way.
- This also means discrepancies between dependencies and dev-dependencies are errors
- The error could be improved to call out that this was from minimal
  versions but I felt getting this out now and starting to collect
  feedback was more important.

One advantage of this approach over -Zminimal-versions is that it removes most of the problems that cargo-minimal-versions tried to workaround.

As for the implementation, this might not be the most elegant solution but it works and we can always iterate and improve on it in the future.

We keep the state as a bool throughout but compensate for that by explicitly creating a variable to abstract away constants
The name changes depending on the context, from direct_minimal_version when dealing with the unstable flag to first_minimal_version when the concept of "direct" is lost to first_version when we split off the ordering concept into a separate variable
Packages that respect direct_minimal_versions are determined by whether they are the top-level summaries that get past into resolve

What does this PR try to resolve?

The primary use case is verifying version requirements to avoid depending on something newer than might be available in a dependent

For this to help the MSRV use case, the crate author must directly depend on all indirect dependencies where their latest release has too new of an MSRV but at least they can do so with the ^ operator, rather than < and breaking the ecosystem.

How should we test and review this PR?

The first two commits add tests using -Zminimal-versions. The commit that adds -Zdirect-minimal-versions updates the tests, highlighting the differences in behavior.

Additional information

Potential areas of conversation for stablization

Flag name
Handling of yanked (pick first non-yanked, pick yanked, error)
Quality of error message
Should the package have a "memory" of this flag being set by writing it to the lockfile?

Potential future work

Stablize this
Remove -Zminimal-versions
Update cargo publishs --verify step to use this.
- The challenge is this won't be using the packaged Cargo.lock which probably should also be verified.

rustbot · 2023-02-07T22:44:11Z

r? @weihanglo

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

epage · 2023-02-07T22:49:09Z

CI will be blocked until rust-lang/rust#107688 lands in nightly.

#11679 (comment)

Should we decouple nightly from the other test jobs so we can at least see the rest of CI status or maybe even allow merging regardless of nightly failures?

Eh2406 · 2023-02-08T21:30:12Z

So if I understand the functionality right you are passing around a flag that means "for this dependency only provide 1 candidate". Then we make sure to pass that as "true" for direct dependencies and "false" for indirect dependencies. Did I understand that correctly?

On a different topic, thank you for adding tests! If you've already written/added the tests for -Zminimal-versions then let's keep them as long as we have that flag.

Similarly there are some tests for -Zminimal-versions in crates/resolver-tests a unit test and a proptest. I would love to see this new functionality added to the proptests. We should think about what properties the new functionality has.

epage · 2023-02-08T21:36:42Z

So if I understand the functionality right you are passing around a flag that means "for this dependency only provide 1 candidate". Then we make sure to pass that as "true" for direct dependencies and "false" for indirect dependencies. Did I understand that correctly?

Yes, that is correct. It effectively gives us a = semver operator without the complexity of rewriting a version requirement for this.

Eh2406 · 2023-02-09T18:56:41Z

One of the resolver's chronic problems is having a large number of knobs and state that need to be kept synchronized, with little in the code to teach the connections.
I wonder if there is some structure we could put this bool in help make it clear when it needs to be set and what the implications are. (Of course it would be hypocritical of me to block this PR on organizing the code better than I managed.)

Naively it sounds like "this dependency requirement should only ever give one candidate" should live in a Dependency. Can you try that?

epage · 2023-02-09T20:58:14Z

One of the resolver's chronic problems is having a large number of knobs and state that need to be kept synchronized, with little in the code to teach the connections. I wonder if there is some structure we could put this bool in help make it clear when it needs to be set and what the implications are. (Of course it would be hypocritical of me to block this PR on organizing the code better than I managed.)

Naively it sounds like "this dependency requirement should only ever give one candidate" should live in a Dependency. Can you try that?

I lean towards gathering more information before generalizing this. We have -Zminimal-versions and now -Zdirect-minimal-versions with how they customize the resolver. We will hopefully soon have "my system depends on a single rustc version" for the cargo install MSRV experience and "build with the lowest common denominator"' with the "xargo check MSRV experience. I feel like seeing how each of these systems interact would help provide insight into what would work well for generalizing this while now it feels a little speculative. I could see prototyping the MSRV behavior to lean what is needed but cleaning this up before merging it.

Thoughts?

Eh2406 · 2023-02-10T19:54:14Z

I agree that we are in a prototyping phase. I think the semantics of this new flag are reasonable as a next thing to try. All I'm wondering about is where is the best place to shoehorn in this extra bool. This PR currently passes it around as an argument, and it doesn't look too bad. I would like to see some experimentation adding it to Dependency. Dependency::Inner gets a match_only_one flag. Dependency gets a with_match_only_one(&self, bool) -> Self and a is_match_only_one(&self) -> bool.

bors · 2023-02-14T11:07:37Z

☔ The latest upstream changes (presumably #11646) made this pull request unmergeable. Please resolve the merge conflicts.

This is an alternative to `-Zminimal-versions` as discussed in rust-lang#5657. The problem with `-Zminimal-versions` is it requires the root most dependencies to verify it and we then percolate that up the stack. This requires a massive level of cooperation to accomplish and so far there have been mixed results with it to the point that cargo's unstable documentation discourages its use. `-Zdirect-minimal-versions` instead only applies this rule to your direct dependencies, allowing anyone in the stack to immediately adopt it, independent of everyone else. Special notes - Living up to the name and the existing design, this ignores yanked crates. This makes sense for `^1.1` version requirements but might look weird for `^1.1.1` version requirements as it could select `1.1.2`. - This will error if an indirect dependency requires a newer version. Your version requirement will need to capture what you use **and** all of you dependencies. An alternative design would have tried to merge the result of minimum versions for direct dependencies and maximum versions for indirect dependencies. This would have been complex and led to weird corner cases, making it harder to predict. I also suspect the value gained would be relatively low as you can't verify that version requirement in any other way. - The error could be improved to call out that this was from minimal versions but I felt getting this out now and starting to collect feedback was more important.

epage · 2023-02-14T22:05:10Z

@Eh2406 as discussed, I've kept the current approach as its more surgical (Dependencies has a lot more impact) and the proptest cases (I think I got them all)

Eh2406

This looks good as a first PR for the feature.

I'd like to register two things that need to be dealt with before stabilization:

When we know we've got the semantics correct, we must experiment with other abstractions. Only having tried alternatives would I be willing to stabilize passing around this argument.
We need better error messages. Specifically versions that meet the requirements ^1.1 are: 1.1.0 needs to explain why 1.2.0 does not match. Of course, not a blocker on initial experimentation.

crates/resolver-tests/tests/resolve.rs

src/cargo/core/resolver/dep_cache.rs

Eh2406 · 2023-02-15T22:14:22Z

src/cargo/core/resolver/dep_cache.rs

+        &mut self,
+        dep: &Dependency,
+        first_minimal_version: bool,
+    ) -> Poll<CargoResult<Rc<Vec<Summary>>>> {
        if let Some(out) = self.registry_cache.get(dep).cloned() {


Trying to think whether we need to add first_minimal_version to the cache key. I think we do, as it's perfectly reasonable to have a transitive and a non-transitive Dependency but where this query returns different results.

But either adding it to the cash key or adding a comment explaining why it's not needed would be appreciated.

I went with your other comment about parent.is_none() though I hesitate about that because that is knowledge outside of the resolver.

There are 2 caches here. self.registry_cache has parent in is cache key. Thank you for adding a comment.

But self.registry_cache appears to me not to keep track of the fact that the result of query may differ depending on the value of first_minimal_version. If I've missed something please add a comment.

self.registry_cache is now tracking the first_minimal_version

src/cargo/core/resolver/mod.rs

weihanglo · 2023-02-20T16:44:20Z

Hand this over to Jacob
r? @Eh2406

Eh2406 · 2023-02-27T16:27:45Z

On a larger note... We have many levels of caching going on in the resolver. It is not clear to me that they still all pull their weight. When each one was added it was a significant performance when. But now resolver speed is not the significant bottleneck it used to be so absolute performance is less critical. Especially as I now consider the cost of code complexity as higher than I used to. But in order to decide we would need an objective and repeatable measure of the performance cost. The resolver needs an isolated benchmark.

cc https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Benchmarking.20without.20lockfile

Eh2406 · 2023-03-01T19:14:04Z

@bors r+

bors · 2023-03-01T19:14:06Z

📌 Commit 1d153f1 has been approved by Eh2406

It is now in the queue for this repository.

bors · 2023-03-01T19:27:02Z

⌛ Testing commit 1d153f1 with merge 80f1a5d...

bors · 2023-03-01T20:09:28Z

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 80f1a5d to master...

23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d 2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000 - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) Note that some 3rd-party licensing allowed list changed due to the introducion of `gix` dependency

Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`

My understanding is that it's okay that we don't use these minimal versions directly in our normal/committed Cargo.lock file, because we may want to pick up more recent compatible versions that have addressed security issues or improved performance, etc. That said, we still want to periodically test the minimal versions to ensure that our own project still compiles and runs as expected. This will help to increase compatibility across the whole Rust ecosystem when there are shared dependencies and someone is using our library. I've also abstracted out the CUE _testRust steps here, so they can be shared. See: - rust-lang/cargo#11688 - https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277 - https://twitter.com/jonhoo/status/1571290371124260865 - cue-lang/cue#860

My understanding is that it's okay that we don't use these minimal versions directly in our normal/committed Cargo.lock file, because we may want to pick up more recent compatible versions that have addressed security issues or improved performance, etc. That said, we still want to periodically test the minimal versions to ensure that our own project still compiles and runs as expected. This will help to increase compatibility across the whole Rust ecosystem when there are shared dependencies and someone is using our library. I've also abstracted out the CUE _testRust steps here, so they can be shared. See: - rust-lang/cargo#11688 - https://doc.rust-lang.org/beta/cargo/reference/unstable.html#minimal-versions - https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277 - https://twitter.com/jonhoo/status/1571290371124260865 - cue-lang/cue#860

rustbot assigned weihanglo Feb 7, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2023

taiki-e mentioned this pull request Feb 7, 2023

Support -Zdirect-minimal-versions taiki-e/cargo-minimal-versions#16

Closed

epage force-pushed the minimal branch from ad61a32 to fc0fcc9 Compare February 8, 2023 12:58

epage force-pushed the minimal branch from fc0fcc9 to dc1567b Compare February 9, 2023 12:31

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-unstable Area: nightly unstable support labels Feb 9, 2023

epage added 3 commits February 14, 2023 15:43

test(resolver): Create baseline for `-Zdirect-minimal-versions

12000a9

test(resolver): Additional direct minimal version tests

c07f067

epage force-pushed the minimal branch from dc1567b to 81223fc Compare February 14, 2023 22:03

vilgotf mentioned this pull request Feb 15, 2023

ci: speedup & reduce cache storage twilight-rs/twilight#2145

Merged

Eh2406 approved these changes Feb 15, 2023

View reviewed changes

rustbot assigned Eh2406 and unassigned weihanglo Feb 20, 2023

epage force-pushed the minimal branch from 81223fc to 78d30e4 Compare February 23, 2023 22:22

epage force-pushed the minimal branch from 78d30e4 to 1d153f1 Compare March 1, 2023 18:58

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 Mar 1, 2023

bors merged commit 80f1a5d into rust-lang:master Mar 1, 2023

epage deleted the minimal branch March 1, 2023 20:36

weihanglo mentioned this pull request Mar 2, 2023

Update cargo rust-lang/rust#108665

Merged

ehuss added this to the 1.70.0 milestone Mar 9, 2023

Pat-Lafon mentioned this pull request Apr 27, 2023

Minimal versions lalrpop/lalrpop#782

Merged

elasticdog mentioned this pull request May 7, 2023

Run direct-minimal-versions tests on a weekly basis EarthmanMuons/rustops-blueprint#78

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(resolver): `-Zdirect-minimal-versions` #11688

feat(resolver): `-Zdirect-minimal-versions` #11688

epage commented Feb 7, 2023 •

edited

Loading

rustbot commented Feb 7, 2023

epage commented Feb 7, 2023

Eh2406 commented Feb 8, 2023

epage commented Feb 8, 2023

Eh2406 commented Feb 9, 2023

epage commented Feb 9, 2023

Eh2406 commented Feb 10, 2023

bors commented Feb 14, 2023

epage commented Feb 14, 2023

Eh2406 left a comment

Eh2406 Feb 15, 2023

epage Feb 23, 2023

Eh2406 Feb 25, 2023

epage Mar 1, 2023

weihanglo commented Feb 20, 2023

Eh2406 commented Feb 27, 2023

Eh2406 commented Mar 1, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

feat(resolver): -Zdirect-minimal-versions #11688

feat(resolver): -Zdirect-minimal-versions #11688

Conversation

epage commented Feb 7, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Feb 7, 2023

epage commented Feb 7, 2023

Eh2406 commented Feb 8, 2023

epage commented Feb 8, 2023

Eh2406 commented Feb 9, 2023

epage commented Feb 9, 2023

Eh2406 commented Feb 10, 2023

bors commented Feb 14, 2023

epage commented Feb 14, 2023

Eh2406 left a comment

Choose a reason for hiding this comment

Eh2406 Feb 15, 2023

Choose a reason for hiding this comment

epage Feb 23, 2023

Choose a reason for hiding this comment

Eh2406 Feb 25, 2023

Choose a reason for hiding this comment

epage Mar 1, 2023

Choose a reason for hiding this comment

weihanglo commented Feb 20, 2023

Eh2406 commented Feb 27, 2023

Eh2406 commented Mar 1, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

feat(resolver): `-Zdirect-minimal-versions` #11688

feat(resolver): `-Zdirect-minimal-versions` #11688

epage commented Feb 7, 2023 •

edited

Loading