Add ndk-context. #223

dvc94ch · 2022-02-07T10:10:23Z

Closes #211

ndk-context/src/lib.rs

zmerp · 2022-02-07T16:10:19Z

As this PR stands, winit will not be able to replace ndk-glue with ndk-context, but maybe this is intended. Besides that this should be enough for many other creates, including openxrs.

dvc94ch · 2022-02-07T17:00:01Z

As this PR stands, winit will not be able to replace ndk-glue with ndk-context, but maybe this is intended.

This is indeed intended. winit (and presumably a game engine) is highly tied to the runtime and cargo-apk. I think this should be enough for most crates wanting to add some android support. If there are crates for which this isn't sufficient we can collect the exact reasons why not in an issue and then see if we have to take further steps.

dvc94ch · 2022-02-12T20:30:51Z

Ping @MarijnS95

dvc94ch · 2022-02-13T19:29:03Z

@msiglreith looks like @MarijnS95 is on vaccation or something. what are your thoughts on this PR?

msiglreith

API-wise it looks good to me. The crate should be listed in the README and probably also include a section regarding ndk-context vs ndk-glue somewhere

dvc94ch · 2022-02-14T11:40:55Z

So wondering if we shoud remove the fn native_activity() from ndk-glue. winit is using it to access the AssetManager. I think there is probably a way to get an AssetManager using only ndk-context. Maybe marking it as deprecated would get the message accross to people to stop using it.

dvc94ch · 2022-02-14T11:46:40Z

Yep, we should deprecate it [0] shows how to access it using the jni.

[0] https://stackoverflow.com/questions/22436259/android-ndk-why-is-aassetmanager-open-returning-null/22436260#22436260

MarijnS95 · 2022-02-14T14:00:22Z

looks like @MarijnS95 is on vaccation or something

Not anymore 😉. Taking a (still rather short) break from open source was well needed.

I thought the idea was to remove all statics from ndk-glue and shove them into ndk-context (with the premise that we never do any breaking changes with breaking updates). It is yet unclear to me if it's right to assume that all points of contention over these duplicate static variables originate from purely needing the vm and/or activity? Looks like "deprecating" native_activity() is a bit too harsh?

ndk-glue/src/lib.rs

ndk-context/src/lib.rs

MarijnS95 · 2022-02-14T14:16:20Z

So wondering if we shoud remove the fn native_activity() from ndk-glue. winit is using it to access the AssetManager. I think there is probably a way to get an AssetManager using only ndk-context. Maybe marking it as deprecated would get the message accross to people to stop using it.

Regarding this, since there don't seem to be many users of it after switching everyone over to ndk-context, perhaps it's still worth pushing through my idea (#211 (comment)) of passing the ANativeActivity object into fn main() and removing static-global access to it entirely? That way you eradicate this problem entirely without working around a deprecation warning in certain cases. Where needed, a version mismatch results in a compile error (and we can recommend raw pointer casts if a version bump is tricky).

Maybe good idea to document for most of these popular crates - https://crates.io/crates/ndk-glue/reverse_dependencies - what exactly they need from ndk-glue

dvc94ch · 2022-02-14T15:06:33Z

Yes, think removing the static NATIVE_ACTIVITY is the right thing to do (and complementary to ndk-context). Although not sure about removing NATIVE_WINDOW, INPUT_QUEUE and CONTENT_RECT.

So in general are you ok with the ndk-context approach to partly solving this problem?

This reverts commit bb117f0.

dvc94ch · 2022-02-14T15:18:47Z

So removed the NATIVE_ACTIVITY global as I think this is one we agree on and is worth doing.

MarijnS95 · 2022-02-14T15:32:16Z

https://github.com/MarijnS95/android-ndk-rs/compare/anativeactivity-instance @dvc94ch This is my draft idea for removing fallible static globals entirely, I can open a PR and we can iterate on it in parallel with your changes.

dvc94ch · 2022-02-14T15:44:44Z

@MarijnS95 that looks good, I think it's an improvement for some use cases. But in general I still think we should try to discourage the use of ndk-glue unless it's absolutely necessary. If you open a PR we can merge it and I'll change jni-audio back to using ndk-context.

ndk-context/src/lib.rs

MarijnS95 · 2022-02-14T15:52:15Z

But in general I still think we should try to discourage the use of ndk-glue unless it's absolutely necessary.

I think both PRs are a great step in that direction 😉. Mine still has some open questions though:

what if a client application already uses the public instance field for its own cases?
static is gone now, but what if two different ndk-glue versions with incompatible GlueInstance layout try to cast and use the same pointer?

Will need some more thoughts, I think I'll get this merged + released + backported ASAP, then PR the migration to most obvious crates and see where to move next.

ndk-context/src/lib.rs

MarijnS95 · 2022-02-14T16:21:37Z

Looks like GH comment notifications are all over the place. Are we removing native_activity(), deprecating it at least, or leaving it to a separate PR entirely?

(Sorry about closing this, ctrl+enter vs ctrl+shift+enter muscle memory isn't what it's supposed to be)

Co-authored-by: Marijn Suijten <marijns95@gmail.com>

MarijnS95 · 2022-02-14T16:24:30Z

Do we need a changelog for the new crate, and the changes listed in the ndk-glue crate? Then I think we are ready for merging and releasing. I'm preparing the necessary changes to backport this to older ndk-glue crates which are currently used by at least winit.

dvc94ch · 2022-02-14T16:27:24Z

Looks like GH comment notifications are all over the place. Are we removing native_activity(), deprecating it at least, or leaving it to a separate PR entirely?

Think we can hold off for with releasing until your PR lands, I removed my changes. Got to go afk for a while, will add a CHANGELOG afterwards.

MarijnS95 · 2022-02-14T16:42:12Z

We can do a non-breaking patch release (same for the backports) now with #[deprecated = "Don't use for `vm()` purposes"], preparing everyone for this future. Or without the deprecation, as my PR would be breaking anyway.

In order to show that [#223] solves our problem ([#211]) with multiple ndk-glue versions in tree, all having their own `static` globals, backport the `ndk-context` initialization to `ndk-glue 0.5` so that everyone on `winit 0.26` (pending a `winit 0.27` release) can be compatible with other crates using the Java VM and/or Android Context. These crates will need to migrate to `ndk-context` first though. [#211]: #211 [#223]: #223

zmerp · 2022-02-15T12:51:54Z

The crate oboe requires the VM and Context pointers, but also the sdk version. Is there a way of getting the sdk version from the context pointer?

In order to show that [#223] solves our problem ([#211]) with multiple ndk-glue versions in tree, all having their own `static` globals, backport the `ndk-context` initialization to `ndk-glue 0.5` so that everyone on `winit 0.26` (pending a `winit 0.27` release) can be compatible with other crates using the Java VM and/or Android Context. These crates will need to migrate to `ndk-context` first though. [#211]: #211 [#223]: #223

dvc94ch · 2022-02-15T12:57:15Z

yes, should be possible, although will require fiddling a bit with the jni.

https://developer.android.com/reference/android/os/Build

zmerp · 2022-02-15T13:03:11Z

@dvc94ch Oh that's great, thanks!

`ndk-glue` suffers one fatal flaw: it's "only" supposed to be used by the crate providing `fn main()` and only supposed to end up in the dependency graph once as it has `static` globals which get duplicated across versions. In the current case with `winit 0.26` still on `ndk-glue 0.5` but `app_dirs2` booted from `0.4` to `0.6` it'll always panic in `fn native_activity()` as the `static` globals on these versions are not initialized: app-dirs-rs#18 (comment) Introducing `ndk-context`: a crate that holds these `static`s, with the intention/premise to not see a breaking release /ever/ and make this a problem of the past. The crate is currently initialized with the VM and Android Context on `ndk-glue` 0.5.1 and 0.6.1 making it compatible with whatever is current, and the possibility for backporting to older `ndk-glue` versions too. See also: rust-mobile/ndk#211 rust-mobile/ndk#223

In order to show that [#223] solves our problem ([#211]) with multiple ndk-glue versions in tree, all having their own `static` globals, backport the `ndk-context` initialization to `ndk-glue 0.4` so that everyone on this older crate can be compatible with other crates using the Java VM and/or Android Context. These crates will need to migrate to `ndk-context` first though. [#211]: #211 [#223]: #223

`ndk-glue` suffers one fatal flaw: it's "only" supposed to be used by the crate providing `fn main()` and only supposed to end up in the dependency graph once as it has `static` globals which get duplicated across versions. In the current case with `winit 0.26` still on `ndk-glue 0.5` but `cpal` on `ndk-glue 0.6` it'll always panic in `fn native_activity()` as the `static` globals on this version is not initialized. Introducing `ndk-context`: a crate that holds these `static`s, with the intention/premise to not see a breaking release /ever/ and make this a problem of the past. The crate is currently initialized with the VM and Android Context on `ndk-glue` 0.5.1 and 0.6.1 (0.4.1 pending) making it compatible with whatever is current, and the possibility for backporting to older `ndk-glue` versions too. See also: rust-mobile/ndk#211 rust-mobile/ndk#223

In order to show that [#223] solves our problem ([#211]) with multiple ndk-glue versions in tree, all having their own `static` globals, backport the `ndk-context` initialization to `ndk-glue 0.4` so that everyone on this older crate can be compatible with other crates using the Java VM and/or Android Context. These crates will need to migrate to `ndk-context` first though. [#211]: #211 [#223]: #223

Add ndk-context.

8eb1cff

dvc94ch force-pushed the ndk-context branch from cff4320 to 8eb1cff Compare February 7, 2022 10:12

dvc94ch mentioned this pull request Feb 7, 2022

Runtime problems when including multiple versions of ndk-glue #211

Closed

dvc94ch requested a review from MarijnS95 February 7, 2022 10:44

rklaehn approved these changes Feb 7, 2022

View reviewed changes

zmerp suggested changes Feb 7, 2022

View reviewed changes

ndk-context/src/lib.rs Outdated Show resolved Hide resolved

Add doc comment for initialize_android_context.

be60894

dvc94ch force-pushed the ndk-context branch from a8eee84 to be60894 Compare February 12, 2022 23:26

msiglreith approved these changes Feb 14, 2022

View reviewed changes

Add links to README and some metadata.

622aa6a

Fix ndk-build doc link.

611a32e

Deprecate native_activity.

bb117f0

MarijnS95 reviewed Feb 14, 2022

View reviewed changes

ndk-glue/src/lib.rs Outdated Show resolved Hide resolved

ndk-context/src/lib.rs Outdated Show resolved Hide resolved

ndk-context/src/lib.rs Outdated Show resolved Hide resolved

dvc94ch added 2 commits February 14, 2022 16:08

Revert "Deprecate native_activity."

2df0733

This reverts commit bb117f0.

Address review comments.

d20c948

dvc94ch force-pushed the ndk-context branch from ad04c11 to d20c948 Compare February 14, 2022 15:48

MarijnS95 reviewed Feb 14, 2022

View reviewed changes

ndk-context/src/lib.rs Show resolved Hide resolved

Address review comments.

5417ab6

MarijnS95 reviewed Feb 14, 2022

View reviewed changes

ndk-context/src/lib.rs Outdated Show resolved Hide resolved

MarijnS95 closed this Feb 14, 2022

MarijnS95 reopened this Feb 14, 2022

Update ndk-context/src/lib.rs

f922d0c

Co-authored-by: Marijn Suijten <marijns95@gmail.com>

dvc94ch added 2 commits February 14, 2022 18:27

Add deprecation warning.

7776227

Prepare release.

9e17608

dvc94ch merged commit 21e78af into master Feb 14, 2022

dvc94ch deleted the ndk-context branch February 14, 2022 17:45

MarijnS95 mentioned this pull request Feb 15, 2022

ndk-glue: Initialize ndk-context on 0.5 stable releases #225

Merged

MarijnS95 mentioned this pull request Feb 15, 2022

android: Migrate from ndk-glue to ndk-context app-dirs-rs/app_dirs2#19

Merged

MarijnS95 mentioned this pull request Feb 15, 2022

ndk-glue: Initialize ndk-context on 0.4 stable releases #226

Merged

MarijnS95 mentioned this pull request Feb 15, 2022

android: Migrate from ndk-glue to ndk-context RustAudio/cpal#641

Merged

MarijnS95 mentioned this pull request May 28, 2022

ndk-glue: Drop deprecated annotation from native_activity() #280

Merged

MarijnS95 mentioned this pull request Jun 14, 2022

Consistently deliver a Resumed event on all platforms rust-windowing/winit#2331

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ndk-context. #223

Add ndk-context. #223

dvc94ch commented Feb 7, 2022

zmerp commented Feb 7, 2022 •

edited

Loading

dvc94ch commented Feb 7, 2022

dvc94ch commented Feb 12, 2022

dvc94ch commented Feb 13, 2022

msiglreith left a comment

dvc94ch commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 •

edited

Loading

dvc94ch commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 •

edited

Loading

MarijnS95 commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 •

edited

Loading

zmerp commented Feb 15, 2022

dvc94ch commented Feb 15, 2022

zmerp commented Feb 15, 2022

Add ndk-context. #223

Add ndk-context. #223

Conversation

dvc94ch commented Feb 7, 2022

zmerp commented Feb 7, 2022 • edited Loading

dvc94ch commented Feb 7, 2022

dvc94ch commented Feb 12, 2022

dvc94ch commented Feb 13, 2022

msiglreith left a comment

Choose a reason for hiding this comment

dvc94ch commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 • edited Loading

dvc94ch commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 • edited Loading

MarijnS95 commented Feb 14, 2022

dvc94ch commented Feb 14, 2022

MarijnS95 commented Feb 14, 2022 • edited Loading

zmerp commented Feb 15, 2022

dvc94ch commented Feb 15, 2022

zmerp commented Feb 15, 2022

zmerp commented Feb 7, 2022 •

edited

Loading

MarijnS95 commented Feb 14, 2022 •

edited

Loading

MarijnS95 commented Feb 14, 2022 •

edited

Loading

MarijnS95 commented Feb 14, 2022 •

edited

Loading