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

Suggest MSRV policy #227

Closed
wants to merge 4 commits into from
Closed

Suggest MSRV policy #227

wants to merge 4 commits into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Nov 6, 2020

I don't think there's a strong consensus here yet.

However, there seems to be a weak consensus, and also de-facto
practice for many popular crates (cfg-if, lazy-static).
Additionally, there's a lot of continuous discussion and re-litigation
across various forums and issue trackers.

So it seems like tentatively documenting current practice might make sense?

I don't think there's a strong consensus here yet.

However, there seems to be a weak consensus, and also de-facto
practice for many popular crates (`cfg-if`, `lazy-static`).
Additionally, there's a lot of continuous discussion and re-litigation
across various forums and issue trackers.

So it seems like tentatively documenting current practice might make sense?
@matklad
Copy link
Member Author

matklad commented Nov 6, 2020

cc @BurntSushi

@BurntSushi
Copy link
Member

I agree with this recommendation and routinely advocate it. However, I worry about elevating it to the API guidelines because there are some prominent crates that treat an MSRV increase as a semver breaking change. I think the crossbeam and rayon projects do this, for example, and IIRC there are some crypto crates that do as well. (base64 might as well. In practice they do, anyway, but I've never seen their policy clear stated.)

One possible compromise would be to mention that there are some crates that treat MSRV bumps as a semver breaking change, but that the library team does not recommend it.

It would be nice to do an FCP for the library team, but I'm not sure if this repo is hooked up to that. Instead I'll just ping library team members: @Amanieu @KodrAus @dtolnay @sfackler @withoutboats

For me personally, I'd be in favor of this. I think treating MSRV bumps as a semver breaking change is largely impractical. Or more specifically, the advantages of treating MSRV bumps as semver breaking changes do not outweigh the disadvantages. More practically, a number of core crates don't treat MSRV bumps as semver breaking changes, which makes practicing that policy rather difficult in reality anyway.

@matklad
Copy link
Member Author

matklad commented Nov 6, 2020

Added a wording that bumping major is not wrong.

I think the crossbeam and rayon projects do this, for example

It's interesting that rayon didn't do a major bump when upgrading crossbeam to a new major recently: rayon-rs/rayon#807. For those bumping into this issue, this is actually a good example for why not bumping major might be a good idea: now the latest rayon is only compatible with 1.36. If crossbeam published a semver compatible release, new rayon could still be usable with 1.31 (with sutably crafted Cargo.lock, pining crossbeam to an older version).

src/checklist.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
src/necessities.md Outdated Show resolved Hide resolved
@lunacookies
Copy link

Sorry, I really should have done that as one review, instead of individual comments. I’ve never done code review on GitHub before, so please excuse my mistake :)

Co-authored-by: Aramis Razzaghipour <[email protected]>
Comment on lines 144 to 149
To reliably test MSRV on CI, use a dedicated `Cargo.lock` file with dependencies
pinned to minimal versions:

```bash
$ cp ci/Cargo.lock.min ./Cargo.lock
$ cargo +$MSRV test
Copy link
Member

@jyn514 jyn514 Nov 18, 2020

Choose a reason for hiding this comment

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

This is actually the first good usecase I've seen for RUSTC_BOOTSTRAP.

Suggested change
To reliably test MSRV on CI, use a dedicated `Cargo.lock` file with dependencies
pinned to minimal versions:
```bash
$ cp ci/Cargo.lock.min ./Cargo.lock
$ cargo +$MSRV test
To reliably test MSRV on CI, use `-Z minimal-versions`:
```bash
$ RUSTC_BOOTSTRAP=1 cargo +MSRV -Z minimal-versions test
```

That said, maybe it's not a good idea to recommend it so prominently - it might be possible to find the equivalent nightly but it sounds annoying to do.

https://doc.rust-lang.org/cargo/reference/unstable.html#minimal-versions

Copy link
Member

@taiki-e taiki-e Nov 19, 2020

Choose a reason for hiding this comment

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

I definitely do not recommend using RUSTC_BOOTSTRAP + -Z minimal-versions:

  • -Z minimal-versions is added in cargo 1.27. It does not work on older versions. (rust-lang/cargo@9a09892)
  • If dependencies are incompatible with -Z minimal-versions, compile will fail for reasons unrelated to MSRV.
  • It is an unstable feature, so may be changed in the future.

(Note that dev-dependencies may raise version requirements, so running test with -Z minimal-versions may not work properly for its intended purpose. Ideally, it needs to run cargo update -Z minimal-versions after completely remove the dev-dependencies. See tokio-rs/tokio#3131 (comment) for more.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I concur that manually updating Cargo.lock.min at this moment is likely to require less maintenance. But -Z minimal-versions is clearly an interesting alternative, I bet someone will learn about it from this comment!

In practice, dev-deps often have much more lenient MSRV policy than
then crate under test. Using `build` rather than `test` works around
that.
@KodrAus
Copy link
Contributor

KodrAus commented Dec 22, 2020

Thanks everyone for participating so far! We're currently trying to wrangle the API Guidelines project a bit by shifting discussion towards GitHub's discussions feature where possible and reserve PRs for guidelines that have been generally accepted already. This hasn't been communicated at all yet so sorry everyone for jumbling things around here 🙇

I've opened #231 to continue the discussion on MSRVs and will close this one for now.

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