Closed notgull closed 1 year ago
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.
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.
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.
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.
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.
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? :/
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.
The issue will happen right when you start building for android though, it won't allow you to do so.
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.
It seems to me that we need to have a two pronged MSRV policy:
Would this work for both cases?
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.
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.
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
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.
I'd still prefer to target more stable platforms with an msrv.
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')
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.
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.
(such as the warning for using unsafe blocks in unsafe functions which was removed from newer version of Clippy)
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...
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.
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.
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.
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.
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?
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
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: https://github.com/Smithay/wayland-rs/pull/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.
I can bump up to 1.66, 1.65 is a good msrv candidate and I'm not against using it.
For a long term policy, I like the idea of pinning desktop/web platforms to a distro and pinning mobile to stable.
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.
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?
Earlier,
android-activity
bumped its MSRV to 1.68.1 in https://github.com/rust-mobile/android-activity/pull/103. This means thatwinit
's MSRV is now effectively 1.68.1 as well. While discussing this in chat, it occurred to me thatwinit
, @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 https://github.com/rust-windowing/winit/issues/1075.