Runtime problems when including multiple versions of ndk-glue

[zmerp]

Having multiple versions of ndk-glue in the dependency tree leads to crashes because of uninitialized NATIVE_ACTIVITY.

Example: Including ndk-glue 0.5. When entering main(), NATIVE_ACTIVITY 0.5 gets initialized. Including Oboe crate that uses ndk-glue 0.4.0. When enumerating audio devices, i get:

thread 'tokio-runtime-worker' panicked at 'called `Option::unwrap()` on a `None` value', /Users/ric/.cargo/registry/src/github.com-1ecc6299db9ec823/ndk-glue-0.4.0/src/lib.rs:59:39

I tried using [patch.crates-io] but that does not seem to work.

This bug might be related to using resolver = "2" as required by wgpu.

[MarijnS95]

That's not really a bug, Just How Things Work™.

That said, winit added a match table to prevent this (and we should have a comment or issue around here documenting the exact same crash), so this is a very real problem.

[patch.crates-io] only works when the versions are the same, allowing you to make small local modifications. It's impossible to use this to replace ndk-glue 0.4.0 with ndk-glue 0.5.0, since they're considered different crates in the same tree.

What we can however do is the semver trick: we release one final patch-release of ndk-glue 0.4.x, reexporting the functions from ndk-glue 0.5.x (and removing the statics): that causes duplication on the crate graph but makes sure that - assuming everyone sets the minimum patch version correctly in their toml or lock file - both are interoperable.

[zmerp]

I thought of another hack: make NATIVE_ACTIVITY public (or create a unsafe setter). This way we can import multiple ndk-glue versions and for each additional one set the the native activity manually.

[zmerp]

I was thinking about something like this:

Cargo.toml:

ndk-glue = "0.5"
ndk-glue-4 = { package = "ndk-glue", version = "0.4" }

main:

let native_activity_ptr = ndk_glue::native_activity().ptr().as_ptr();
unsafe { ndk_glue_4::set_native_activity(native_activity_ptr as _) };

We can add a patch release for v0.4 and v0.5 with this new function, I think it's cleaner. I can make a PR if you want.

[MarijnS95]

I don't really like that approach as it's quite unsafe (ie. allows users to break things at will), requires careful consideration by the caller on what ndk-glue version the static is initialized versus what version(s) use it, and will also need to be updated.

While NATIVE_ACTIVITY itself won't change during the runtime of the program (except that we should drop it at the end, see #160), there are (at the time of writing) four more statics that need all of the above including updates during the runtime of the program while Android fires callbacks.

As such I don't think the manual approach scales.

Re-exporting pub accessor functions is tricky too, as we'd need to perform those pointer casts to wrapper-structs from different ndk versions while still holding on to the same lock guard. Since the layout between all the versions is the same we could transmute the lock guards to make them hold a different ndk version's newtype/wrapper struct.
However, this approach removes all the thinking for the user - it'll work out of the box if we implement it correctly.

In the end it's probably best to just update oboe to a newer ndk-glue?

[zmerp]

In the end it's probably best to just update oboe to a newer ndk-glue?

I already sent a PR for oboe some days ago, but your last release made it already deprecated. openxrs also breaks if I update ndk-glue to 0.6 in my project. This is not sustainable. If we can't introduce cross-version initialization functions we should at least make so ndk-glue does not need to be updated so often. The ndk crate dependency should be removed from ndk-glue, and maybe move some functionality inside the ndk crate.

Maybe we need a ndk-ptrs crate for storing the static pointers. The crate should have no dependencies (apart for lazy_static), and the pointers can be untyped (c_void).

[enfipy]

In the end it's probably best to just update oboe to a newer ndk-glue?

@MarijnS95 In the end, is the best approach is to set ndk = "*" and ndk-glue = "*" in libraries that use android-ndk? It's such a pain to bump every library to use the newest version of ndk-glue.

[repi]

In the end, is the best approach is to set ndk = "" and ndk-glue = "" in libraries that use android-ndk? It's such a pain to bump every library to use the newest version of ndk-glue.

Don't think you can publish a crate with wildcard version? Believe it only works for local crates

[MarijnS95]

but your last release made it already deprecated. openxrs also breaks if I update ndk-glue to 0.6 in my project. This is not sustainable.

Indeed, and increased use of ndk-glue lately shows.

The ndk crate dependency should be removed from ndk-glue

Not easily possible since ndk-glue has to know of and call/use some of the functions/types from the ndk. We have better solutions.

and maybe move some functionality inside the ndk crate.

Nah, I think the ndk crate should purely hold the (safe) abstractions to the NDK. ndk-glue is "just" a helper (like Android's "official" glue layer) that users can leverage to simplify their Android setup. Either way it'll suffer from breaking changes to the ndk there too... Pick your poison.

Maybe we need a ndk-ptrs crate for storing the static pointers. The crate should have no dependencies (apart for lazy_static), and the pointers can be untyped (c_void).

I've been thinking about the exact same, we can even apply this retroactively to previous crate-releases as an additional patch. It'll just need a dependency on one of the crates that provides a Mutex/RWLock with a mapable inner value on the *LockGuard. That way we can convert from the pointer to the typed handle, and give the user what they asked for while held inside the *LockGuard.

@msiglreith @francesca64 @dvc94ch What are your thoughts on this? I/we can start working on a little proof of concept for this, having to make sure that we don't need to release any breaking changes to it for a good long while.

Alternatively we could do away with statics entirely and work with a heap allocation to which a pointer is stored in ANativeActivity::instance. Users would then have to pass this object around to get access to the NativeActivity and NativeWindow, and code would simply not compile if differing versions of ndk/ndk-glue are combined. Safer and more predictable, but the same dependency management hell for all the library crates involved.

[MarijnS95]

@MarijnS95 In the end, is the best approach is to set ndk = "*" and ndk-glue = "*" in libraries that use android-ndk? It's such a pain to bump every library to use the newest version of ndk-glue.

@enfipy As @repi mentioned such crates cannot be published... But it should be possible to publish them with a range of compatible crate revisions. Perhaps ndk-glue could set something (imaginary, I didn't compatibility-test this) like ndk = ">= 0.3, <= 0.6". Then we only need to bump these (usually only the upper bound) in a patch release.
(We can probably do this regardless of introducing an ndk-ptrs/statics crate mentioned above?)

Note that an upper bound is mandatory in my eyes to prevent a situation like gwihlidal/vk-mem-rs#54. It's much safer for us to validate a setup and push a patch release enlarging the available range of versions than to retroactively publish a new patch release with the upper bound fixed to the last working/compatible version, and yanking everything without that upper bound from crates.io.

The only thing I am not sure about is crate selection. I assume, but have not tested this, that if a crate were to select ndk = "0.5" any crate with ndk-glue with the imaginary ndk = ">= 0.3, <= 0.6" dependency from above would get just ndk 0.5.0? Though perhaps if cargo were to try and select the highest or the lowest (-Zminimal-versions) you could end up with duplicates on different versions after all. Then only Embark's cargo-deny could save you (or the aforementioned suggestion to remove statics from the crate altogether).

[msiglreith]

I'm currently leaning towards the separate ptr crate approach as it seems to minimize the risk of breaking changes compared to just using a version range for ndk.

Regarding the ANativeActivity::instance I'm not totally sure if I understand it in detail. In particular who is owning the memory storing the pointers? The ANativeActivity::instance is then only for internal usage?

[dvc94ch]

why do you have two versions in your dependency tree? ndk-glue only needs to be a dependency of the windowing crate you use (either winit, or your own), and you can reexport anything you need from the windowing crate. not sure why you have it more than once in your dependency graph.

[repi]

Here is a list of dependencies that we use that today all depend on ndk-glue for their Android version and as such run into this problem:

• app_dirs2
• cpal
• oboe
• webbrowser
• winit

actually cpal (v0.13.4) is extra bad at this as it depends on both ndk-glue v0.4.0 directly and then through oboe depends on and brings in ndk-glue v0.6.0.

That's probably a cause of recent Android crashes we've seen from this issue with multiple ndk crate dependencies (cc @hrydgard ). Probably easy enough to pin within cpal to a single version, but this overall issue is very error prone and dangerous.

[dvc94ch]

those libraries should probably expose an init function that takes a jnienv. although I can see that this is annoying yes.

[dvc94ch]

so how about a crate that looks like this:

pub struct AndroidContext {
    jni_env: std::ffi::c_void,
   context: std::ffi::c_void,
}
pub fn android_context() -> AndroidContext;

#[doc(hidden)]
pub unsafe intialize_android_context(ctx: AndroidContext);

ndk-glue can initialize it and all library crates can get the stuff they need from the ndk-context crate? If you spawn a new thread you can use the get_java_vm method and then attach to the new thread. this would mean that downstream crates don't depend on ndk-glue (which they shouldn't).