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

Raw window handle strategy #141

Closed
Vrixyz opened this issue Oct 22, 2023 · 9 comments · Fixed by #142
Closed

Raw window handle strategy #141

Vrixyz opened this issue Oct 22, 2023 · 9 comments · Fixed by #142

Comments

@Vrixyz
Copy link
Contributor

Vrixyz commented Oct 22, 2023

Hello, I'm attempting to update our dependencies (from bevy, through winit).

Following rust-mobile/ndk#434, It looks like I cannot set a feature to use a specific raw-window-handle implementation (0.5). Would you be interested in supporting it ? How can we move forward ?

@rib
Copy link
Collaborator

rib commented Oct 23, 2023

Ah, right, yes we should add some corresponding features in android-activity.

@rib
Copy link
Collaborator

rib commented Oct 23, 2023

Actually I'm not entirely sure we need to do anything special in this crate?

There is a general issue that Cargo features are additive and I think perhaps the Winit and ndk crates shouldn't have provided any default for enabling rwh_06 but if you specifically want to use 0.4 or 0.5 then I guess it should be possible to specify these features via your own explicit winit or ndk dependency?

This crate doesn't currently have any opinion about raw-window-handle - it's just an example of a crate that doesn't disable the default features for the ndk crate - but there could be numerous other crates that may depend on the ndk crate with default features so I'd guess if if the aim is to be able to build without pulling in raw-window-handle 0.6 then we will have a whack-a-mole problem where we have to stop everyone from depending on the ndk crate with default features?

We could perhaps explicitly pass default-features = false for our ndk dependency here.

Since we don't currently re-export the ndk crate then I guess we might be able to get away with that, without that being a semver incompatible change for this crate.

That would then increase the chances that you might be able to build without pulling in rwh_06.

Or maybe I'm missing some other details here?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Oct 24, 2023

I think your analysis is correct, https://github.com/rust-mobile/ndk/blob/f414cabc12211ebcba4edb3da90950967d4a1933/ndk/Cargo.toml#L16 is problematic because of implicit dependency. It forces me to pass default-features = false then features=["rwh_05"].

In the end it's a matter of how strongly winit and ndk wants to promote the latest dependency for raw_window_handle.

It might be worth to open an issue on their side to bring your point about the "whack a mole".

we might be able to get away with [pass default-features = false for our ndk dependency here], without that being a semver incompatible change for this crate.

I think it's correct for most, but if a crate were to not explicitly pass the feature to ndk, it would break them, but in practice it's probably not a lot (because I'm assuming dependants have winit in their dependencies, which will handle that ndk/rwh feature.). Still, my point is it's theoretically possible?

@rib
Copy link
Collaborator

rib commented Oct 24, 2023

It forces me to pass default-features = false then features=["rwh_05"]

My current assumption/understanding is that these features are additive, so technically you shouldn't need to pass default-features = false unless these features are actually mutually exclusive. Not passing in default-features = false should just mean that you end up with the redundant rwh_06 which you don't currently need.

If that assumption is wrong though and these features really are mutually exclusive then I think there's probably a more serious concern here and the default should almost certainly be removed from the ndk/winit crates.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Oct 24, 2023

Thanks for the correction! I just tested so I confirm these features are additive and not mutually exclusive.

So yeah this issue is less blocking than I thought 🎉, still might be an improvement for compile times and general understanding of the dependency tree (cargo tree -i raw-window-handle led me to this (android-activity) crate bringing a unneeded version of raw-window-handle).

@rib
Copy link
Collaborator

rib commented Oct 25, 2023

Okey, good to confirm that they are additive!

Yeah it's not ideal that it's awkward to avoid the rwh_06 feature if you don't need it.

Since we currently only have quite a minimal exposure of the ndk API in the public API of android-activity I'm guessing we could switch to using default-features = false and maybe we can keep this issue open as a reminder to look at that.

@MarijnS95
Copy link
Member

MarijnS95 commented Oct 25, 2023

My current assumption/understanding is that these features are additive, so technically you shouldn't need to pass default-features = false unless these features are actually mutually exclusive. Not passing in default-features = false should just mean that you end up with the redundant rwh_06 which you don't currently need.

If that assumption is wrong though and these features really are mutually exclusive then I think there's probably a more serious concern here and the default should almost certainly be removed from the ndk/winit crates.

rust-mobile/ndk#434 (comment)

Yeah it's not ideal that it's awkward to avoid the rwh_06 feature if you don't need it.

Shall we drop this after all? It wasn't optional before and I've kept it that way for 0.8.0 now (and it's only noticed because the ecosystem is still behind on 0.5 while I made sure to set 0.6 as the default to nudge towards upgrading).

Vrixyz added a commit to Vrixyz/android-activity that referenced this issue Oct 25, 2023
The `ndk` crate enables `raw-window-handle 0.6` by default (because of rust-mobile/ndk#434 (comment)) which might not be used by consumers of the `android-activity` crate at all, or might (still) be a mismatching version. Even if the `rwh_0x` features are additive, figuring that out leads to cryptic errors and it is best to turn off these defaults completely and leave it to the user to turn it back on in their own `[dependencies]` section if desired.
@Vrixyz
Copy link
Contributor Author

Vrixyz commented Oct 25, 2023

I think I like the default to "latest supported window-raw-handle":

  • it lowers the barrier of tweaks to do for a first setup (or "good defaults)
  • nudges towards upgrading

I agree on your point it was noticed because the ecosystem is behind on 0.5.

@rib
Copy link
Collaborator

rib commented Oct 25, 2023

I'd guess that maybe it would have been ok to not provide a default from the pov that most things have no opinion on which feature is enabled and then it could have been left up to the middleware glue crates to make the choice according to what they need (e.g. if you know what version of wgpu you're using and what version of raw-window-handle it needs you can then enable the corresponding rwh_ feature to be able to pass handles from winit to wgpu)

but a default of rwh_06 doesn't seem too bad if we will anyway trend towards everything switching over to raw-window-handle 0.6.

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 a pull request may close this issue.

3 participants