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

Revert 'Bump MSRV to 1.68' #104

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Revert 'Bump MSRV to 1.68' #104

merged 1 commit into from
Aug 7, 2023

Conversation

rib
Copy link
Collaborator

@rib rib commented Aug 4, 2023

This effectively reverts 66cfc68 (from #103) 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

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
@rib
Copy link
Collaborator Author

rib commented Aug 4, 2023

Cc: @MarijnS95 @kchibisov

Will just sit on the briefly in case we might end up updating Winit's CI re: rust-windowing/winit#3000

@MarijnS95
Copy link
Member

I still don't think we should purely blame winit for cargo-ndk dropping the workaround so early and quickly. Not everyone updates their Rust version on day one.

The clippy argument is irrelevant, the lints change all the time.

@rib
Copy link
Collaborator Author

rib commented Aug 5, 2023

The general issue is that Winit aims to support very old versions of Rust, essentially for the sake of being able to package Alacritty for Debian and that shouldn't affect Android development.

Ideally there should be some way to decouple the CI constraints for different platforms, especially from Linux.

Winit is where we have a conflict of interest currently because there is imho a good reason to raise the rust-version to 1.68 on Android but we are constrained by a policy that's based on supporting Linux packaging.

Regardless of whether its possible to workaround the Android linking issue it's reasonable to want to bump the rust version to the point where the linker workaround is no longer needed.

Sure it's not Winit's fault that cargo ndk dropped the linker workaround but the reality is that they did. For me I depend on cargo ndk 3 and its a practical pain to have to toggle installing cargo ndk 2 for being able to test old versions of Rust. I dont really want to have that extra hassle while maintaining android-activity.

I don't really want to argue about cargo ndk - it feels a bit like your antagonising here because the tools you maintain do still support the workaround.

I also don't want to argue about lints; sure the lint isn't important here, it's a minor paper cut at most.

The linker issue and lint paper cut are essentially just examples that help highlight that it's not ideal that Winit tests all backends with a compiler that is coming up for a year old, forcing a low common denominator that comes from needing to support Debian.

If it wasnt for the current conflict with Winit's CI then raising the rust-version to 1.68 here should have been reasonable.

1.68 is currently 5 months old and by the time winit 0.29 releases its likely that 1.68 will be 6 months old. By most standards 6 months should be more than enough time to update Rust for Android development.

@MarijnS95
Copy link
Member

6 months is what I consider acceptable for raising MSRV in the generic case.

@rib
Copy link
Collaborator Author

rib commented Aug 7, 2023

okey for now I'm reverting this - even though it's looking like we should be able to again reinstate this hopefully before winit 0.29 but I'd like to land the unicode character mapping PR ready for winit's next beta and not be blocked on this.

@rib rib merged commit 3464ba2 into main Aug 7, 2023
3 checks passed
@MarijnS95 MarijnS95 deleted the rib/pr/revert-msrv-bump-for-winit branch October 25, 2023 21:16
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.

2 participants