Skip to content

[RFC] Minimum Supported Rust Version Revisit #449

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

Closed
wants to merge 5 commits into from
Closed

Conversation

jamesmunns
Copy link
Member

@jamesmunns jamesmunns commented Apr 28, 2020

Rendered RFC.

Marked as a draft, as not all fields of the RFC have been completed, but the major points are all there.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up!


* The **MSRV** should be stated in the project README.
* The **MSRV** should be tested using CI.
* If a WG crate begins failing due to a dependency using new features of Rust, the WG crate should issue a semver-trivial release setting a maximum version of that dependency. Additionally, the WG crate may consider making a **Versioning Bump** without setting the maximum version of that dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

the WG crate should issue a semver-trivial release setting a maximum version of that dependency

Are there existing projects that do this?

I've argued against this in the last meeting, and I still feel that this is a bad idea for a general policy. To summarize my thoughts:

  • Defaulting to this will make any consumers of the library miss patch releases with potentially critical bug fixes
  • I believe Cargo's default behavior is to only pull in one semver-compatible version of a crate, but this might break when a maximum version constraint is used (since those are very uncommon). Either way, it will lead to either of these two outcomes:
    • The new release of the dependency will not be used at all because of this constraint, even if other dependencies are fine with the new version.
    • Two versions of the dependency will be used by every crate that depends on the WG crate and an unrelated crate that also pulls in the dependency. (This means there's also no way to opt out here; you'll still get the old version even if you use the new version too, which implies that you're fine with a newer Rust version)

As such, I feel like this solution is not fit to be part of the general MSRV policy. It can still make sense to do this, but that's a case-by-case decision. I think the general policy should be to just bump the WG crate's version accordingly with the next release. Also note that we shouldn't feel like we're forced to do the new release immediately, as an MSRV bump is just a documentation change, so it can be queued up with other changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there existing projects that do this?

I'm not aware of any other projects that do this. I don't have a huge amount of visibility, though.

Defaulting to this will make any consumers of the library miss patch releases with potentially critical bug fixes

That being said, I did try to mitigate this approach by suggesting this in terms of documentation:

  • For users that are sensitive to MSRV changes, we should suggest that they specify the Major and Minor semver version, e.g. version = "1.5.*".
  • For all other users, we should suggest they specify only the Major semver version, e.g. version = "1.*".

This would be the difference between "taking changes, including ones that break MSRV", and "do not take changes that break MSRV".

Without setting an upper bound on the dependency, then we have implicitly increased the MSRV, which at least as described in this RFC, would require a version bump.

but this might break when a maximum version constraint is used

I would suggest that this might be an issue that should be fixed upstream in Cargo itself, if it is something that "should" work.

Two versions of the dependency will be used by every crate that depends on the WG crate and an unrelated crate that also pulls in the dependency. (This means there's also no way to opt out here; you'll still get the old version even if you use the new version too, which implies that you're fine with a newer Rust version)

Couldn't this be fixed by an implicit cargo update -p msrv_breaking_crate step to force re-consolidation of versions? Not sure if I'm completely understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

For users that are sensitive to MSRV changes, we should suggest that they specify the Major and Minor semver version, e.g. version = "1.5.*".

Does this work in practice or does Cargo have any pitfalls with * requirements? Also, the default recommended format should probably just be the usual version = "1.x.y", not version = "1.*".

Couldn't this be fixed by an implicit cargo update -p msrv_breaking_crate step to force re-consolidation of versions? Not sure if I'm completely understanding.

If the WG crate specifies an upper bound, cargo update must respect that. If any other crate does not specify that bound and pulls in a second, newer version, cargo update will not do anything since it doesn't downgrade unless you tell it to choose a specific version of the dependency.

Copy link
Member

Choose a reason for hiding this comment

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

but this might break when a maximum version constraint is used

I would suggest that this might be an issue that should be fixed upstream in Cargo itself, if it is something that "should" work.

I'm not sure it's a good idea. Pulling a single version of dependency instead of multiple of them is better: multiple versions of the same crate can lead to multiple incompatible interfaces.

@adamgreig
Copy link
Member

We might want to consider regular automated CI runs so we are alerted to dependencies breaking our MSRV without someone having to tell us. At the moment I don't think there's any good notification system, but perhaps something could drop a message into the Matrix or display results on a dashboard or something.

@jamesmunns
Copy link
Member Author

We might want to consider regular automated CI runs so we are alerted to dependencies breaking our MSRV without someone having to tell us. At the moment I don't think there's any good notification system, but perhaps something could drop a message into the Matrix or display results on a dashboard or something.

Yes! I definitely meant to include nightly CI checks in the RFC. I will add this.

Co-Authored-By: Jonas Schievink <[email protected]>
@dbrgn
Copy link

dbrgn commented Apr 29, 2020

This looks like a well-balanced RFC to me, I fully support this 🙂 Thanks for the write-up.

@adamgreig
Copy link
Member

adamgreig commented Oct 27, 2020

We recently hit a problem with the current MSRV policy in cortex-m, where a dependency (of a dependency of a dependency) had started to use MaybeUninit in a minor release, which had the effect of increasing the MSRV for the already published version of cortex-m. While that problem in general is hard to avoid, at least this new policy would explicitly permit our own updates with increased MSRV to be non-breaking semver changes.

I've tried to summarise the outstanding comments/suggestions/objections to the current RFC text below:

  • "If a WG crate begins failing due to a dependency using new features of Rust, the WG crate should issue a semver-trivial release setting a maximum version of that dependency."
    • [RFC] Minimum Supported Rust Version Revisit #449 (comment)
    • Causes our dependants to miss updates to that library
    • Might be a breaking change in its own right, since any consumers of our library which were using new features of that dependency would start breaking after updating our library (or they would end up with two copies of the dependency)
    • Perhaps the solution is to just increase our stated MSRV in the next release of our crate, semver-trivial or otherwise, or force a new semver-trivial release with that stated MSRV.
    • Perhaps the wording of our MSRV statement needs to take this into account.
  • Add a mention of periodic automatic CI runs [RFC] Minimum Supported Rust Version Revisit #449 (comment)
  • "Crates maintained by the Embedded WG must always compile..."
  • "The MSRV is inclusive of any possible feature of the crate, as well as the MSRV of all dependencies"
    • We probably want to allow some features to require nightly (e.g. inline-asm), which isn't compatible with an MSRV, and the same is true of our dependencies. Perhaps the MSRV could only apply to the default features, or "unless otherwise indicated".

I think the first point is the most contentious. Does anyone else have suggstions? @jamesmunns, would you be able to update the RFC to take some of the points into account?

@eldruin
Copy link
Member

eldruin commented Oct 28, 2020

About point 1, if we relax the policy to be able to raise the MSRV in this situation, I think it would be good to recommend swiftly releasing a new semver-incompatible version, thus making the previous release as somewhat deprecated.
What I would specially like to avoid is that the MSRV needs to be updated several times within a semver-compatible version because the MSRV then really loses its value.

@jamesmunns
Copy link
Member Author

@adamgreig I have addresses all comments except for 1. in this comment. I feel that this request is incompatible with the policy I describe in this RFC. Specifically:

Regarding @adamgreig's comments:

Causes our dependants to miss updates to that library

I mean, that's the tradeoff with supporting MSRV in my opinion.

Might be a breaking change in its own right, since any consumers of our library which were using new features of that dependency would start breaking after updating our library (or they would end up with two copies of the dependency)

That's a fair comment, but I'm not sure what else we can do, outside of specifying exact/maximum versions of every dependency, and updating on every release

Perhaps the solution is to just increase our stated MSRV in the next release of our crate, semver-trivial or otherwise, or force a new semver-trivial release with that stated MSRV.

The current RFC proposes semver-trivial updates to pin the max acceptable version, and recommends a semver-minor update if we want to continue with a higher MSRV.

Perhaps the wording of our MSRV statement needs to take this into account.

I'm open to specific suggestions.

Regarding @Disasm's most recent comment above:

I'm not sure it's a good idea. Pulling a single version of dependency instead of multiple of them is better: multiple versions of the same crate can lead to multiple incompatible interfaces.

I think this is the tradeoff. Update your MSRV to consolidate versions, or don't, and deal with multiple versions. I don't think there is a tradeoff-less option here.

@jamesmunns jamesmunns marked this pull request as ready for review November 10, 2020 18:54
@jamesmunns jamesmunns requested a review from a team as a code owner November 10, 2020 18:54
@jamesmunns
Copy link
Member Author

Repeating what I said on chat:

I'd like to put a soft limit on proposing any new changes to the MSRV management I described, unless there is something internally inconsistent with the policy I proposed. I understand if people don't like the behavior I proposed, but I don't see any other way of "connecting the dots" without just making different tradeoffs.

If someone else wants to edit or change this proposal and submit a competing RFC, or reject this RFC, I support that as an alternative.

This is ready for approval/rejection.

bors bot added a commit that referenced this pull request Nov 27, 2020
523: [RFC] Alternative new MSRV policy r=therealprof a=adamgreig

This RFC is an alternative to #449, as discussed in this week's meeting ([logs](https://freenode.logbot.info/rust-embedded/20201110#c5777770)). It proposes essentially removing our MSRV policy; the new requirement would be "build on current stable Rust".

I would specifically like to solicit feedback from anyone who benefits from our current MSRV policy (generally old Rust versions, at the least no newer than stable-3, specifically documented and tested against, and updated infrequently). At the moment I'm only directly aware of one use-case for a custom target which requires a custom rustc/llvm build. It would be really useful to hear from people who for whatever reason need to use an older Rust version, and to understand a bit more about why they need that and how this change might affect them.

[Rendered](https://github.com/rust-embedded/wg/blob/new-msrv/rfcs/0523-msrv-2020.md)

Co-authored-by: Adam Greig <[email protected]>
@jamesmunns
Copy link
Member Author

Closing in favor of #523. Thanks all!

@jamesmunns jamesmunns closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants