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

Config support for controlling MSRV-aware resolver #13540

Closed
Tracked by #9930
epage opened this issue Mar 5, 2024 · 6 comments · Fixed by #14296
Closed
Tracked by #9930

Config support for controlling MSRV-aware resolver #13540

epage opened this issue Mar 5, 2024 · 6 comments · Fixed by #14296
Labels
A-configuration Area: cargo config files and env vars A-dependency-resolution Area: dependency resolution and the resolver C-enhancement Category: enhancement

Comments

@epage
Copy link
Contributor

epage commented Mar 5, 2024

Split out of #9930 for RFC #3537 for a more focused conversation

We need a config field but this was left undefined in the RFC

@epage epage added A-configuration Area: cargo config files and env vars C-enhancement Category: enhancement A-dependency-resolution Area: dependency resolution and the resolver labels Mar 5, 2024
@epage
Copy link
Contributor Author

epage commented Mar 5, 2024

The RFC proposed

[build]
resolver.precedence = "rust-version"  # Default with `v3`

with the following values (on first release)

  • maximum
  • rust-version (falls back to rustc version if not present)

Future possibilities

@epage
Copy link
Contributor Author

epage commented Mar 5, 2024

For rustc value, I think we should see how to align with the package.rust-version = "tbd" field so people can connect the dots more easily.

Like with term (#13337), I wonder if we should be doing more individual fields rather than be-all fields.

Something like:

[resolver]
maximum = <true|false>
rust-version = <true|false|manifest|toolchain|<X>.<Y>.<Z>>
# yanked = <allow|ignore>
# prerelease = <allow|ignore>

This would also give control over #13290, #4225

We can have a separate [install.resolver]

Thoughts

  • I'd prefer more consistency in field names and values
  • I'd like to cover cases of "deny <incompatible|yanked|prerelease>", "discourage <...>", "allow <...>"
  • To satisfy the above two, i suspect we should break out the "max rust version" from the "rust version policy" fields

This leads to something like

[resolver]
maximum = <true|false>
incompatible-rust-version = <deny|discourage|allow>
yanked = <deny|discourage|allow>
prerelease = <deny|discourage|allow>

rust-version = <workspace|toolchain|<X>.<Y>.<Z>>
  • Not thrilled with the names
  • Not liking the location of rust-version as it should also apply to cargo add (but not cargo check?)
    • Maybe [build] could have its own rust-version field
    • Maybe we could call this table [dependencies] and have it apply to cargo add?

@epage
Copy link
Contributor Author

epage commented Apr 4, 2024

I wonder if, instead of maximum = <true|false> with minimum being what you get with maximum = false, if it'd be better to have a <something> = <maximum|minimum>.

@epage
Copy link
Contributor Author

epage commented Apr 4, 2024

Talking this through with @Eh2406

  • Fewer fields offers a higher level experience with a combinatoric problem
  • More fields offers a lower level experience, allowing people to build what they want, but some combinations might not make sense

There isn't really much pushing us one direction or the other, so one way to look at this is which way is easier to evolve. Having fewer fields makes it so we can change it so that field sets defaults on the others. Having more fields makes that a little messier.

Current proposal

[resolver]
precedence = "<maximum|rust-version>"
# precedence = "<maximum|rust-version|minimum>"
# future: rust-version = "<workspace|toolchain|<X>.<Y>.<Z>"

epage added a commit to epage/cargo that referenced this issue Apr 10, 2024
This will hopefully help when merging between CLI and config with rust-lang#13540.
bors added a commit that referenced this issue Apr 10, 2024
refactor: Track when MSRV is explicitly set, either way

### What does this PR try to resolve?

This will hopefully help when merging between CLI and config with #13540.

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

### Additional information
bors added a commit that referenced this issue Apr 13, 2024
refactor(config): Consistently use kebab-case

This shouldn't change the behavior but makes it safer if
- We add new fields where it will matter
- Copy/paste these for new structs

I did not change things related to the Index because we are already stuck with that case (whether we want it or not)

Came across this when working on #13540 and almost made the mistake of copying what was already there
epage added a commit to epage/cargo that referenced this issue Apr 17, 2024
This is a part of rust-lang#13540 which is a party of rust-lang#9930.

The config is `resolver.something-like-precedence` with values:
- `something-like-maximum` (default)
- `something-like-rust-version`

This is punting on the actual config schema so we can implement
`package.resolver` and `edition = "2024"` support as we want the
MSRV-aware resolver available without `cargo_features`.
epage added a commit to epage/cargo that referenced this issue Apr 18, 2024
This is a part of rust-lang#13540 which is a party of rust-lang#9930.

The config is `resolver.something-like-precedence` with values:
- `something-like-maximum` (default)
- `something-like-rust-version`

This is punting on the actual config schema so we can implement
`package.resolver` and `edition = "2024"` support as we want the
MSRV-aware resolver available without `cargo_features`.
epage added a commit to epage/cargo that referenced this issue Apr 18, 2024
This is a part of rust-lang#13540 which is a party of rust-lang#9930.

The config is `resolver.something-like-precedence` with values:
- `something-like-maximum` (default)
- `something-like-rust-version`

This is punting on the actual config schema so we can implement
`package.resolver` and `edition = "2024"` support as we want the
MSRV-aware resolver available without `cargo_features`.
bors added a commit that referenced this issue Apr 18, 2024
fix(msrv): Put MSRV-aware resolver behind a config

### What does this PR try to resolve?
This is a part of #13540 which is a party of #9930.

The config is `resolver.something-like-precedence` with values:
- `something-like-maximum` (default)
- `something-like-rust-version`

This is punting on the actual config schema so we can implement
`package.resolver` and `edition = "2024"` support as we want the
MSRV-aware resolver available without `cargo_features`.

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

One of the included test cases shows a bug with `cargo install`.  Resolving that will be tracked in #9930

### Additional information
@epage
Copy link
Contributor Author

epage commented Jun 5, 2024

We talked about this in the Cargo team meeting today.

We lean away from resolver.precedence / resolver.policy because

  • It will quickly run into scaling issues
  • It reads a little funny to have maximum and rust-version as mutually exclusive

When it came to resolver.rust-version, the biggest concerns were

  • Naming that aligns with potential future expansion
  • How to name the new resolver behavior of using incompatible-rust-version as a "last resort"

We leaned towards resolver.incompatible-rust-version

  • Right polarity for potential other fields
  • Keeps it distinct from setting a "resolve to rust version" field
  • Expands to support rejecting of incompatible rust version package versions in the future

For the resolver.incompatible-rust-version field, we have three potential states

  • "treat like everything else" (current behavior)
  • "use if no other option" (new behavior)
  • "use only if already in lockfile" (not planned atm)

Potential names for "use if no other option" include:

  • discourage
  • eschew
  • avoid
  • fallback

Fallback seems like just the right weight. The challenge is that its a
noun while other words we are considering are verbs (allow, deny).

@epage
Copy link
Contributor Author

epage commented Jul 10, 2024

Goal: provide field values for

[resolver]
incompatible-rust-version = "<field>"  # can be one of two meanings for now, expandable to three

Stepping back to more fundamentals and playing a game of mad libs...

We need to pick one specific version of a software package from among many

  • candidates
  • potential deps
  • options
  • prospects
  • nominees

Some of these "candidate" versions of the package will be

  • filtered out
  • ignored
  • rejected
  • denied
  • disallowed

After that, we then choose the highest priority package from among "candidates". We aren't wanting to have users manually specify the priority order but just to say whether we

  • ignore
  • are agnostic of
  • allow
  • treat as "normal", "standard"

certain traits or if certain traits are

  • discouraged
  • eschewed
  • avoided

when no other option is available. Or in other words, these are "fallback" versions

We should also put these in a sentence with the trait

  • packages with "incompatible rust versions" are ...
  • ... packages with "incompatible rust versions"

Examples

  1. allow, deny, fallback
  • "allow packages with incompatible-rust-versions"
  • "deny packages with incompatible-rust-versions"
  • "fallback to packages with incompatible-rust-versions"

bors added a commit that referenced this issue Jul 31, 2024
fix(config): Adjust MSRV resolve config field name / values

### What does this PR try to resolve?

Fixes #13540

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

### Additional information
@bors bors closed this as completed in 8622918 Jul 31, 2024
stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this issue Aug 7, 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-dependency-resolution Area: dependency resolution and the resolver C-enhancement Category: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant