book: describe how the current resolver sometimes duplicates deps #11604

pkgw · 2023-01-20T16:53:06Z

What does this PR try to resolve?

This updates the book to document the behavior described in #9029, where sometimes Cargo will duplicate a dep when it doesn't have to.

How should we test and review this PR?

Doc-only change; someone with knowledge of the resolver should read and assess.

This documents the behavior described in rust-lang#9029, where sometimes Cargo will duplicate a dep when it doesn't have to.

rustbot · 2023-01-20T16:53:11Z

r? @ehuss

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

Eh2406

Thank you for working on this, this is a great example.

Eh2406 · 2023-01-20T17:17:17Z

src/doc/src/reference/resolver.md

@@ -7,8 +7,10 @@ result of the resolution is stored in the `Cargo.lock` file which "locks" the
 dependencies to specific versions, and keeps them fixed over time.

 The resolver attempts to unify common dependencies while considering possibly
-conflicting requirements. The sections below provide some details on how these
-constraints are handled, and how to work with the resolver.
+conflicting requirements. Dependency resolution is, however, an NP-hard problem,


Nit: I don't think it's the NP-hardnes of the problem that causes counterintuitive results. The heuristic comes in in deciding which solution, among the seemingly infinite, valid solutions to pick.

I'd argue that they're strongly related if not strictly causative. Or, to put it another way, if the problem wasn't NP-hard, I suspect that resolution would produce counterintuitive results much more rarely, possibly never — but also it doesn't feel very helpful to speculate about a hypothetical different problem!

(addendum: In other words I feel like there's a relation between NP-hard-ness and "seemingly infinite" solution sets that do not admit a strict ranking, although I can think of counterexamples and am not very familiar with the relevant theory.)

Having thought about your comment for three days, and sorry for the slow response, I think you are technically correct. One of the definitions of NP-hard is "exponential to find a solution, polynomial to check a solution". And what want for our example is a problem where "the solutions are so simple that people don't think about the algorithm" but where "there are nearly infinite number of possible solutions". So yes, these seem to be isomorphic definitions.

Which still leaves us with question about how we best communicate with a Rust user. Who is probably reading this section because they are frustrated that cargo gave them the "wrong" solution to a simple problem. (Where "wrong" can be "why did you bloat my binary with more than one version" or "why did you give me this ancient version just because it's somewhere else in my dependency tree".) This user may very well never have heard of NP-hard, I certainly had not when I started using Rust. Even if they have, it may not be a clear connection, as I demonstrated by taking three days to understand your point.

I've rephrased this bit (in response to @ehuss's comments as well). I do think that it would be illuminating if we can find a good way to introduce the idea that dependency resolution is not as easy as one might naively think.

Eh2406 · 2023-01-20T17:19:12Z

src/doc/src/reference/resolver.md

@@ -91,6 +93,31 @@ at the time of this writing) and Package B will use the greatest `0.6` release
 (`0.6.5` for example). This can lead to potential problems, see the
 [Version-incompatibility hazards] section for more details.

+The resolver algorithm favors the greatest available versions of dependencies
+and has limited support for backtracking in the solutions it attempts.


"and has limited support for backtracking in the solutions it attempts." I don't think this is technically true. If there is no valid solution, it can backtrack as much as it needs to to prove that. Potentially trying literally every possible combination of versions. It does not do backtracking to find a "better" solution than the valid solution it currently has.

What are we trying to say with the sentence? And how can we say it more clearly?

This is where your familiarity with the actual algorithm is probably needed to be more precise. The intention is to temper expectations set by an earlier sentence:

When multiple packages specify a dependency for a common package, the resolver attempts to ensure that they use the same version of that common package,

... which would make me expect the example to resolve to a non-duplicated solution. Would it be fair to say that "the resolver does not optimize to reduce crate duplications", "does not have a deduplication pass", or something along those lines?

That makes sense. In effect were trying to elaborate on the part of the quote you left out:

When multiple packages specify a dependency for a common package, the resolver
attempts to ensure that they use the same version of that common package, as
long as they are within a SemVer compatibility range.

Specifically within open requirement cargo treated as a case of "If multiple packages have a common dependency with semver-incompatible versions..." instead of a case of "When multiple packages specify a dependency for a common package, the resolver attempts to ensure that they use the same version of that common package...", and it does not do something fancy like trying to solve for the second and then falling back to the first if there's an error. 🤔 about how best to say that.

Words are hard.

I've rephrase this piece fairly significantly.

ehuss · 2023-01-23T14:14:01Z

src/doc/src/reference/resolver.md

+and so the resolver uses a heuristic algorithm that may yield counterintuitive
+results in some cases. The sections below provide some details on how


I would be reluctant to start the chapter emphasizing that Cargo's primary job may be "counterintuitive". I personally would prefer to leave out this new sentence. Can you say what the intent of it is?

I think that someone new to dependency management might naively think, "OK, Cargo gets all the requirements, finds the versions that work, easy enough" — I don't think it's obvious to a newcomer that dependency-resolution is, indeed, an NP-hard problem where there may be no clear "best" solution to converge to. My intent here was to raise that idea — "this might sound straightforward but actually it isn't always!"

src/doc/src/reference/resolver.md

pkgw · 2023-01-24T00:39:15Z

I've updated the PR based on the current feedback.

Eh2406 · 2023-01-24T03:27:02Z

Thank you for the clear description of a confusion that is often brought up. I appreciate your follow through on making this documentation better.

ehuss · 2023-01-25T19:31:50Z

This looks great, thank you!

@bors r+

bors · 2023-01-25T19:31:52Z

📌 Commit 50588a9 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-01-25T19:31:58Z

⌛ Testing commit 50588a9 with merge 4a2ebfb...

bors · 2023-01-25T20:18:27Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 4a2ebfb to master...

18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621)

Update cargo 18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621) r? `@ghost`

docs(ref): describe how the current resolver sometimes duplicates deps

b0b0d91

This documents the behavior described in rust-lang#9029, where sometimes Cargo will duplicate a dep when it doesn't have to.

rustbot assigned ehuss Jan 20, 2023

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

pkgw mentioned this pull request Jan 20, 2023

Major-open semver range does not properly unify with closed semver ranges #9029

Open

Eh2406 requested changes Jan 20, 2023

View reviewed changes

ehuss reviewed Jan 23, 2023

View reviewed changes

docs(ref): revise resolver duplication edits based on feedback

50588a9

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 Jan 25, 2023

bors merged commit 4a2ebfb into rust-lang:master Jan 25, 2023

pkgw deleted the resolver-unification-docs branch January 25, 2023 20:22

weihanglo mentioned this pull request Feb 1, 2023

Update cargo rust-lang/rust#107541

Merged

ehuss added this to the 1.69.0 milestone Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

book: describe how the current resolver sometimes duplicates deps #11604

book: describe how the current resolver sometimes duplicates deps #11604

pkgw commented Jan 20, 2023

rustbot commented Jan 20, 2023

Eh2406 left a comment

Eh2406 Jan 20, 2023

pkgw Jan 20, 2023 •

edited

Loading

Eh2406 Jan 23, 2023

pkgw Jan 24, 2023

Eh2406 Jan 20, 2023

pkgw Jan 20, 2023

Eh2406 Jan 23, 2023

pkgw Jan 24, 2023

ehuss Jan 23, 2023

pkgw Jan 23, 2023

pkgw commented Jan 24, 2023

Eh2406 commented Jan 24, 2023

ehuss commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

		and so the resolver uses a heuristic algorithm that may yield counterintuitive
		results in some cases. The sections below provide some details on how

book: describe how the current resolver sometimes duplicates deps #11604

book: describe how the current resolver sometimes duplicates deps #11604

Conversation

pkgw commented Jan 20, 2023

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Jan 20, 2023

Eh2406 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkgw Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkgw commented Jan 24, 2023

Eh2406 commented Jan 24, 2023

ehuss commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

pkgw Jan 20, 2023 •

edited

Loading