1581628 - android wrench jobs fail with newer NDKs

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Nathan Froyd [:froydnj]]

Trying to build all relevant jobs with the r20 NDK, I ran into:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b611587749a5b6dd474b0a34a2340af8f65429&selectedJob=266916868

[task 2019-09-16T19:44:16.505Z] + ../cargo-apk/bin/cargo-apk build --frozen --verbose
[task 2019-09-16T19:44:17.203Z] Compiling android_native_app_glue.c
[task 2019-09-16T19:44:17.203Z] error: could not execute process `/builds/worker/fetches/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc /builds/worker/fetches/android-ndk/sources/android/native_app_glue/android_native_app_glue.c -c -o /builds/worker/checkouts/gecko/gfx/wr/target/android-artifacts/armv7-linux-androideabi/android_native_app_glue.o --sysroot /builds/worker/fetches/android-ndk/sysroot -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/arm-linux-androideabi` (never executed)

which looks like something inside cargo-apk is trying to use the NDK's gcc, which no longer exists as of r19. The release notes for r19 say that the wrapper was not really functional in the first place.

Something needs to be changed to use clang out of the NDK, but I'm not sure what.

Comment 1

[Nathan Froyd [:froydnj]]

I don't understand all the wrench vendoring processes; jrmuizel or jnicol, gw said one of you might be able to take a look at this while kats is out? This is the last piece blocking the NDK r20 landing, which is blocking other things like a C++17 upgrade, so I'd like to get this resolved soon-ish.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I can take a look.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I believe upstream https://github.com/rust-windowing/android-rs-glue/ has support for newer ndks now. https://github.com/staktrace/android-rs-glue/tree/mozilla_master may need to be rebased on top of the latest upstream and that might be sufficient.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Here's a try push just using straight upstream android-rs-glue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9de5eb2c7994853febf9d46fa698ca0409453c3

Comment 5

[Jeff Muizelaar [:jrmuizel]]

or this https://treeherder.mozilla.org/#/jobs?repo=try&revision=0767a58be1894156efd75d4d486c6f1913137350

Comment 6

[Jeff Muizelaar [:jrmuizel]]

It fails with:

[task 2019-09-18T00:56:23.928Z] keytool error: java.io.FileNotFoundException: /builds/worker/.android/debug.keystore (No such file or directory)
[task 2019-09-18T00:56:23.928Z] java.io.FileNotFoundException: /builds/worker/.android/debug.keystore (No such file or directory)
[task 2019-09-18T00:56:23.928Z] 	at java.io.FileOutputStream.open0(Native Method)
[task 2019-09-18T00:56:23.928Z] 	at java.io.FileOutputStream.open(FileOutputStream.java:270)
[task 2019-09-18T00:56:23.928Z] 	at java.io.FileOutputStream.<init>(FileOutputStream.java:213)
[task 2019-09-18T00:56:23.928Z] 	at java.io.FileOutputStream.<init>(FileOutputStream.java:101)
[task 2019-09-18T00:56:23.928Z] 	at sun.security.tools.keytool.Main.doCommands(Main.java:1194)
[task 2019-09-18T00:56:23.928Z] 	at sun.security.tools.keytool.Main.run(Main.java:366)
[task 2019-09-18T00:56:23.928Z] 	at sun.security.tools.keytool.Main.main(Main.java:359)
[task 2019-09-18T00:56:23.941Z] error: process didn't exit successfully: `/usr/bin/keytool -genkey -v -keystore /builds/worker/.android/debug.keystore -storepass android -alias androidebugkey -keypass android -dname 'CN=Android Debug,O=Android,C=US' -keyalg RSA -keysize 2048 -validity 10000` (exit code: 1)

Comment 7

[Jeff Muizelaar [:jrmuizel]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f48375adda22b99772ba3a2d440cd34a8d847ded&selectedJob=267163672 succeeds.

Comment 8

[Jamie Nicol [:jnicol]]

This is great that we can use upstream cargo-apk.

I tried actually running wrench and encountered a few issues in testing/mozharness/scripts/android_wrench.py:

• The app output location has changed: (from gfx/wr/target/android-artifacts/app/build/outputs/apk/app-debug.apk to gfx/wr/target/android-artifacts/app/build/outputs/debug/apk/wrench.apk)
• The activity name we need to launch has changed (from rust.wrench.MainActivity to android.app.NativeActivity)

And then I got a permissions warning which I couldn't immediately figure out how to fix:

Unable to grant runtime permissions to org.mozilla.wrench due to args: /home/jamie/.wrench/android-sdk-linux/platform-tools/adb wait-for-device shell pm grant org.mozilla.wrench android.permission.WRITE_EXTERNAL_STORAGE; echo adb_returncode=$?, exitcode: 1, stdout: Operation not allowed: java.lang.SecurityException: Package org.mozilla.wrench has not requested permission android.permission.WRITE_EXTERNAL_STORAGE

and then it timed out trying to launch.

Comment 9

[Jamie Nicol [:jnicol]]

Assigning to you to clear from my triage inbox, but feel free to reassign it to me if you'd rather I take it from here.

Comment 10

[Jamie Nicol [:jnicol]]

Taking this off Jeff because I've been making a bit of progress with it.

Continuing on from comment 8:

The "timeout" and the permission warning are linked. With upstream cargo-apk we now need to explicitly request permissions in the manifest. This can be done in Cargo.toml. Some of the permissions we try to request aren't necessary, but we need READ_STORAGE to parse the command line args file. Without this we error when trying to open the args file, print the help message and quit. Then the script times out waiting for output that will never happen.

With the permissions fixed we hit a crash when running:

thread '<unnamed>' panicked at 'assertion failed: (*self.data.get()).is_none()', src/libstd/sync/mpsc/oneshot.rs:90:13

I don't understand the cause of this, but I bisected cargo-apk, and found this commit to be responsible. I will investigate the reason for that further, but for the time being we could continue to use a custom branch which has that commit reverted.

With that, debug tests pass. However, there seems to be an issue with the release builds, that the apk is not being signed properly so cannot be installed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=72f13b96e27fe0bf5f61970ca8738b3531ca237e

Comment 11

[Jamie Nicol [:jnicol]]

(In reply to Jamie Nicol [:jnicol] from comment #10)

I will investigate the reason for that further, but for the time being we could continue to use a custom branch which has that commit reverted.

I should have just searched the github issues and saved myself a bit of time: https://github.com/rust-windowing/android-rs-glue/issues/239

It is due to using a locally installed version of cargo-apk, and the crates.io version of android-glue, which have mismatched enums due to the commit in comment 10. I assume it should be possible to override the version of android_glue we link against, but it might just be easiest to wait for them to publish the new version to crates.io (depending on how long that takes, should be soon hopefully)

Comment 12

[Jamie Nicol [:jnicol]]

The signing issue is because upstream cargo-apk now automatically takes care of signing the build for us. I assume android doesn't like it when we then try to sign it again. Removing our manual signing fixes the problem. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d65d23da954aeade26938a86a848d548634162f4

This is why Jeff had to add mkdir /builds/worker/.android to the build script in comment 7. Because it fails when trying to sign if ~/.android doesn't exist. Keeping the mkdir in our script is probably okay but I might file an upstream bug about this too.

Comment 13

[Jamie Nicol [:jnicol]]

Green run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2146cac42b681994da6d69202a7dc40b82294f58 \o/

I've added a patch section to Cargo.toml to ensure that winit is built with a suitable version of android_glue. We can remove this when a new version is published to crates.io.

Will clean up these patches and put it up for review.

Comment 14

[Jamie Nicol [:jnicol]]

Attachment: Bug 1581628 - Update wrench jobs to work with NDK r20. r?jrmuizel

To support NDK r20, wrench needs to be built with a more recent, upstream
(though still unpublished) version of cargo-apk. This has some consequences
which have been adjusted for:

• Gradle is no longer required to build wrench.
• The output apk file paths have changed.
• The apks are now signed automatically.
• The default activity name has changed.
• Android permissions must be explicitly requested.
• We must ensure winit is built with a matching version of android_glue.

Comment 15

[Jamie Nicol [:jnicol]]

The above patch will need to be landed in conjunction with bug 1577220, otherwise it'll be a bad time.

I assume I can probably make that happen in phabricator somehow. will have a look.

Comment 16

[Jamie Nicol [:jnicol]]

Nathan, do you want to land this in conjunction with bug 1577220?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Jamie Nicol [:jnicol] from comment #16)

Nathan, do you want to land this in conjunction with bug 1577220?

Yes, I will do that, thank you so much for tracking this down. For some reason I thought this could be landed independently of the r20 work, but on reflection, that doesn't make a whole lot of sense. :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34ab960f8d33
Update wrench jobs to work with NDK r20. r=jrmuizel

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/34ab960f8d33