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

feat: Stabilize MSRV-aware resolver config #14639

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 3, 2024

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

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

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
@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.

This includes
- `cargo generate-lockfile --ignore-rust-version`
- `cargo update --ignore-rust-version`

This does not include
- `edition = "2024"`
- `resolver = "3"`
@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. 🔔

- `--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

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 force-pushed the stabilize-msrv-config branch 2 times, most recently from cdb8304 to 68d86fc Compare October 16, 2024 12:05
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Oct 17, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 17, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

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.

Some nitpicks as a reader that often forgot what I was reading in previous paragraphs.

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

@bors r+

The FCP has ended. To maximise the testing window I am going to merge this soon. Thank you all for your inputs!

@bors
Copy link
Collaborator

bors commented Oct 18, 2024

📌 Commit 498d4df has been approved by 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 18, 2024
@bors
Copy link
Collaborator

bors commented Oct 18, 2024

⌛ Testing commit 498d4df with merge cf53cc5...

@bors
Copy link
Collaborator

bors commented Oct 18, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing cf53cc5 to master...

@bors bors merged commit cf53cc5 into rust-lang:master Oct 18, 2024
22 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
Update cargo

7 commits in 8c30ce53688e25f7e9d860b33cc914fb2957ca9a..cf53cc54bb593b5ec3dc2be4b1702f50c36d24d5
2024-10-15 16:43:16 +0000 to 2024-10-18 13:56:15 +0000
- feat: Stabilize MSRV-aware resolver config (rust-lang/cargo#14639)
- Help with `[patch.crates.io]` (rust-lang/cargo#14700)
- test: Migrate publish snapshotting to snapbox (rust-lang/cargo#14642)
- Bump to 0.85.0; update changelog (rust-lang/cargo#14695)
- Fix typo in faq.md (rust-lang/cargo#14696)
- fix(registry): `HttpRegistry` `block_until_ready` returns early when work is still pending (rust-lang/cargo#14694)
- fix(resolver): avoid cloning when iterating using RcVecIter (rust-lang/cargo#14690)
@rustbot rustbot added this to the 1.84.0 milestone Oct 19, 2024
@epage epage deleted the stabilize-msrv-config branch October 21, 2024 02:30
bors added a commit that referenced this pull request Oct 21, 2024
docs(ci): Don't constrainty latest_deps job by MSRV

Missed this in #14639
epage added a commit to epage/cargo that referenced this pull request Oct 30, 2024
bors added a commit that referenced this pull request Oct 30, 2024
test: Remove unused msrv-policy

### What does this PR try to resolve?

Missed this in #14639

### How should we test and review this PR?

### Additional information
@epage epage added the relnotes Release-note worthy label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 finished-final-comment-period FCP complete relnotes Release-note worthy S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants