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

MSRV-dependent dependency version resolution #9930

Open
17 of 25 tasks
newpavlov opened this issue Sep 21, 2021 · 64 comments
Open
17 of 25 tasks

MSRV-dependent dependency version resolution #9930

newpavlov opened this issue Sep 21, 2021 · 64 comments
Labels
A-dependency-resolution Area: dependency resolution and the resolver A-edition-next Area: may require a breaking change over an edition A-rust-version Area: rust-version in Cargo.toml C-tracking-issue Category: A tracking issue for something unstable.

Comments

@newpavlov
Copy link

newpavlov commented Sep 21, 2021

Based on RFC 2495 Rust now supports the rust-version field, which currently works as:

If the currently selected version of the Rust compiler is older than the stated version, cargo will exit with an error, telling the user what version is require

This behavior is not ideal. If a project depends on crate foo v0.1.0 with MSRV 1.56, then releasing crate foo v0.1.1 with MSRV 1.60 will break build of the project on older toolchains after simple cargo update. Ideally Cargo would select foo v0.1.0 on older tolchains, thus preventing the breakage.

RFC 2495 has described MSRV-dependent version resolution as a potential future extension. This issue is intended for design discussions and tracking implementation progress of this feature.

Third-party support


This has been approved as RFC #3537.

Implementation by stabilization milestone

Changes from RFC

Unresolved questions

Deferred

@djc
Copy link
Contributor

djc commented Sep 21, 2021

I think this probably makes more sense in the Cargo repository, since version resolution happens in Cargo -- might even make sense to do an RFC for it. @ehuss @Eh2406 what do you think?

I can see the attraction of this feature, but I'm also worried about it -- if this behavior doesn't come with a warning, it's trivially possible that downstream crates no longer get updated to upstream dependencies that fixed security vulnerabilities, for example. While I usually deploy cargo-deny in CI to make sure I'm aware of this, as long as something similar is not integrated in Cargo it would be a little scary that semver-compatible updates no longer flow as freely as they used to.

@newpavlov
Copy link
Author

newpavlov commented Sep 21, 2021

Yes, having a Cargo warning for cases when there are dependency updates with incompatible MSRV would be a good feature.

As for security vulnerabilities, I think it should be solved by advisories like RustSec and by yanking affected versions. Also note that today crates can have minor (for pre-1.0, major otherwise) version updates with security fixes which do not get ported to older releases. So I don't think that the proposed MSRV-dependent version resolution is fundamentally different from the existing status quo in this regard.

@Eh2406 Eh2406 transferred this issue from rust-lang/rust Sep 21, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 21, 2021

Transferred to Cargo.

IMO one of the big problems blocking this is the error messages. The current Resolver has pretty bad messages, it just reports on the last thing that did not work. So In effect the Resolver will just ignore all versions with a different MSRV leaving no record of why newer versions were not considered. This may be on the list of things to consider when/if we use PubGrub. But that is just my opinion.

@leo60228
Copy link

Is this actually a good idea? I feel like rust-version should solely be a safeguard, and not actually affect behavior. IMO pinning a Rust version without pinning dependencies is user error, and it's better for this to be a hard error instead of doing what might be the wrong thing.

@Eh2406 Eh2406 added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Oct 13, 2021
@gilescope
Copy link
Contributor

broken code doesn't help anyone. If cargo update / upgrade gives warnings that your rust version is too old to bring in all the latest updates then people will be more likely to run cargo upgrade as they are more likely to get a result that works. Pragmatically some stuff upgraded is better than nothing upgraded.

@newpavlov
Copy link
Author

newpavlov commented Oct 21, 2021

@leo60228

Is this actually a good idea?

It was my main motivation for writing the MSRV RFC. Without it I (and many others) consider bumping MSRV a breaking change, since otherwise we risk breaking people's builds on older toolchains (e.g. think of Rust packaged with Debian). Printing an error is just slightly improves ergonomics, no solves the problem at the root.

@djc
Copy link
Contributor

djc commented Oct 21, 2021

I think most maintainers have stepped away from the idea of MSRV bumps requiring a semver bump. Even fundamental things like serde have done it. Instead, just keep a conservative enough MSRV for your crate's audience.

@ehuss ehuss added the A-dependency-resolution Area: dependency resolution and the resolver label Oct 22, 2021
@asomers
Copy link

asomers commented Oct 23, 2021

I think most maintainers have stepped away from the idea of MSRV bumps requiring a semver bump. Even fundamental things like serde have done it. Instead, just keep a conservative enough MSRV for your crate's audience.

But keeping "a conservative enough MSRV for your crate's audience" is not_possible if your upstream dependencies don't do the same. If upstream crates raise their MSRV without a major version bump, then you have little choice but to follow suit. As @leo60228 suggested you could pin those crates versions to before their MSRV bump, but that causes a different problem: Cargo refuses to build two semver-compatible versions of a single crate in a single build. That can result in downstream users' builds breaking, even if they don't care about MSRV. So Rust really does need an MSRV-sensitive resolver.

@matklad
Copy link
Member

matklad commented Oct 25, 2021

pinning a Rust version without pinning dependencies is user error, and it's better for this to be a hard error instead of doing what might be the wrong thing.

One way to resolve this concern is to make the behavior opt-in. By default, behavior is at is today (modulo a better error message), but you can pass --rust-version flag to commands like update or generate-lockfile to request resolution at a specific version.

@djc
Copy link
Contributor

djc commented Oct 26, 2021

But keeping "a conservative enough MSRV for your crate's audience" is not_possible if your upstream dependencies don't do the same. If upstream crates raise their MSRV without a major version bump, then you have little choice but to follow suit.

In theory, this is true. In practice, I've found that upstream maintainers have usually been happy to accomodate my requests for changes to allow a more conservative MSRV -- just a matter of (a) being aware when changes happen and (b) engaging with upstream to voice your concerns/discuss trade-offs.

@HeroicKatora
Copy link

How should this interact with rust-toolchain.toml? At the moment it is only selecting a toolchain from rustup. Should it become the default version for update and generate-lockfile as well?

@newpavlov
Copy link
Author

@Eh2406

IMO one of the big problems blocking this is the error messages.

Wouldn't it be viable to implement an MVP with bad error messages first and work on improving them later?

So In effect the Resolver will just ignore all versions with a different MSRV leaving no record of why newer versions were not considered.

I think it should be fine to start with a simple warning if at least one dependency version was ignored due to the MSRV filtration.

@embediver
Copy link

I came across a related problem, where a CI/CD pipeline failed due to a updated dependency which demanded a higher MSRV than specified for the downstream project.

While I am not sure, that doing a automated MSRV-dependent dependency version resolution is a good idea in general, I would like a feature which notifies you if dependencies can't be build with the specified MSRV.
As far as I can tell, it should be no big deal to implement a warning which, when building, notifies you about this.
This may not prevent the problem that a upstream crate may change the MSRV in a semver compatible release in the future, but ensures that there isn't already a unwanted mismatch at development time.

Furthermore I am not completely sure how its meant to be specified: Should the developer ensure, that the MSRV of his Crate is at least as high as all upstream dependencies?

RFC 2495 mentions this, but there seems to be no check implemented right now.

rust field value will be checked as well. During crate build cargo will check if all upstream dependencies can be built with the specified MSRV. (i.e. it will check if there is exists solution for given crates and Rust versions constraints) Yanked crates will be ignored in this process.

@djc
Copy link
Contributor

djc commented May 30, 2022

While I am not sure, that doing a automated MSRV-dependent dependency version resolution is a good idea in general, I would like a feature which notifies you if dependencies can't be build with the specified MSRV. As far as I can tell, it should be no big deal to implement a warning which, when building, notifies you about this.

Cargo from Rust 1.56 and newer should yield an MSRV-specific error when trying to build a dependency that has its rust-version set to something lower than the currently executing version of the compiler.

Furthermore I am not completely sure how its meant to be specified: Should the developer ensure, that the MSRV of his Crate is at least as high as all upstream dependencies?

Yes, there is currently no way that the tooling can ensure this.

@embediver
Copy link

Cargo from Rust 1.56 and newer should yield an MSRV-specific error when trying to build a dependency that has its rust-version set to something lower than the currently executing version of the compiler.

Just tried to provoke this behaviour with two freshly created crates, and can't really confirm this.
As long as the edition and rust-version key match, there seems to be no warning or error at all.
Or did I get something wrong here.

When building crate A which depends on crate B, I can set rust-version and edition to whatever I like for both crates and won't get a error (e.g. rust-version 1.42 for crate A and 1.61 for crate B).
cargo --version gives me cargo 1.61.0 (a028ae4 2022-04-29).

@djc
Copy link
Contributor

djc commented May 30, 2022

I didn't say you would get an error in that situation. You're building with Cargo 1.61 and both of the crates have 1.61 or older, so there is no error. We currently don't check that depending crates have a newer or equal MSRV compared to the crate they're depending on.

You can refer to the documentation here: https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field.

As far as I can tell, all of this discussion is off-topic for this issue, which is about influencing the Cargo resolver such that it would select the lower version of a dependency if that is a better match for the current Cargo version.

@embediver
Copy link

Ok, thanks for the clarification. And yes its somewhat off-topic, my apologies.

Initially I was just confused that the RFC excerpt I cited isn't implemented in some tooling right now and came up to this Issue.

To contribute at least a bit here: If the Cargo resolver will make some decisions based on the MSRV, the tooling which hints the developer of incompatible MSRVs, is basically included in that. IMO that would be good to have.

@tarcieri
Copy link

Perhaps there could be an unstable option similar to cargo update -Z minimal-versions to perform a resolution that considers MSRV?

bors added a commit that referenced this issue Apr 11, 2024
feat(reslve): Respect '--ignore-rust-version'

### What does this PR try to resolve?

This is a part of #9930.

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

I had considered several ways of implementing this.  I first looked at passing this into `ops::resolve*`.
.This would get a bit annoying with the function signature, so I considered moving it to a builder..
Each of the entry points is slightly different with different ownership needs, making it hard to have a common abstraction.
In doing this, I noticed we currently pass some state around to the resolver via `Workspace`, so I mirrored that.

The nice thing about this location is it provides a good place to hook in config and `package.resolve` so they affect this.

### Additional information
bors added a commit that referenced this issue Apr 12, 2024
fix(help): Generalize --ignore-rust-version

### What does this PR try to resolve?

This is part of #9930 and updates for the help to accommodate #13738 and adding `--ignore-rust-version` to `cargo update`  for when they are stable

This includes
- Moving `--ignore-rust-version` to be under the "Manifest options" header
- Generalizing the help description

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

### Additional information
bors added a commit that referenced this issue Apr 12, 2024
feat(cli): Add --ignore-rust-version to update/generate-lockfile

### What does this PR try to resolve?

This is part of #9930 and extends `--ignore-rust-version` to `cargo update` and `cargo generate-lockfile`

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

First commit sets up tests

### Additional information
bors added a commit that referenced this issue Apr 12, 2024
feat(resolve): Fallback to 'rustc -V' for MSRV resolving

### What does this PR try to resolve?

This is part of #9930 and adds a fallback if the rust-version isn't set

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

Tests are added in a separate commit so people can see how the behavior changed.

### Additional information
epage added a commit to epage/cargo that referenced this issue Apr 14, 2024
This is to help with rust-lang#9930

Example changes:
```diff
-[LOCKING] 4 packages
+[LOCKING] 4 packages to latest version
-[LOCKING] 2 packages
+[LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[LOCKING] 2 packages
+[LOCKING] 2 packages to earliest versions
```

Benefits
- The package count is of "added" packages and this makes that more
  logically clear
- This gives users transparency into what is happening, especially with
  - what rust-version is use
  - the transition to this feature in the new edition
  - whether the planned config was applied or not (as I don't want it to
    require an MSRV bump)
- Will make it easier in tests to show what changed
- Provides more motiviation to show this message in `cargo update` and
  `cargo install` (that will be explored in a follow up PR)

This does come at the cost of more verbose output but hopefully not too
verbose.  This is why I left off other factors, like avoid-dev-deps.
epage added a commit to epage/cargo that referenced this issue Apr 15, 2024
This is to help with rust-lang#9930

Example changes:
```diff
-[LOCKING] 4 packages
+[LOCKING] 4 packages to latest version
-[LOCKING] 2 packages
+[LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[LOCKING] 2 packages
+[LOCKING] 2 packages to earliest versions
```

Benefits
- The package count is of "added" packages and this makes that more
  logically clear
- This gives users transparency into what is happening, especially with
  - what rust-version is use
  - the transition to this feature in the new edition
  - whether the planned config was applied or not (as I don't want it to
    require an MSRV bump)
- Will make it easier in tests to show what changed
- Provides more motiviation to show this message in `cargo update` and
  `cargo install` (that will be explored in a follow up PR)

This does come at the cost of more verbose output but hopefully not too
verbose.  This is why I left off other factors, like avoid-dev-deps.
epage added a commit to epage/cargo that referenced this issue Apr 15, 2024
This is to help with rust-lang#9930

Example changes:
```diff
-[LOCKING] 4 packages
+[LOCKING] 4 packages to latest version
-[LOCKING] 2 packages
+[LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[LOCKING] 2 packages
+[LOCKING] 2 packages to earliest versions
```

Benefits
- The package count is of "added" packages and this makes that more
  logically clear
- This gives users transparency into what is happening, especially with
  - what rust-version is use
  - the transition to this feature in the new edition
  - whether the planned config was applied or not (as I don't want it to
    require an MSRV bump)
- Will make it easier in tests to show what changed
- Provides more motiviation to show this message in `cargo update` and
  `cargo install` (that will be explored in a follow up PR)

This does come at the cost of more verbose output but hopefully not too
verbose.  This is why I left off other factors, like avoid-dev-deps.
bors added a commit that referenced this issue Apr 15, 2024
feat(resolve): Tell the user the style of resovle done

### What does this PR try to resolve?

This is to help with #9930

Example changes:
```diff
-[LOCKING] 4 packages
+[LOCKING] 4 packages to latest compatible version
-[LOCKING] 2 packages
+[LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[LOCKING] 2 packages
+[LOCKING] 2 packages to earliest compatible versions
```

Benefits
- The package count is of "added" packages and this makes that more
  logically clear
- This gives users transparency into what is happening, especially with
  - what rust-version is use
  - the transition to this feature in the new edition
  - whether the planned config was applied or not (as I don't want it to
    require an MSRV bump)
- Will make it easier in tests to show what changed
- Provides more motiviation to show this message in `cargo update` and
  `cargo install` (that will be explored in a follow up PR)

This does come at the cost of more verbose output but hopefully not too
verbose.  This is why I left off other factors, like avoid-dev-deps.

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

### Additional information
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 added a commit to epage/cargo that referenced this issue Apr 18, 2024
This is a part of rust-lang#9930 and is important for changing the default with
the new edition.
epage added a commit to epage/cargo that referenced this issue Apr 19, 2024
This is a part of rust-lang#9930 and is important for changing the default with
the new edition.
epage added a commit to epage/cargo that referenced this issue Apr 19, 2024
This is a part of rust-lang#9930 and is important for changing the default with
the new edition.
epage added a commit to epage/cargo that referenced this issue Apr 20, 2024
This is a part of rust-lang#9930 and is important for changing the default with
the new edition.
bors added a commit that referenced this issue Apr 20, 2024
feat(resolver): Add v3 resolver for MSRV-aware resolving

### What does this PR try to resolve?
This is a part of #9930 and is important for changing the default with the new edition.

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

### Additional information
bors added a commit that referenced this issue Apr 23, 2024
feat(resolver): Add default Edition2024 to resolver v3

### What does this PR try to resolve?

With #13776 done, we can now make MSRV-aware resolver the default for the new edition as part of #9930

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

### Additional information
epage added a commit to epage/cargo that referenced this issue Apr 23, 2024
epage added a commit to epage/cargo that referenced this issue Apr 23, 2024
bors added a commit that referenced this issue Apr 23, 2024
fix(install): Don't respect MSRV for non-local installs

### What does this PR try to resolve?

This is part of #9930

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

### Additional information
bors added a commit that referenced this issue May 1, 2024
fix(resolver): Treat unset MSRV as compatible

### What does this PR try to resolve?

Have the resolver treat no-MSRV as `rust-version = "*"`, like `cargo add` does for version-requirement selection

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

We last tweaked this logic in #13066.
However, we noticed this was inconsistent with `cargo add` in automatically selecting version requirements.

It looks like this is a revert of #13066, taking us back to the behavior in #12950.
In #12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely.

There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the priority?

One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.

See also #9930 (comment)

r? `@Eh2406`

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver A-edition-next Area: may require a breaking change over an edition A-rust-version Area: rust-version in Cargo.toml C-tracking-issue Category: A tracking issue for something unstable.
Projects
Status: In Progress
Development

No branches or pull requests