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

Adopt an official MSRV policy #3000

Closed
notgull opened this issue Aug 4, 2023 · 30 comments
Closed

Adopt an official MSRV policy #3000

notgull opened this issue Aug 4, 2023 · 30 comments

Comments

@notgull
Copy link
Member

notgull commented Aug 4, 2023

Earlier, android-activity bumped its MSRV to 1.68.1 in rust-mobile/android-activity#103. This means that winit's MSRV is now effectively 1.68.1 as well. While discussing this in chat, it occurred to me that winit, @rust-windowing and @rust-mobile not have an official MSRV policy.

I've heard conflicting answers from different maintainers about how they enforce the MSRV, ranging from a seven month rolling release window to pinning to various Linux distro's supported versions. I think that a policy should be standardized not only in winit, but also across all repos in @rust-windowing and @rust-mobile. This would prevent the situation linked above from happening.

Edit by @madsmtm to make it visible for newcomers: This has been discussed before in #1075.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

One notable limitation of cargo's rust-version support is that you can't specify different versions for different targets afaik.

This recent attempt to bump to 1.68 for Android is a pretty good example I think of where it's not really reasonable to impose a single strict policy that affects all backends for Winit because we have good technical reasons why it makes sense on Android to set the MSRV to >= 1.68.

For Android specifically, Rust updated the Android NDK version that they build against for 1.68 which fixed an annoying issue with the standard library being linked against libgcc which is no longer included in recent NDK toolchains. This means that any Android project building with < 1.68 either needs to use a very old NDK toolchain (with different issues) or they need to implement a workaround to link in libunwind as if it were libgcc.

Personally I depend on cargo ndk 3+ across all my Android projects and also for work, and as it happens cargo ndk removed support for the linker workaround now that it shouldn't be needed. In this case I literally can't build on Android with < 1.68 unless I install an older version of cargo ndk which brings with it another set of problems.

So in practice, we're kind of held hostage by Winit's MSRV policy and we're only maintaining the ability to compile with 1.64 in CI based on an old version of cargo ndk that developers wouldn't really use otherwise.

It's a completely artificial constraint that makes it more awkward to maintain android-activity basically because of how Debian is maintained - which shouldn't be relevant for Android development.

Proposal

I think a reasonable solution that works with the limitation that we can only have one rust-version, and should be straight-forward to maintain, would be for Winit's CI setup to be able to allow building different platforms against newer toolchains.

In ci.yml we can update the test matrix so we take 1.64 out of the base rust_version array and add an include section for the matrix to set the minimum test version for each platform instead.

Technically Winit would just keep the conservative rust-version for supporting Debian Sid on Linux but this rust-version wouldn't be enforced in CI on Android.

The final required rust-version from a developer's point of view is the maximum rust-version across all dependencies and on Android it's reasonable that it could require a more recent rust-version.

Winit's CI would be testing the rust-version that Cargo says is required for Android which would likely come from the android-activity + ndk crates.

@kchibisov
Copy link
Member

The problem comes from the Cargo.toml rust-version as well, I think the only way to make it work the way you want is to remove the rust-version out of the config and then in the CI builds simply try to build enforcing the versions where it's required maintaining the msrv matrix somewhere in the project we could refer to?

Like the main issue that we need one policy for desktop stuff, and completely different one for the mobile, because mobile requires more recent targets most of the time.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I think the rust-version in Cargo.toml can be the minimum version across all platforms (mostly likely whatever Linux needs).

Conceptually the way I see it is that Winit can have one MSRV (specified as rust-version in Cargo.toml) that is only authoritative over the winit crate itself and it defines the minimum version that might possibly be usable if all required dependencies also have the same minimum rust-version or lower.

But app developers or backend developers can choose to pull in deps that optionally require more recent versions of Rust which Winit doesn't need to block.

The rust-version only needs to be concerned with the crate itself that the version is attached too - Cargo will calculate the real minimum version based on the final crates that a project pulls in.

The missing bit I think is just that Winit's CI could take into account that on some platforms there can be other crates that actually require more recent versions of Rust and it shouldn't be assumed that the rust-version for Winit itself doesn't get constrained further by platform-specific dependencies.

