Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(resolver): Lay groundwork for documenting MSRV-aware logic #14662

Merged
merged 13 commits into from
Oct 10, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 9, 2024

What does this PR try to resolve?

This is prep for document the MSRV-aware resolver (see #14639), in particular

  • This give more breathing room for adding this new heuristic to the resolver documentation
  • This provides the context for understanding the limitations

In moving documentation, I asked the question "where would I look to find this if I had a question on it". I tried to balance this by not putting too much formal / technical documentation in more guide-level descriptions. In particular, while "Specifying Dependencies" is in the reference, its also written in somewhat of a guide-style.

There is likely more work that can be done, including

  • Maybe making the "SemVer Compatibility" chapter the de facto reference for Cargo's version of semver that other sections reference for a more exhaustive description.
  • Splitting discussion of the Feature resolver out of the resolver and features documentation. In the current implementation, we have 3 resolve phases (1) lockfile, (2) adapt to the current compilation, (3) resolve features. The last two really serve the same role and I'd consider merging discussion of them.

How should we test and review this PR?

I tried to break up changes in smaller to digest chunks. This means some times a new section doesn't fully jive with another section until a follow up commit. I'd recommend reviewing by commit while having the full diff up on the side to see if a concern is still relevant.

Additional information

At the end of this section, we say the same but add a lot more context.
This is modeled off of `resolver.md` and is prep for moving it to other
places.
This implied SemVer's pre-1.0.0 rules,
rather than Cargo's modified SemVer policy.

The distinction between "minor" and "patch" is
[muddy](https://epage.github.io/blog/2022/02/minor-semver-issue/),
so leaving that for the more detailed documentation to cover.

This also dramatically simplifies the section.
…w expanded

Trying to encourage having places that "own" a concept and letting other areas
better focus.
I'm able to shrink / better focus the text as most of the content is
covered by the version field.
This will let the dependency resolution docs better focus.
This is to provide better context for the sections that come after.
We talked about three different roles of versions within the resolver in
a way that made it easy to miss some of them.
@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2024

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

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2024
and how much the resolver backtracks, affecting resolver performance,
- Unifying versions (`try_unify_version`, `needs_version_unification`):
Ensuring Cargo reuses versions affects build time and allows types from common dependencies to be passed between APIs.
Cargo would prefer to backtrack or error rather than include two versions that would have unified except their [dependency specifications] conflicted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what the sentence is trying to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean

Ensuring Cargo reuses versions affects build time and allows types from common dependencies to be passed between APIs.

I changed it to

Cargo reuses versions where possible to reduce build times and allow types from common dependencies to be passed between APIs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused by "Cargo would prefer to backtrack or error rather than include two versions that would have unified except their [dependency specifications] conflicted."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is talking about the inability to unify "=1.2.4" with "=1.5.6".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are at least four different concepts covered by this sentence (at least the way I'm parsing it):

  • Cargo would prefer
  • backtrack or error rather
  • include two versions that would have unified
  • except their [dependency specifications] conflicted

Often when I've written a convoluted sentence it comes out clearer if I try writing with the clauses in the opposite order. For the last three bullet points that giving something like:

If two [dependency specifications] conflict then they can not be unified on a single version. Cargo backtracks or errors in this case.

I think by "Cargo would prefer" the reader is meant to think about the the other design possibilities the creators of cargo could have chosen. There are so many different other decisions the creators of cargo could have made that it's not clear what is intended by "prefer". If we want to elaborate that point it would be something like.

In this case cargo could have chosen to build both versions, the way npm does, which would lead to more resolutions working but also more code to compile and more version mismatch errors at compile time. Alternatively, cargo could have chosen never to duplicate packages, the way Python does, leading to more dependency conflicts at resolution time but less code to compile and no version mismatch errors at compile time. Cargo chose to do a compromise, requiring unification with in semver compatibility range and duplicating packages where semver incompatible.

But I don't know if that's the level of detail we want to get into, particularly here. Perhaps I've just misunderstood the sentence and "would prefer" was meant to imply something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If two [dependency specifications] conflict then they can not be unified on a single version. Cargo backtracks or errors in this case.

A challenge in writing this is to be so clear we cannot be misunderstood. saying two dependency specifications that can conflict is open ended for the user to make assumptions about what may conflict, like major versions. This is why I was emphasizing that we attempt to unify but can't. Working on a new sentence based on your feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next proposal

If multiple versions would have been unified if it wasn't for conflicts in their [dependency specifications], Cargo will backtrack, erroring if no solution is found, rather than selecting multiple versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely better. We still have some ambiguity about what can and can't be unified.

- Preferring versions (`pick_next_version`):
Cargo may decide that we should prefer a specific version,
falling back to the next when backtracking.
The resolver cannot backtrack because a more-preferred version becomes available.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also having trouble parsing this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part?

The first half is trying to describe our sorting of versions.

The second half is trying to deal with limitations in relying on version order, e.g. a package without an MSRV depending on one that does and we can't backtrack to pick a better version of the non-MSRV package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having trouble with "The resolver cannot backtrack because a more-preferred version becomes available."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the explanation I gave, any thoughts on how to cover this? Or should we skip it and leave it to the MSRV section that I'll add later?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on that situation? What's the alternative design that we would love to have if it was technically possible?

The hope of these questions is that having written it out it becomes clear what point should be summarized here, or it becomes clear that it should be its own paragraph in the MSRV section. In either case the writing/thinking that is done in this comment can be used in that follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less about design decisions but more about implications. I'm trying to help the user take the principles and see the effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and removed it as its hard to talk about it in a theoretical sense as its hard to describe how the resolver could hit this today. With my MSRV documentation, it will be eaiser.

@epage epage changed the title docs(resolver): docs(resolver): Lay groundwork for documenting MSRV-aware logic Oct 9, 2024
@epage epage force-pushed the resolver branch 2 times, most recently from 322fc94 to 66470c7 Compare October 10, 2024 16:42
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this information and clarification. I think it makes the situation a lot clearer for our users!

I'm not going to single-handedly r+, because documentation requires review from many perspectives. But I'm excited to see this land once others have had a chance to review it.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks a lot!

Left some nits but feel free to @bors r=Eh2406,weihanglo (not sure if it is the correct syntax for multiple reviewers)

I am unsure about how resolver actually works. Since Jacob has approved, I am comfortable :)

@@ -39,19 +39,6 @@ with leading zeros. For example, `0.1.0` and `0.1.2` are compatible, but
`0.1.0` and `0.2.0` are not. Similarly, `0.0.1` and `0.0.2` are not
compatible.

As a quick refresher, the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this table as a quick reference. Could we move it to somewhere such as specifying-dependencies.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is where to put it. I would normally put it at the beginning of a reference but:

I tried to balance this by not putting too much formal / technical documentation in more guide-level descriptions. In particular, while "Specifying Dependencies" is in the reference, its also written in somewhat of a guide-style.

We could have a summary section at the end that also includes the recommendations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either works for me.

Let's not block this PR on this, as the original one was in resolver.md, and I don't expect average users to read that chapter. Users above average don't need that either.

If someone requests, we can add it back to somewhere.

src/doc/src/reference/specifying-dependencies.md Outdated Show resolved Hide resolved
src/doc/src/reference/resolver.md Show resolved Hide resolved
src/doc/src/reference/manifest.md Show resolved Hide resolved
@weihanglo
Copy link
Member

@bors r=Eh2406,weihanglo

@bors
Copy link
Contributor

bors commented Oct 10, 2024

📌 Commit 0d072e1 has been approved by Eh2406,weihanglo

It is now in the queue for this repository.

@bors 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 Oct 10, 2024
@bors
Copy link
Contributor

bors commented Oct 10, 2024

⌛ Testing commit 0d072e1 with merge ab71ba9...

@bors
Copy link
Contributor

bors commented Oct 10, 2024

☀️ Test successful - checks-actions
Approved by: Eh2406,weihanglo
Pushing ab71ba9 to master...

@bors bors merged commit ab71ba9 into rust-lang:master Oct 10, 2024
22 checks passed
@epage epage deleted the resolver branch October 13, 2024 01:51
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
Update cargo

14 commits in 15fbd2f607d4defc87053b8b76bf5038f2483cf4..8c30ce53688e25f7e9d860b33cc914fb2957ca9a
2024-10-08 21:08:11 +0000 to 2024-10-15 16:43:16 +0000
- docs: More information on what is and isn't included by cargo package (rust-lang/cargo#14684)
- fix(resolver): share conflict cache between activation retries (rust-lang/cargo#14692)
- fix(git): dont fetch tags by default (rust-lang/cargo#14688)
- Support package selection options like `--exclude` in `cargo publish` (rust-lang/cargo#14659)
- docs: install options -> uninstall options (rust-lang/cargo#14682)
- docs: tools should only interpret a line starting with `{` as JSON (rust-lang/cargo#14677)
- cargo test --help: clarify --tests and --benches (rust-lang/cargo#14675)
- docs(env): minor improvements in environment variables doc (rust-lang/cargo#14676)
- docs: document official external commands (rust-lang/cargo#14669)
- Fix panic when running cargo tree on a package with a cross compiled bindep (rust-lang/cargo#14593)
- Remove the support for `Cargo.toml` of the cargo-script (rust-lang/cargo#14670)
- docs(resolver): Lay groundwork for documenting MSRV-aware logic (rust-lang/cargo#14662)
- chore(deps): update rust crate pulldown-cmark to 0.12.0 (rust-lang/cargo#14668)
- Improve resolver speed (rust-lang/cargo#14663)
@rustbot rustbot added this to the 1.84.0 milestone Oct 16, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 17, 2024
Update cargo

14 commits in 15fbd2f607d4defc87053b8b76bf5038f2483cf4..8c30ce53688e25f7e9d860b33cc914fb2957ca9a
2024-10-08 21:08:11 +0000 to 2024-10-15 16:43:16 +0000
- docs: More information on what is and isn't included by cargo package (rust-lang/cargo#14684)
- fix(resolver): share conflict cache between activation retries (rust-lang/cargo#14692)
- fix(git): dont fetch tags by default (rust-lang/cargo#14688)
- Support package selection options like `--exclude` in `cargo publish` (rust-lang/cargo#14659)
- docs: install options -> uninstall options (rust-lang/cargo#14682)
- docs: tools should only interpret a line starting with `{` as JSON (rust-lang/cargo#14677)
- cargo test --help: clarify --tests and --benches (rust-lang/cargo#14675)
- docs(env): minor improvements in environment variables doc (rust-lang/cargo#14676)
- docs: document official external commands (rust-lang/cargo#14669)
- Fix panic when running cargo tree on a package with a cross compiled bindep (rust-lang/cargo#14593)
- Remove the support for `Cargo.toml` of the cargo-script (rust-lang/cargo#14670)
- docs(resolver): Lay groundwork for documenting MSRV-aware logic (rust-lang/cargo#14662)
- chore(deps): update rust crate pulldown-cmark to 0.12.0 (rust-lang/cargo#14668)
- Improve resolver speed (rust-lang/cargo#14663)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants