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

Explicitly specify minimum supported rust version #2400

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

kchibisov
Copy link
Member

This should help with distributing apps using winit.

Fixes #1075.

--

Will require a patch bump afterwards unfortunately.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I don't mind testing some minimum Rust version in CI (as long as we don't start saying "no, I won't accept this feature because it bumps the MSRV").

Note that we can't guarantee this for downstream users without all of our dependencies also setting the field to the same version or lower!

@dhardy
Copy link
Contributor

dhardy commented Jul 27, 2022

Want a badge in the README?

![Minimum rustc version](https://img.shields.io/badge/rustc-1.57-lightgray.svg)

@kchibisov
Copy link
Member Author

Want a badge in the README?

I don't like them, and we have that stated in Cargo.toml anyway.

Note that we can't guarantee this for downstream users without all of our dependencies also setting the field to the same version or lower!

We actually do guarantee, given that CI builds winit now with MSRV and it won't pass otherwise.

@madsmtm
Copy link
Member

madsmtm commented Jul 27, 2022

We actually do guarantee, given that CI builds winit now with MSRV and it won't pass otherwise.

True, but that only says something about a version of Rust that winit currently builds with right now, not what it is guaranteed to build with in the future.

As an example, let's take a hypothetical winit v0.27.1, which has the semver guarantee "builds with Rust 1.57". This version depends on the crate instant v0.1, which doesn't specify a rust-version, but it builds on Rust 1.57, so all is fine.
Later on (could theoretically be years from now), the instant crate is improved, using some new Rust > 1.57 feature, and the instant crate includes this in a patch version, which they can since they don't provide any guarantees about their MSRV.
Aaand suddenly the guarantee that winit v0.27.1 builds with Rust 1.57 no longer holds!

@madsmtm
Copy link
Member

madsmtm commented Jul 27, 2022

My point is, I'm fine with stating what our current MSRV is, but we must take care not to make it part of our SemVer contract, since we can't uphold such a guarantee.

@kchibisov
Copy link
Member Author

Yes, but applications can downgrade the dependencies, that's the point. In general version change is a breaking change so library writers should have some sanity.

@madsmtm
Copy link
Member

madsmtm commented Jul 27, 2022

Yes, but applications can downgrade the dependencies, that's the point.

I think I understand the use-case better now, so we just state "there exists dependencies that can satisfy this specific version of winit, and can compile on Rust 1.57 - but you'll have to go looking for them yourself, since they may have been updated since".
Distributions can then use this information to say "we can safely specify a loose winit = "0.27" requirement, since a bump to 0.27.2 would not increase MSRV", while they'd still have to pin e.g. the instant = "=0.1.12" crate since that crate doesn't make any such guarantees.

Is that the idea? Or is there another use-case for distributions that I'm missing?

Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

Won't our CI break if one of our dependencies bumps their MSRV in a patch release (or minor release for post-1.0 versions)?

CONTRIBUTING.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member Author

Yes. And that we can ship alacritty to fedora/ubuntu since those update rust not that often.

@kchibisov
Copy link
Member Author

Won't our CI break if one of our dependencies bumps their MSRV in a patch release (or minor release for post-1.0 versions)?

I'm not entire sure, but we'll see how it works.

In general libraries must not break msrv in a not breaking change without a reason. At some point we can make all of them behave.

I've started that since it broke the ability to ship alacritty to any non rolling release distro, and I haven't noticed it due to not having automatic tests for msrv, etc. (But I was very happy having Mutex in static without any Lazy craft).

@maroider
Copy link
Member

I've started that since it broke the ability to ship alacritty to any non rolling release distro, and I haven't noticed it due to not having automatic tests for msrv, etc.

Do we have some sort of list of distros we care about in this regard, since I assume we then want to keep ourselves compatible with the rustc versions they ship?

@kchibisov
Copy link
Member Author

Do we have some sort of list of distros we care about in this regard, since I assume we then want to keep ourselves compatible with the rustc versions they ship?

In general I'd suggest to target latest stable ubuntu rustc version. Every other distro is likely having newer version.

@kchibisov
Copy link
Member Author

In general, what would be the resolution here? I think that we can go ahead and keep it that way and see how often it'll break CI for us, but it should be pretty rare case.

@MarijnS95
Copy link
Member

@kchibisov I'd just go ahead with this. Any crate that breaks us / our CI at some point should simply result in a discussion whether the crate in question is at fault (and should realistically be yanked if MSRV is considered part of "breaking version bump" semver-wise), or our view of MSRV should be bumped and published. At the very least it won't hurt testing for it now.

.github/workflows/ci.yml Show resolved Hide resolved
src/platform/unix.rs Show resolved Hide resolved
src/platform_impl/ios/app_state.rs Outdated Show resolved Hide resolved
src/platform_impl/linux/mod.rs Show resolved Hide resolved
This should help with distributing apps using winit.

Fixes rust-windowing#1075.
@kchibisov kchibisov merged commit bf53700 into rust-windowing:master Jul 29, 2022
@kchibisov kchibisov deleted the rust-msrv branch July 29, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Minimum Supported Rust Version
5 participants