Whether or not you can build Alacritty on Linux + Debian should come down to what version of Rust Cargo calculates for all your Alacritty dependencies when building for Linux. Since a crate can only have one rust-version you improve the chances of being able to support Linux + Debian by constraining the winit crate itself to 1.64 by making it possible to support 1.64 but that doesn't mean its necessary to impose the same constraint on the full graph of dependencies across all backends for Winit.

I think we can have the best of both, which is to keep the conservative rust-version of 1.64 for Winit, which can be maintained based on whatever Debian Sid requires but allow backends to pull in dependencies that can constraint the final rust-version more. You just want to avoid constraining it via deps on Linux. CI then just needs to take this into account by making it easy to adjust the version of Rust used to build/test different platforms according to the minimum value calculated by Cargo - taking into account platform-specific dependencies.

@kchibisov
Copy link
Member

I think the rust-version in Cargo.toml can be the minimum version across all platforms (mostly likely whatever Linux needs).

Do you know that rust will abort compilation on mismatches? It'll also be hard for contributors to always pass a flag to disable the rust version checks and just 'build'.

The rust-version only needs to be concerned with the crate itself that the version is attached too - Cargo will calculate the real minimum version based on the final crates that a project pulls in.

The thing is about constraining all the deps as well, so users could know that they could build with the given msrv, that's the point why it's msrv, in the end.

Since a crate can only have one rust-version you improve the chances of being able to support Linux + Debian by constraining the winit crate itself to 1.64 by making it possible to support 1.64 but that doesn't mean its necessary to impose the same constraint on the full graph of dependencies across all backends for Winit.

rust-version enforces the requirements on all the deps and it's a must, otherwise it'll become unmaintainable, we must not pull anything that will break msrv.

I think we can have the best of both, which is to keep the conservative rust-version of 1.64 for Winit, which can be maintained based on whatever Debian Sid requires but allow backends to pull in dependencies that can constraint the final rust-version more. You just want to avoid constraining it via deps on Linux. CI then just needs to take this into account by making it easy to adjust the version of Rust used to build/test different platforms according to the minimum value calculated by Cargo - taking into account platform-specific dependencies.

The only way you can do that is if you force 'ignore rust version' checks on CI for Android and force all contributors to android to use ignore rust version flag for cargo, you can't do it differently with the current tooling rust offers for stuff like that.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I think the rust-version in Cargo.toml can be the minimum version across all platforms (mostly likely whatever Linux needs).

Do you know that rust will abort compilation on mismatches? It'll also be hard for contributors to always pass a flag to disable the rust version checks and just 'build'.

I had my fingers crossed that Cargo only traverses the optional dependencies that are enabled but haven't double checked that - is that really not what it does? :/

@rib
Copy link
Contributor

rib commented Aug 4, 2023

Testing this with winit having an optional dependency on android-activity that itself has a rust-version of 1.68 doesn't stop me building for Windows with 1.64, so yeah it seem like Cargo does the right thing and calculates the minimum version based on the crates you currently depend on.

@kchibisov
Copy link
Member

The issue will happen right when you start building for android though, it won't allow you to do so.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

That seems fine too - it's always fine to build with a newer version than the rust-version.

Ignoring the rust-version checks would be applicable the other way around if we set Winit's rust-version to e.g. 1.68 but if you knew it was actually possible to build with 1.64 for Debian then you could tell Cargo to ignore the version checks.

@notgull
Copy link
Member Author

notgull commented Aug 4, 2023

It seems to me that we need to have a two pronged MSRV policy:

  • For desktop and web platforms, the MSRV must not exceed the current Rust version available in Debian Testing or Fedora, whatever's lowest.
  • For mobile platforms, we need a more liberal MSRV, probably a six-month rolling window would work here?

Would this work for both cases?

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I would prefer to keep it simple, and not authoritative over platform-specific crates.

CI can test each platform according to the MSRV that Cargo reports for that platform - it doesn't need to be Winit's responsibility to impose what that should be.

The current MSRV policy for android-activity is to (at least) support stable releases over the last three months, but to only bump lazily when there's a reason.

With Android development there's nothing comparable to Linux distribution where we have to cater to people packaging for systems stuck using older toolchains. Even having to wait 6 months before we can use new Rust features is an unnecessary restriction in my view.

For a specific platform like Android I'd much rather start closer to bleeding edge and wait and see if there are specific challenges for Android developers with that policy - as opposed to starting with an unnecessarily conservative policy which just slows down being able to adopt new features but for no real reason.

I think Winit's MSRV policy could be updated to clarify that the rust-version for the winit crate is the minimum possible version, not considering optional dependencies.

Winit's policy could also highlight the specific case that it does guarantee that optional dependencies for the Linux backend are expected to also support the specified rust-version so that Winit can be packaged by Linux distributions.

Essentially the MSRV policy would document that effective minimum rust-version is platform specific.

For other platforms though, I think Winit can leave it up to other crate maintainers to manage platform-specific MSRV policies for platform-specific crates in whatever way makes the most sense for those platforms - which may require newer versions of Rust for some backends, which Cargo will determine and report to users.

Maybe Winit's MSRV policy could link to some of the key platform-specific crates that are most likely to effectively determine the policy for that platform - such as the android-activity + ndk + jni crates on Android.

To allow for this I think Winit would just need to update how it tests different backends so that it uses the rust-version that Cargo reports as the minimum required version. Winit doesn't need to be opinionated here (except on Linux) it can just update its CI when there are MSRV bumps upstream for platform-specific crates.

@kchibisov
Copy link
Member

I think what you suggest is overly complicated though. I'd simply use msrv for desktop platforms (excluding orbital), and stable for the rest. Maybe removing the rust_version from inside the winit itself and solely relying on the CI.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

Yeah, could be simpler and reasonable to just test with stable+nightly for some backends besides Linux.

Dropping the rust-version from Winit considering how it makes sense for it to be platform-specific also sounds reasonable to me - and just relying on CI to ensure Winit builds for old versions of Rust on Linux

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I don't think I see this as a 'desktop' vs 'mobile' thing though - the outlier in all of this is basically just Linux + Debian I think.

@kchibisov
Copy link
Member

I'd still prefer to target more stable platforms with an msrv.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

In that case I'd still consider a broad split in CI which would treat Linux as a special case (e.g. 1yr rolling), then e.g. have a 6 month rolling msrv for "stable" platforms and platform-specific for others (just built with 'stable')

@kchibisov
Copy link
Member

I mean, desktop platforms don't change that often as Android/ios/wasm/redox, like I don't want to have a codebase mix different rust versions, simply because it's so annoying when you have changes from e.g. clippy which you can't fix at all, because some code uses e.g. new syntax, but others don't.

Let's just use stable for special platform which you're likely have a special code entry anyway.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I wouldn't think you'd want to run Clippy with each version - just the newest, since Rust is backwards compatible and you get newer rules including fixes for bad rules.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

(such as the warning for using unsafe blocks in unsafe functions which was removed from newer version of Clippy)

rib added a commit to rust-mobile/android-activity that referenced this issue Aug 4, 2023
This effectively reverts 66cfc68
and adds some comments explaining that we're currently blocked by
Winit's MSRV policy + CI from being able to increase our
rust-version.

This is a frustrating conflict that I hope can be addressed by
updating Winit's CI system to allow different platforms to require
more recent versions of Rust (which notably isn't in conflict with
setting a conservative rust-version in Winit for supporting Debian
on Linux)

This re-instates building android-activity with cargo-ndk 2 because
building on Android with 1.64 requires a linker workaround that's
not implemented in newer version of cargo-ndk.

This also reinstates the clippy false-negative warning suppression
for unsafe blocks. Again it's frustrating that we can't have good
things because of how Winit wants to support Debian which shouldn't
be relevant for Android development.

Here is an upstream issue to discuss a potential solution for this:
rust-windowing/winit#3000
@kchibisov
Copy link
Member

Well, I guess we could run clippy only from msrv, it just sometimes could suggest changes for the top level common API in a non-compatible with other platforms msrv way.

It's a bit complicated and it certainly happens pretty often...

@rib
Copy link
Contributor

rib commented Aug 4, 2023

I would think you want to just run clippy from stable, not msrv.

Clippy from msrv could be nearly a year old and newer versions of clippy should have improved lints, including fixes for bad lints that might be in the msrv version.

@kchibisov
Copy link
Member

The issue that stable clippy could suggest changes, which won't compile on msrv at all, so you run msrv clippy to not make it happen and fix the lints on demand.

However if we split CI, android's clippy run could trigger change in common code breaking linux due to msrv incompat.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

yeah, either way you get a potential whack a mole. I'd think the best set of lints should come from the newest version and then CI helps you catch changes that actually don't compile on older versions of Rust and you can add a clippy allow if needed.

@dhardy
Copy link
Contributor

dhardy commented Aug 4, 2023

The other problem with Clippy is new lints can be enabled a bit too "eagerly", only to be demoted later due to too many false positives. Only running Clippy on the MSRV helps avoid this.

@ids1024
Copy link
Member

ids1024 commented Aug 4, 2023

I think it makes sense in this case to have a higher MSRV for Android than for other platforms. And presumably the rust-version field of the dependency will make things behave as expected. So it's just a matter to adjusting CI to use that version for Android.

As for a broader MSRV policy, do any of the major projects that depend on Winit have a particular MSRV policy, or are Rust GUI toolkit and game engine libraries mostly just targeting the latest stable?

@dhardy
Copy link
Contributor

dhardy commented Aug 4, 2023

A few I could find:

Bevy: 1.70
egui: 1.65
Kas: 1.65 (policy is "any stable Rustc")
wgpu: 1.65

This may have something to do with GATs

@ids1024
Copy link
Member

ids1024 commented Aug 4, 2023

Makes sense. 1.65 is what I'd suggest as a current MSRV for Winit currently (well, as soon as it makes use of anything requiring that). I have a PR on wayland-rs requiring that, for GATs: Smithay/wayland-rs#640

So I think in the immediate future we want to target 1.68 for Android and 1.65 for everything else, but I'm not sure about a broader policy for MSRV bumps in the future.

@kchibisov
Copy link
Member

I can bump up to 1.66, 1.65 is a good msrv candidate and I'm not against using it.

@notgull
Copy link
Member Author

notgull commented Aug 4, 2023

For a long term policy, I like the idea of pinning desktop/web platforms to a distro and pinning mobile to stable.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

At least for mobile / android I'd be happy enough I think if winit went with stable and essentially wasn't opinionated about how android-activity updates its rust-version but technically it would be better if Winit tested with 1.68 if that's the actual minimum version - since there's a small risk otherwise of introducing code in the Android backend that requires > 1.68.

@rib
Copy link
Contributor

rib commented Aug 4, 2023

The other problem with Clippy is new lints can be enabled a bit too "eagerly", only to be demoted later due to too many false positives. Only running Clippy on the MSRV helps avoid this.

The reverse happens too with old false-negatives being removed in newer versions (e.g. there's an example of that my PR that bumped the MSRV for android-activity)

Though how does using an MSRV version avoid this problem - won't any particular version have some number of "new" lints that turn out to be badly designed and then that hopefully gets fixed in newer versions.

Rust doesn't have LTS releases so bad lints in an old release of Rust will essentially stay there wont they?

notgull added a commit that referenced this issue Aug 25, 2023
As discussed in #3000, this adopts an official MSRV policy for the
rust-windowing organization. It uses Debian Testing as the definition of
our support. In the future once winit becomes more stable we might want
to use Debian Stable instead, in order to allow Debian to use crates
like Alacritty in the stable channel.

Outstanding questions:

- What are the general thoughts on the organizational policy?
- Does this give us enough leeway to add new features?
- Are there any other Linux distros that we need to support?

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Aug 27, 2023
As discussed in #3000, this adopts an official MSRV policy for the
rust-windowing organization. It uses Debian Testing as the definition of
our support. In the future once winit becomes more stable we might want
to use Debian Stable instead, in order to allow Debian to use crates
like Alacritty in the stable channel.

Outstanding questions:

- What are the general thoughts on the organizational policy?
- Does this give us enough leeway to add new features?
- Are there any other Linux distros that we need to support?

Signed-off-by: John Nunley <dev@notgull.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants