Android nightly builds are failing

[kchibisov]

@enfipy since you've bumped ndk recently could you take a look into CI failures winit is getting on android due to some deprecation warnings?

[enfipy]

@kchibisov android-ndk-rs moved to the new ndk-context solution, and it's currently WIP to support the winit use case of ndk.

There's an open issue for this: rust-mobile/ndk#211 (comment).

I think it's better to ping @MarijnS95 as he has a WIP branch with ANativeActivity support: rust-mobile/ndk@master...MarijnS95:anativeactivity-instance.

P.S: We can set ndk-glue to use the 0.6.0 version as a temporary workaround if needed, but perhaps we can move to ndk-context as soon as it will work?

[madsmtm]

We can fix this in the interim with just allowing the warnings, see #2197

[MarijnS95]

I thought about this just as the release got pushed out, hence intentionally didn't carry over the deprecation warning to the 0.4.1 and 0.5.1 backport release of ndk-glue (to leverage ndk-context and get back the cross-version compatibility), it probably shouldn't have been in 0.6.1 either at least not until a proper (breaking change) fixed 0.7 is available, since there's no alternative currently.

It's a bit of a tricky issue since I really wanted all the public static globals that winit needs access to (activity, looper, input queue, polling fds...), to be shoved into ndk-context as well (CC @dvc94ch).
Alternatively as per the branch linked above, give the user access to these only as owned/borrowed argument passed through the fn main(GlueInstance). While this doesn't solve the issue of crate versions getting out of sync/duplicated, if winit consumes the same GlueInstance a version mismatch will now be a compile error instead of a vague unwrap panic at runtime deep within the machinery of ndk-glue.

That however means winit needs a way to consume said GlueInstance (presumably in the builder) leading to more platform-specific new behaviour ¹. We should probably have a discussion here on winit before proceeding.

Footnotes

1. Just like the Suspend Resume logic, which I'd have loved to see generalized across all platforms. For example by removing the function to retrieve a RawWindowHandle from winit and instead providing it on every platform through a (renamed form of) Event::Resumed(RawWindowHandle). For a different issue though, if you think this is worth pursuing. ↩

[dvc94ch]

I don't think shoving it into ndk-context is a good idea, as those are coupled to ndk-glue while ndk-context is not.

[MarijnS95]

They can always be Optional, though I consider the move to ndk-context to be a fallback if the GlueInstance approach detailed above doesn't work out (ie. is too cumbersome). I don't think creating yet another "never sees a breaking release" crate just to house these additional statics next to ndk-context is a good idea.

[dvc94ch]

Not quite sure why winit can't just depend on ndk-glue? when ndk-glue updates winit updates. everyone else uses ndk-context so there isn't an issue.

[dvc94ch]

@kchibisov Sorry about the deprecation warning and breaking the builds. For now it would be sufficient to use #[allow(deprecated)].

@MarijnS95 took a quick look at your WIP branch. the changes seem sensible to me without requiring changes to ndk-context.

[madsmtm]

Sorry about the deprecation warning and breaking the builds

No worries from my part here, it is to be expected when using --deny warnings

[MarijnS95]

Not quite sure why winit can't just depend on ndk-glue? when ndk-glue updates winit updates. everyone else uses ndk-context so there isn't an issue.

Not saying that it can't, that is in fact what the WIP branch still does. I however think that a compatibility table in the README and a crash at runtime if versions mismatch is not "out of the box" enough. This should either be a compiler error (what that WIP branch is all about, because types from different ndk-glue versions will be incompatible) or not occur at runtime at all.

No worries from my part here, it is to be expected when using --deny warnings

Fwiw I find it quite normal - expected even - to disallow warnings in CI. Most Rust projects managed to stay rather clean and don't have a massive "issues to address" mountain to climb because of that 👍

[dvc94ch]

A compile time error would be nice yes. At the moment we have a deprecation warning only winit should silence, think that's quite neat. Even better would be using ndk-context/jni to retrieve the asset manager and remove fn native_activity() from ndk-glue. That way no one is tempted to do anything they shouldn't.

[MarijnS95]

That'd be neat, yeah :)

Though don't be fooled here, winit uses other static globals through poll_events(), input_queue(), native_window(), content_rect() :). That's what my branch aims to replace. I'll see if I can find some time to finish this approach, set up a PoC for winit and then we can discuss how to move ahead from there?

[dvc94ch]

I know winit uses those other globals, but they have a much lower abuse potential. (And they're actually there for winit to use)

I'll see if I can find some time to finish this approach, set up a PoC for winit and then we can discuss how to move ahead from there?

Yes, sounds good.

[MarijnS95]

Not afraid about "abuse potential", just an unsuspecting user running and initializing ndk-glue = "0.6" but pairing that with winit = "0.26" which uses 0.5 and seeing those nasty unwraps deep in the bowels of ndk-glue 🤞

[dvc94ch]

Well the skilled coder will quickly determine the cause and move on. The unskilled coder can hone their skills or give up and look for a different career