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

RFC: Make Cargo respect minimum supported Rust version (MSRV) when selecting dependencies #3537

Merged
merged 184 commits into from
Feb 28, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 29, 2023

Rendered
FCP


taken from a post

If the proposed resolver solution is still not to your liking, I would recommend focusing the discussion on (in priority order)

  1. Whether there are disagreements on the core principles
  2. If agreement above, whether there is disagreement with the evaluation of the workflows based on the above principles
  3. If agreement above, whether there is disagreement with the workflow prioritization based on the above evaluation
  4. If agreement above, whether there is disagreement with the evaluation of the solutions impact on the workflows, based on the principles
  5. If agreement above, whether there is disagreement with the selected solution based on the evaluation

In some cases, there is a need for a lot of digging in to understand concerns and use cases. Asynchronous back and forth can be wearing. I do host Office Hours for some synchronous communication. If none of those times work, feel free to reach out to me on Zulip to see if we can find another time.

@epage epage added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Nov 29, 2023
Comment on lines 480 to 483
~~When missing, `cargo publish` could inject `package.rust-version` using the version of rustc used during publish.~~
However, this will err on the side of a higher MSRV than necessary and the only way to
workaround it is to set `CARGO_BUILD_RESOLVER_PRECEDENCE=maximum` which will then lose
all other protections.
Copy link
Member

Choose a reason for hiding this comment

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

A similar alternative I would like to use is to explicitly specify the rust-version during publish, cargo publish --rust-version 1.74. Currently for crates tracking latest stable only there are three options I see, none of which have been acceptable enough for me to start adding rust-version to my crates:

  • Add the rust-version then cargo publish --allow-dirty; breaks the direct link between package and repository
  • Add the rust-version, push to a separate branch/tag, then cargo publish; leaves a lot of offshoots in the repository, is more annoying to integrate with checking CI on the commit before publish
  • Run a 6-weekly CI job to update the rust-version in the repo; results in a lot of PR spam for low-activity projects

Having this instead be applied to the generated Cargo.toml (ideally with an additional comment noting how it was injected) while leaving the Cargo.toml.orig matching the repository would be preferable.

Basing it off the currently used rustc version would be acceptable, but I would like confirmation in that case, it would be too easy to accidentally publish with an older stable or a nightly and correcting this metadata is annoying.

However, this will err on the side of a higher MSRV than necessary...

For some crates this is the advertised MSRV policy though. If that causes issues for downstreams that didn't realise they were using something with such a policy, that's a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run a 6-weekly CI job to update the rust-version in the repo; results in a lot of PR spam for low-activity projects

This is what cargo does

A similar alternative I would like to use is to explicitly specify the rust-version during publish, cargo publish --rust-version 1.74.

Should we expect users to type it in on the command-line correctly or should we add a meta-value like rust-version = "current" that gets populated by rustc --version on publish?

Granted, this would be relegated to the Future Possibilities section. Might not need a full RFC on its own but could potentially be an MCP in cargo.

For some crates this is the advertised MSRV policy though. If that causes issues for downstreams that didn't realise they were using something with such a policy, that's a good thing.

imo the problem is that its implicit, that the author isn't opting into it and acknowledging it as a policy.

I like the idea of separate fields because it bypasses all conversations on the quality of the data. The RFC then intentionally leaves room for us to improve our heuristics for the no-rust-version case over time so we can play with and adjust as we learn from experience. Storing a heuristic in a user-provided field that is immutable makes that a lot harder.

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 what cargo does

Yeah, it works for cargo since it is high-activity. I have crates which I touch about once every 6 months, having 4 PRs changing the rustc-version in that time is... actually not as bad as I was thinking it would be, but still not as good as just injecting the correct value during publish since it's not something that is important to have in-repo.

imo the problem is that its implicit, that the author isn't opting into it and acknowledging it as a policy.

My crates are very explicit about this:

Rust Version Policy

This crate only supports the current stable version of Rust, patch releases may use new features at any time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit from cargo's perspective. What are your thoughts on the idea I mentioned for making that policy explicit with cargo?

Copy link
Member

@Nemo157 Nemo157 Nov 29, 2023

Choose a reason for hiding this comment

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

Ah, having rust-version = "current" in the Cargo.toml would be fine with me too. Though, I'd still require that to have an explicit "Publish with rust-version = "1.74"?" question during cargo publish to avoid accidentally publishing the wrong version (and an extra warning if the current cargo is a nightly build).

I was thinking about the ability to include policy metadata for the later "establish policy on MSRV" section. The data that mentions like "A report of package.rust-version for the latest versions of packages on crates.io" feels like it's just trying to infer policies from the lossy available data. Being able to explicitly specify current - 3 or 6 months ago would make it feasible to actually work out what the situation will be going forward (if how that relates to semver is also worked out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo msrv can help you bisect your MSRV

As for thorough enough static analysis for declaring an MSRV, that is likely to be too large of an effort to take on. At best we can likely get static analysis to help developers find common MSRV problems earlier in their development process than CI.

Copy link

@nicoburns nicoburns Nov 30, 2023

Choose a reason for hiding this comment

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

If there is a "current" shorthand, I think it would be good to provide slightly smarter semver-aware variants "current-major" and "current-minor" that only bump the version when publishing a major or minor versions. Bumping MSRV in patch versions seem like bad practice to me, as it forces people into upgrading Rust in order to get critical bug fixes.

I do not test with anything except the latest stable, so I cannot be sure what the required lower value actually is.

To me, this sort of approach largely undermines the value of this feature, which is dependent on the MSRVs actually being accurate. If crates just put "latest stable at time of publishing" then they'll be too many false positives in the incompatibility detection for it to be useable.

It's pretty easy to add a CI task to test that compilation still works with the set MSRV version, and then you can either bump the version or remove the offending feature if it fails. I feel like we should at least be encouraging this sort of workflow, even if we don't enforce it.

As an example: a crate I maintain - taffy - has a "latest stable" MSRV policy, because we don't want to block use of new features where they are useful. But Taffy actually compiles on Rust versions down to 1.65, because we don't actually need anything newer than that. It would not be great if users of older versions of Rust were blocked from upgrading to new bugfix releases of Taffy simply because we had our MSRV set to "current" when publishing them. It would mean that all users of Taffy would have to use latest stable Rust (or wade through the changelog to every time any transitive dependency publishes an update - but I feel like that's not really feasible in practice), even though it might be the case that none of their dependencies actually require it. And if that's the case, then there would be no need for this feature. They could just use latest stable rust and be done with it.

I also found with clap 2->3 that people assume your MSRV policy based on your MSRV and they can get very frustrated when their expectations are not continued to be met.

Well if users reacted like that to an MSRV bump in a major version, imagine how they will react when there are regular MSRV bumps in patch versions!

Copy link
Contributor Author

@epage epage Nov 30, 2023

Choose a reason for hiding this comment

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

Well if users reacted like that to an MSRV bump in a major version, imagine how they will react when there are regular MSRV bumps in patch versions!

The problem wasn't with which field was bumped but they assumed the MSRV was for years old versions of rust, rather than N-2. I bump MSRV in minor today and the difference between minor and patch is negligable. The main reason I care for clap is it is wide-spread enough and critical enough that I offer people the safety valve of "we can patch old versions if needed". I don't bother with that for most of my projects.

To me, this sort of approach largely undermines the value of this feature, which is dependent on the MSRVs actually being accurate. If crates just put "latest stable at time of publishing" then they'll be too many false positives in the incompatibility detection for it to be useable.

It might undermine package.rust-version itself but it doesn't undermine this feature as it makes it easier to work with what we have today for package.rust-version. Even without this feature, if he verified / advertised MSRV is higher than the absolute minimum then you can't run cargo check but have to do cargo check --ignore-rust-version every single time.

Any MSRV policy besides N puts a burden on a maintainer. We need to be aware of that fact and not shame people if they choose to follow the same support policy as rustc's.

Copy link
Member

@Nemo157 Nemo157 Nov 30, 2023

Choose a reason for hiding this comment

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

It would not be great if users of older versions of Rust were blocked from upgrading to new bugfix releases of Taffy simply because we had our MSRV set to "current" when publishing them.

This RFC does not propose to block crates from using versions that don't advertise support for their MSRV, it simply requires them to acknowledge that they are doing so by using --precise or --ignore-rust-version when pulling those dependencies in. They are already taking on the burden of verifying the dependency actually works with their MSRV if it's not within the dependencies policy, this just makes that information explicit.

To me, this sort of approach largely undermines the value of this feature, which is dependent on the MSRVs actually being accurate.

Current stable at time of publish is an accurate minimum supported rust version, I'm not saying that it might not be possible to compile my crates with an older version, but that is the minimum version on which I support it compiling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it simply requires them to acknowledge that they are doing so by using --precise or --ignore-rust-version when pulling those dependencies in.

To add to that list, if you set your version requirement higher (directly or via cargo add --ignore-rust-version) then you can also bypass this though the discussion at #3537 (comment) could affect how this works.

Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
@joshtriplett

This comment was marked as outdated.

epage and others added 11 commits February 27, 2024 10:21
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
@epage
Copy link
Contributor Author

epage commented Feb 27, 2024

However I still fail to see how the juggling is no longer needed with this RFC. Does this RFC provide an additional strategy to that list? Because as I understand it right now, I still have those same four options. The only difference being that generating Cargo.lock.msrv is easier thanks to the new resolver.

In most cases, you won't need a Cargo.lock.msrv to maintain but can instead have CI generate it on the fly much like how people use -Zminimal-versions to approximate MSRV-aware resolving.

@xStrom
Copy link
Contributor

xStrom commented Feb 27, 2024

Aha, now I understand the disconnect we were having.

In the case where one uses Cargo.lock purely for the purpose of being able to compile with their MSRV toolchain, with this RFC applied, they may no longer have to do that as they can generate a working lock file on the fly.

I say may, because I assume a bunch of packages will continue to be published with no rust-version specified, which means that the automatic resolving will be spotty.

However the reasons why I would commit Cargo.lock go beyond just MSRV compatability. Some packages break semver or behave in other strange ways and I don't want these newly published versions to suddenly break CI. Then there's also build reproducibility. For these and some other reasons I would want to commit Cargo.lock anyway. Thus in my workflow the juggling continues.

I see the situation more clearly now. Thanks for explaining.

@ehuss
Copy link
Contributor

ehuss commented Feb 28, 2024

Thank you everyone for your feedback!

Your participation has helped us gain a better understanding of the different ways people use Cargo and what people's needs are. We recognize that there are a lot of competing opinions on how to meet user needs.

Whichever way we go, there comes a point where we need to move forward. However, it is important to remember that RFCs are not a final specification. This RFC in particular will be stabilized a piece at a time (with cargo new changes likely made last). In preparing to stabilize a feature, we will take into account changes in the ecosystem and feedback from testing unstable features. Based on that evaluation, we may make changes from what this RFC says. Whether we make changes or not, stabilization will then require approval of the cargo team to merge (explicit acknowledgement from all but 2 members with no concerns from any member) followed by a 10 days Final Comment Period (FCP) for the remaining 2 team members and the wider community. Cargo FCPs are now tracked in This Week in Rust to ensure the community is aware when this happens and can participate. Even then, a change like what is proposed for cargo new can be reverted without an RFC, likely only needing to follow the FCP process.

With that in mind, I'm going to merge this RFC based on the approval of the Cargo team.

To track further development, subscribe to the tracking issue here:
rust-lang/cargo#9930

@ehuss ehuss merged commit 31c5614 into rust-lang:master Feb 28, 2024
@epage epage deleted the msrv branch February 28, 2024 12:21
epage added a commit to epage/cargo that referenced this pull request Mar 20, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
epage added a commit to epage/cargo that referenced this pull request Mar 20, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
epage added a commit to epage/cargo that referenced this pull request Apr 6, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
bors added a commit to rust-lang/cargo that referenced this pull request Apr 8, 2024
feat(add): Stabilize MSRV-aware version req selection

### What does this PR try to resolve?

This is part of #9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but leaves validation up to the user as the resolver will pick newer versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537

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

As for determining if this is ready for stabilization:

By stabilizing this without the MSRV-aware resolver, this could be confusing to the workflow with an MSRV-compatible lockfile.
PR #13561 at least raises awareness of that discrepancy.
In general there was interest in the RFC discussions to stabilize this ASAP, regardless of what resolver direction we went.

There is an unresolved question on differences in the resolver vs `cargo add` for dealing with an unset `rust-version` (noted in the tracking issue). However, we are only stabilizing the `cargo add` side which is a very light two-way door as this is a UX-focused command and not a programmatic command.

This hasn't gotten much end-user acceptance testing but, as its UX focused, that seems fine (light, two way door)

As such, this seems like it is ready to stabilize.

### Additional information
@epage
Copy link
Contributor Author

epage commented May 6, 2024

Call for testing

Please see rust-lang/cargo#13873 for steps and where to leave feedback.

TWiR: please link directly to that issue

@epage epage added the call-for-testing Add this label + test notes in comments to be included in TWiR `Call for Testing` section. label May 6, 2024
@U007D
Copy link

U007D commented May 7, 2024

This CfT has been added to TWiR Issue 546.

You may now remove the call-for-testing label. Please feel free to re-add the label if you wish this CfT to appear again in a future issue.

@epage epage removed the call-for-testing Add this label + test notes in comments to be included in TWiR `Call for Testing` section. label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet