rust-mobile / android-activity

Glue for building Rust applications on Android with NativeActivity or GameActivity
238 stars 46 forks source link

Raw window handle strategy #141

Closed Vrixyz closed 10 months ago

Vrixyz commented 10 months ago

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

Following https://github.com/rust-mobile/ndk/pull/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 commented 10 months ago

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

rib commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 :tada:, 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 commented 10 months ago

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 commented 10 months ago

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.

https://github.com/rust-mobile/ndk/pull/434#issuecomment-1752089087

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 commented 10 months ago

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

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

rib commented 10 months ago

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.