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

fix(config): Adjust MSRV resolve config field name / values #14296

Merged
merged 3 commits into from
Jul 31, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Jul 24, 2024

What does this PR try to resolve?

Fixes #13540

How should we test and review this PR?

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
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-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2024
@epage epage marked this pull request as draft July 24, 2024 13:54
@epage epage force-pushed the msrv branch 2 times, most recently from 655ae80 to feecbd7 Compare July 24, 2024 15:05
src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
Select which policy should be used when resolving dependencies. Values include
- `something-like-maximum`: prefer highest compatible versions of a package
- `something-like-rust-version`: prefer versions of packages compatible with your project's Rust version
Select how packages with incompatible rust-versions should be resolved. Values include:
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear what an incompatible rust-version is. Should we link "incompatible rust-versions" to package.rust-version in the manifest?

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've called out the field name. We should eventually turn all of those into links.

- `something-like-maximum`: prefer highest compatible versions of a package
- `something-like-rust-version`: prefer versions of packages compatible with your project's Rust version
Select how packages with incompatible rust-versions should be resolved. Values include:
- `allow`: treat them like any other dependency
Copy link
Member

Choose a reason for hiding this comment

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

This is also unclear as user doesn't know how other dependency is treated. Are we making it vague for rooms for future behavior changes?

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 can't think of a reason to be intentionally vague about allow, I'm just unsure of what more needs to be described.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is pronouns and “other”. What is included in “other”? Maybe something like

Suggested change
- `allow`: treat them like any other dependency
- `allow`: treat `package.rust-version` incompatible dependency as compatible
- `fallback`: only consider `package.rust-version` incompatible dependency if no other compatible dependency version matched

Pronouns are often confusing, and I prefer to use them less.

(Maybe it is just me 😬)

Copy link
Contributor Author

@epage epage Jul 31, 2024

Choose a reason for hiding this comment

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

To be fully correct, its an "incompatible dependency version" (not just dependency). This starts to become a mouthful. I gave it a try by skipping on package. and just saying "version".

(and fixed places where I said package or dependency instead of version)

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
@epage epage marked this pull request as ready for review July 30, 2024 20:07
@epage
Copy link
Contributor Author

epage commented Jul 30, 2024

Removing the draft status as I've given time for some community input with only a passing reference to not liking something but the person didn't engage to get more details.

@weihanglo
Copy link
Member

Removing the draft status as I've given time for some community input with only a passing reference to not liking something but the person didn't engage to get more details.

Where can I find this?

@epage
Copy link
Contributor Author

epage commented Jul 31, 2024

Removing the draft status as I've given time for some community input with only a passing reference to not liking something but the person didn't engage to get more details.

Where can I find this?

From https://hachyderm.io/@djc/112842279033657989

@epage so the current behavior is allow and the new default would be fallback, right? I don’t love fallback because it describes a sort of meta-behavior that the resolver implements rather than a decision on the particular package version. From that perspective, I feel like “ignore” might be a better fit? “When Cargo encounters a dependency version with an incompatible rust-version, it allows/ignores that version.”

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.

This seems fine to merge now. We will do an FCP when stabilizing it.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 31, 2024

📌 Commit 48c5f35 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 Jul 31, 2024
@bors
Copy link
Collaborator

bors commented Jul 31, 2024

⌛ Testing commit 48c5f35 with merge 11dccd1...

@bors
Copy link
Collaborator

bors commented Jul 31, 2024

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

@bors bors merged commit 11dccd1 into rust-lang:master Jul 31, 2024
22 checks passed
@epage epage deleted the msrv branch August 1, 2024 14:53
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
Update cargo

15 commits in 257b72b8adfb1f2aa9916cefca67285c21666276..fa646583675d7c140482bd906145c71b7fb4fc2b
2024-07-30 15:08:27 +0000 to 2024-08-02 16:08:06 +0000
- refactor(toml): Rename 'resolved' to 'normalized' (rust-lang/cargo#14342)
- faq: rephrase offline usage. (rust-lang/cargo#14336)
- docs(unstable): Improve nightly link (rust-lang/cargo#14344)
- Fix a typo in 1.81 changes (rust-lang/cargo#14343)
- Change tests to support `rustc` wording changes (rust-lang/cargo#14341)
- chore(deps): update rust crate windows-sys to 0.59 (rust-lang/cargo#14335)
- chore(deps): update rust crate gix to 0.64.0 (rust-lang/cargo#14332)
- chore(deps): update compatible (rust-lang/cargo#14331)
- chore(deps): update rust crate rusqlite to 0.32.0 (rust-lang/cargo#14334)
- fix: also build manpage for cargo.md (rust-lang/cargo#14339)
- fix(config): Adjust MSRV resolve config field name / values (rust-lang/cargo#14296)
- fix(toml): Resolve regression from toml_edit 0.22.18 (rust-lang/cargo#14329)
- test(publish): More dev-dep stripping cases (rust-lang/cargo#14327)
- Use gmake on AIX (rust-lang/cargo#14323)
- fix(publish): Don't strip non-dev features (rust-lang/cargo#14325)

r? ghost
@rustbot rustbot added this to the 1.82.0 milestone Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-workspaces Area: workspaces 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.

Config support for controlling MSRV-aware resolver
4 participants