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.

Sign up for GitHub

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

Jump to bottom

docs(ref): Clarify MSRV is generally minor #12122

Merged
merged 1 commit into from
May 16, 2023
Merged

docs(ref): Clarify MSRV is generally minor #12122

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/doc/src/reference/semver.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1251,7 +1251,8 @@ projects that are using older versions of Rust. This also includes using new
features in a new release of Cargo, and requiring the use of a nightly-only
feature in a crate that previously worked on stable.

Some projects choose to allow this in a minor release for various reasons. It
It is generally recommended to treat this as a minor change, rather than as
Copy link

Choose a reason for hiding this comment

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

Would people agree with saying "strongly recommended"? The disadvantages are really significant and the advantage is minuscule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reason for being conservative with this was I want this to be merged quickly under the idea that "some improvement is better than no improvement" and I want to avoid discussing too much "how strongly should we state this?". I would prefer we leave that to a follow up PR. Maybe no discussion is needed and thats great! I also know it can be easy for something like to have heavy discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Tbh this wording makes this read that there is a policy, and in fact against treating MSRV upgrades as breaking changes, ever.

It should be less strong e.g.

Most crates are careful with upgrading their minimum supported Rust version in patch releases, but will do it eventually. Usually then, an upgrade happens to a release that is some time in the past so that the breakage only affects a subset of users.

This is at least what I've seen most crates do in practice. It's only a minority of crates that immediately make a minor release using some new features, two days after release day and push it to users in a patch upgrade. There is just a lot of talk around that minority because they break builds for everyone else.

As for the link, IMO that discussion is a good start for reading about several pros and cons but it's still not something where there is consensus, so I'm not sure it's good to link it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh this wording makes this read that there is a policy, and in fact against treating MSRV upgrades as breaking changes, ever.

Because there isn't a policy is why I still left this as a "possible-breaking". As we still leave room open, I am ok with this tentative default stance. We'll see what others on the cargo team think.

It should be less strong e.g.

That isn't just less strong but also adds in a lot of other information to be hashed out and I'm wanting to keep the scope narrow (calling out "patch releases").

As for having larger windows of MSRV support, that is covered later in this section.

As for the link, IMO that discussion is a good start for reading about several pros and cons but it's still not something where there is consensus, so I'm not sure it's good to link it.

  • The link is specifically for the pros/cons
  • Even then, it is the more definitive document at this time for a decision, so if we were to link to something for now, it would be that. I also feel it would give people the appropriate breadcrumb for where to go next, even if its not "decided" yet.

Copy link

Choose a reason for hiding this comment

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

This is about minor vs major, not minor vs patch

Copy link
Member

Choose a reason for hiding this comment

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

but also adds in a lot of other information to be hashed out and I'm wanting to keep the scope narrow (calling out "patch releases").

I am ok with both "minor release" or "patch release", whichever is clearer.

If you write "it is generally recommended" then that sounds a lot like there is a policy.

I agree that "Some projects choose to allow this in a minor release for various reasons." should be replaced by something that is actually lived by the community. But the new wording definitely does not help either.

As for having larger windows of MSRV support, that is covered later in this section.

Actually that's a good point, maybe it might be better to edit it there. This statement makes it seem that most projects don't care about anything but the latest compiler, where only "some" projects support N releases back:

Rust also has a rapid 6-week release cycle, and some projects will provide compatibility
within a window of releases (such as the current stable release plus N
previous releases).

Copy link
Member

@est31 est31 May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The link is specifically for the pros/cons

I'm not a native speaker but from my impression, "for various reasons" has a bit of a double meaning. The diversity that "various" encodes can be interpreted in multiple ways. You can either read it that it's indeed a list of pros and cons, and I think this is what you intended. But you can also read it in the way that from multiple angles, it looks like that making waiting for the next MSRV increase until the next major release is a bad idea. I think it can often be misinterpreted as the second.

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 agree that "Some projects choose to allow this in a minor release for various reasons." should be replaced by something that is actually lived by the community. But the new wording definitely does not help either.

Wordsmithing is hard. If Instead say "most projects", I feel I'd need to verify that first

Actually that's a good point, maybe it might be better to edit it there. This statement makes it seem that most projects don't care about anything but the latest compiler, where only "some" projects support N releases back:

Do you feel that this PR has to expand scope to cover other improvements like this or can we leave that to future PRs?

Copy link
Member

Choose a reason for hiding this comment

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

If Instead say "most projects", I feel I'd need to verify that first

To me it seems like a majority of crates do that, but it's just an impression, and doesn't base on hard data.

Do you feel that this PR has to expand scope to cover other improvements like this or can we leave that to future PRs?

I think one can do it in a future PR.

a major change, for [various reasons][msrv-is-minor]. It
is usually relatively easy to update to a newer version of Rust. Rust also has
a rapid 6-week release cycle, and some projects will provide compatibility
within a window of releases (such as the current stable release plus N
Expand Down Expand Up @@ -1530,3 +1531,4 @@ document what your commitments are.
[struct literal]: ../../reference/expressions/struct-expr.html
[wildcard patterns]: ../../reference/patterns.html#wildcard-pattern
[unused_unsafe]: ../../rustc/lints/listing/warn-by-default.html#unused-unsafe
[msrv-is-minor]: https://github.com/rust-lang/api-guidelines/discussions/231