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

Disable ndk default features to remove raw-window-handle 0.6 #142

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Oct 25, 2023

Fixes #141

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.


I couldn't run the test because of that error, I'm on mac, not sure how my android dev setup is right now 😅
image

@MarijnS95
Copy link
Member

This error is triggered based on compile target, how should disabling features have an effect?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Oct 25, 2023

To be honest I don't know exactly how to test this crate (android-activity), I don't think my change is the root cause of my error, but rather something on my end, I just quickly got the PR running to start the discussion because I'm curious on how to address the issue.

I'm not sure how to answer your question, I guess I expected cargo test to select the correct target if only one were available, so I was surprised and unsure how to proceed.

@MarijnS95
Copy link
Member

@Vrixyz right, the way the description is set up (no context what you want to achieve, only something about an error) made me think this PR tried to address the error in the screenshot. Rather, you meant that that error shows up making you unable to test.

Please solve that by providing a commit message and PR description detailing what you want to achieve, because I'd approve the code changes in a pinch.

@MarijnS95
Copy link
Member

I just quickly got the PR running to start the discussion because I'm curious on how to address the issue.

In that case I've done you a solid and wrote a sensible description for you. Note that it was my decision to have rwh_06 in the default feature set for the ndk crate, so I believe I'm the right person to document why this makes sense.


There's likely no need to re-expose ndk-rwh_0x features, even though android-activity reexports the ndk crate. It'll be trivial enough for crate users to add this dependency to their own list, but if undesired I should remove this from the default feature set after all.

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 Vrixyz force-pushed the 141-ndk-no-default-features branch from 8caa2d9 to 77fb275 Compare October 25, 2023 19:31
@Vrixyz
Copy link
Contributor Author

Vrixyz commented Oct 25, 2023

Thanks! I edited the commit to reflect that, but I guess you can also squash and edit it from github UI, let me know if I can be of assistance further.

@rib
Copy link
Collaborator

rib commented Oct 25, 2023

Hey thanks for poking at this @Vrixyz

That error was essentially Cargo saying that the ndk-sys crate (same for the android-activity crate itself) needs to be compiled for Android whereas cargo test will by default try and build and test a crate for your host environment (macOS in this case).

Cargo lets you cross compile crates for other platforms by passing a --target like --target aarch64-linux-android or E.g.:

cargo build --target aarch64-linux-android --features=native-activity

but more generally for Rust Android development you also need to have downloaded the latest Android NDK from https://developer.android.com/ndk/downloads?authuser=3 and have pointed the ANDROID_NDK_ROOT environment variable at that.

I tend to use the cargo ndk crate to help deal with some of those Android cross-compilation details.

E.g. something like:

cargo install cargo-ndk
export ANDROID_NDK_ROOT=/path/to/ndk-r26
cargo ndk build --features=native-activity

Currently though we don't have any tests for this crate besides testing various apps or examples like these: https://github.com/rust-mobile/rust-android-examples

cargo test doesn't generally work for Android development since that's geared for trying to build and run tests on your host machine and android-activity is only designed to be usable on an Android device or emulator.

There are also some examples in this repo that each have a README that shows how they can be built and installed.

@MarijnS95 MarijnS95 changed the title ndk default features disabled Disable ndk default features to remove raw-window-handle 0.6 Oct 25, 2023
@MarijnS95 MarijnS95 merged commit 98aef99 into rust-mobile:main Oct 25, 2023
3 checks passed
@MarijnS95
Copy link
Member

MarijnS95 commented Oct 25, 2023

@Vrixyz thanks! I've gone ahead and squash-merged this while doing some patching (in particular, putting a title back into the commit message).

@MeoMix MeoMix mentioned this pull request Dec 20, 2023
rib added a commit that referenced this pull request Dec 20, 2023
Although this crate has some examples that depend on the ndk, they
aren't regular Cargo examples, they are completely standalone apps
that depend on dev-dependencies.
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 this pull request may close these issues.

Raw window handle strategy
3 participants