Skip to content

MSRV policy #304

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

Merged
merged 4 commits into from
Mar 5, 2019
Merged

MSRV policy #304

merged 4 commits into from
Mar 5, 2019

Conversation

japaric
Copy link
Member

@japaric japaric commented Feb 5, 2019

closes #285

@japaric japaric requested review from dylanmckay, jcsoo and a team as code owners February 5, 2019 11:24
@japaric japaric added needs-decision This RFC or PR needs to be approved by the majority of reviewers before it's merged T-all labels Feb 5, 2019
@japaric
Copy link
Member Author

japaric commented Feb 5, 2019

Previous discussion: #285

therealprof
therealprof previously approved these changes Feb 5, 2019
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM

crate is (post-)1.0.

- Cargo features are allowed to depend a on Rust version greater than the MSRV,
even a nightly compiler. For example, a "const-fn" can bump the required
Copy link

@eddyp eddyp Feb 5, 2019

Choose a reason for hiding this comment

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

I think this is a little confusing. I suspect the intention is to allow something like configs which include code that depends on nightly features, since those configs are not necessarily available for stable. Is this correct?

If so, I think some rephrasing is needed.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by 'configs' exactly? I think as-written it's OK, it's talking specifically about features which we'd allow to require more recent Rust versions since they're so commonly used to test or support new compiler features.

Copy link

Choose a reason for hiding this comment

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

I was thinking of #[cfg] and associated macro https://doc.rust-lang.org/std/macro.cfg.html

@korken89
Copy link
Contributor

korken89 commented Feb 5, 2019

I agree with @eddyp comments, else it LGTM

@thejpster
Copy link
Contributor

I also agree with @eddyp 's comments, else it LGTM


- Changing the MSRV of a crate is a breaking change and requires a semver bump:
minor version bump if the crate is pre-1.0 and a major version bump if the
crate is (post-)1.0.
Copy link
Member

Choose a reason for hiding this comment

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

One problem I have run into with several crates where I setup CI for a given MSRV over time is that even if nothing changes in the crate itself to break the MSRV when CI builds the project it uses the latest version of dependencies matching the Cargo.toml specifications as the project does not include one (as it is not an "application" itself).

Obviously, if every crate in the dependency chain followed this rule there is no problem (perhaps a good reason to keep deps to a minimum for central crates) but for a few of the Linux crates which have a few more dependencies I have run into this situation due to crates like nix violating this rule. I guess the only solution in that case would be to use the version controls to enforce a < requirement.

Probably the only good solution to this problem is to get consensus throughout the rust community that an MSRV change is breaking.

Copy link
Member

@posborne posborne left a comment

Choose a reason for hiding this comment

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

I'm happy with this without changes but will happily review if some of the current conversations result in large changes.

@japaric
Copy link
Member Author

japaric commented Feb 17, 2019

Current votes: 2 registered via GH (@Disasm @posborne) + 1 mine + 1 stale (@therealprof) + 2 in the comments (@korken89 @thejpster) = 7 / 27.

Copy link
Member

@nastevens nastevens left a comment

Choose a reason for hiding this comment

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

I'm very much in favor of not updating the MSRV until necessary (i.e. other changes/refactoring makes updating the MSRV logical), but having recently bumped old codebases I also appreciate the ability to use new Rust language features. Basically, so long as the MSRV is explicit and breaking I am all-in.

@japaric japaric added decision-accepted We voted on this proposal and accepted it and removed needs-decision This RFC or PR needs to be approved by the majority of reviewers before it's merged labels Mar 5, 2019
@japaric
Copy link
Member Author

japaric commented Mar 5, 2019

Huzza! This RFC has received enough votes (9 registered + 1 mine of the required 33%) to be accepted.

Teams please start updating the crate level docs of your crates to indicate the MSRV.

bors +

@japaric
Copy link
Member Author

japaric commented Mar 5, 2019

hmm

bors r+

bors bot added a commit that referenced this pull request Mar 5, 2019
304: MSRV policy r=japaric a=japaric

closes #285

Co-authored-by: Jorge Aparicio <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 5, 2019

Build succeeded

@bors bors bot merged commit 8eb6488 into master Mar 5, 2019
@bors bors bot deleted the ops branch March 5, 2019 18:41
@Disasm
Copy link
Member

Disasm commented Sep 5, 2019

Two notes:

  1. MSRV policy does not work if at least one dependency doesn't obey it. Any patch version bump in the dependency tree may change overall actual MSRV.
  2. Semver bump should be done not only for changing MSRV, but for defining MSRV too (for crates without defined MSRV)

@Disasm
Copy link
Member

Disasm commented Sep 5, 2019

It's not clear what should be done in the first case: the problem should be fixed somehow with patch version bump in order to preserve old MSRV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision-accepted We voted on this proposal and accepted it T-all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimum supported Rust version policy?