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.

Sign up for GitHub

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

Jump to bottom

feat: Stabilize MSRV-aware resolver config #14639

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Stabilize MSRV-aware resolver config #14639

wants to merge 2 commits into from
+128 −243

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 3, 2024 •
edited
Loading

What does this PR try to resolve?

This includes

  • cargo generate-lockfile --ignore-rust-version
  • cargo update --ignore-rust-version

This does not include

  • edition = "2024"
  • resolver = "3"

This is part of #9930

How should we test and review this PR?

Additional information

This is stacked on top of #14636. The commits for this PR start with the commit with a title that matches the PR title.

FCP

@epage epage added the T-cargo Team: Cargo label Oct 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 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-cli Area: Command-line interface, option parsing, etc. A-documenting-cargo-itself Area: Cargo's documentation A-workspaces Area: workspaces Command-generate-lockfile Command-update S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2024
@epage
Copy link
Contributor Author

epage commented Oct 3, 2024 •
edited
Loading

Note: the final touches are not in place for this PR but I assume people can check their boxes based on the whole and assuming comments on this PR will get resolved.

I propose that we stabilize the MSRV-aware resolver.

  • Offer an opt-in config for preferring MSRV-compatible packages over those that are incompatible, regardless of version
  • Packages without an MSRV are always considered compatible
  • If no MSRV is defined, fallback to the current toolchain's version
  • Small tweaks in the algorithm are allowed as changes in the algorithm won't cause changes in lockfiles
  • This only applies to local lockfile generation and not to cargo install

This includes stabilizing

  • cargo generate-lockfile --ignore-rust-version
  • cargo update --ignore-rust-version

This does not include stabilizing

  • edition = "2024"
  • resolver = "3"
  • Features around the resolver that are meant to support MSRV workflows (e.g. --update-rust-version flag)

Deviations from the RFC

  • Instead of the MSRV being the lowest among the workspace's MSRVs, we prioritze packages based on how many MSRVs from the workspace they are compatible with
    • We only fallback to the current toolchain's version if no MSRV is set, rather than treating that as the MSRV for those packages

I had considered proposing we have this cover cargo install

  • When viewed only as a config and keeping in mind that cargo install only reads from the user config (and so wouldn't be affected by the current repo's config), it sounds reasonable on the surface
  • However, people setting something in their config for cargo install would affect any repo that doesn't have its own config. Really, cargo install does need its own [install.resolver] table.
  • This also gets weirder when it comes to resolver = "3" as we'd need to describe that as conditional

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 3, 2024 •
edited by Eh2406
Loading

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Oct 3, 2024
@epage epage mentioned this pull request Oct 3, 2024
Open
26 tasks
@bors
Copy link
Collaborator

bors commented Oct 4, 2024

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

@epage
feat: Stabilize MSRV-aware resolver config
ecf2406 
This includes
- `cargo generate-lockfile --ignore-rust-version`
- `cargo update --ignore-rust-version`

This does not include
- `edition = "2024"`
- `resolver = "3"`
@epage
docs(ref): Document MSRV-aware resolver
cdb8304
@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Oct 7, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 7, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

weihanglo
weihanglo reviewed Oct 8, 2024
View reviewed changes
src/doc/src/reference/unstable.md Outdated
- `--ignore-rust-version` CLI option
- Setting the dependency's version requirement higher than any version with a compatible `rust-version`
- Specifying the version to `cargo update` with `--precise`
This was stabilized in 1.83 in #.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This was stabilized in 1.83 in #.
This was stabilized in 1.83 in [#14639](https://github.com/rust-lang/cargo/pull/14639).

BTW, do you plan not to move anything to "Stabilized" section until every feature is implemented? Should we move just these two stabilized feature under independent h2 headings, which won't break link validity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hadn't really thought that through. Its a question of whether the stable section is documenting features or flags. Overall, I don't have a strong preference.

Copy link
Member

@weihanglo weihanglo Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am slightly towards moving them, but not going to block this PR for that reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As its already that way for some parts, let's handle the decision separately

src/doc/src/reference/config.md Outdated Show resolved Hide resolved
@epage epage mentioned this pull request Oct 9, 2024
Merged
weihanglo
weihanglo approved these changes Oct 10, 2024
View reviewed changes
src/doc/src/reference/unstable.md Outdated
- `--ignore-rust-version` CLI option
- Setting the dependency's version requirement higher than any version with a compatible `rust-version`
- Specifying the version to `cargo update` with `--precise`
This was stabilized in 1.83 in #.
Copy link
Member

@weihanglo weihanglo Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am slightly towards moving them, but not going to block this PR for that reason.

bors added a commit that referenced this pull request Oct 10, 2024
@bors
Auto merge of #14662 - epage:resolver, r=Eh2406,weihanglo
ab71ba9 
docs(resolver): Lay groundwork for documenting MSRV-aware logic

### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

@weihanglo weihanglo weihanglo approved these changes

Assignees

@ehuss ehuss

Labels
A-cli Area: Command-line interface, option parsing, etc. A-documenting-cargo-itself Area: Cargo's documentation A-workspaces Area: workspaces Command-generate-lockfile Command-update disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@epage @rustbot @rfcbot @bors @ehuss @weihanglo