New Android suppport #8288

larsbergstrom · 2015-10-31T20:04:34Z

No need to r+ urgently; I want to do a little bit more testing of the release build, but I'm hoping to land this bit (moving to a more sane build process) next week.

The new version of building an APK:

Removes the glutin-based APK builder from the link step
Adds a build.rs step to the build of the final Servo library that adds the native code required by glutin's android_rs_glue (e.g., ANativeActivity_onCreate definition)
Replaces the link step with a fake-ld.sh script that instead creates a libservo.so
Adds a new mach package step to build the APK that has some Rust code that builds the library from a set of in-tree build files

This setup fixes a number of problems:

We can use gdb, because we use ndk-build, which adds the .gdbserver info, plus we keep around all of the build files (also required by the ndk gdb)
We can add more Java code & hooks to handle Android intents
We no longer have any git submodules or the awkward two-step build with android-rs-glue

Many other setups were tried (and failed). The most obvious ones is building a libservo.so from a dylib target from the servo build on Android. This doesn't work because you can't have a different default lib target on one platform than others in Cargo, and you also can't pass it in from the commandline (e.g., --lib does not have a dylib arg). Additionally, if you don't go through the intermediate libservo.rlib step (which removes unused symbols), then you end up with a TON of missing symbols because our -sys crates are super sloppy about that. I spent a few weeks beginning to clean them up, but since it's something we can't easily enforce (and new -sys packages will have this problem, too, since it's only an issue with the Android loader), it made more sense to me to just have the build set up to discard those unused bits of code before they ever get to the linker, much less the loader.

eefriedman · 2015-10-31T20:28:30Z

Review status: 0 of 24 files reviewed at latest revision, 3 unresolved discussions.

^{.gitmodules, line 3 [r1] (raw file):}
Did you mean to delete this file?

^{components/servo/build.rs, line 11 [r1] (raw file):}
If might be a bit more clear to separate this into two functions: main() and android_main().

^{support/android/build-apk/Cargo.toml, line 15 [r1] (raw file):}
Leftover comment?

Comments from the review on Reviewable.io

larsbergstrom · 2015-10-31T23:28:57Z

Review status: 0 of 24 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/servo/build.rs, line 11 [r1] (raw file):}
Will do.

^{support/android/build-apk/Cargo.toml, line 15 [r1] (raw file):}
Thanks! I'll get it.

Comments from the review on Reviewable.io

larsbergstrom · 2015-10-31T23:29:32Z

@eefriedman re "Did you mean to delete this file?"

Yup, I'm remove the last git submodule in our tree - woo-hoo!

eefriedman · 2015-10-31T23:39:06Z

Err, I meant "Did you mean to delete this file, as opposed to leaving it in the tree as a zero-byte file".

larsbergstrom · 2015-10-31T23:40:25Z

Aha, then yes, I'll delete it - that was the intent :-)

eefriedman · 2015-11-01T05:02:22Z

From tidy: ./python/servo/post_build_commands.py:202: W391 blank line at end of file

larsbergstrom · 2015-11-01T18:18:49Z

BTW, definitely don't merge this; there's a small linker problem on Android release-only where it's stripping some of the critical public symbols (e.g., ANativeActivity_onCreate) that I still need to figure out :-/

eefriedman · 2015-11-02T20:26:07Z

components/servo/build.rs

+    // build.rs is not platform-specific, so we have to check the target here.
+    let target = env::var("TARGET").unwrap();
+    if !target.contains("android") {
+        return;


if target.contains("android") { android_main() }

eefriedman · 2015-11-02T20:27:31Z

(This looks fine in terms of style, but I don't know enough about how building for Android works to review this properly.)

larsbergstrom · 2015-11-03T18:34:40Z

@mbrubeck With that last fixup! (thanks to @tomaka for the hint!), we have working debug and release packages - I think this is ready to be reviewed and landed, when you have a chance.

mbrubeck · 2015-11-03T18:53:32Z

Reviewed 17 of 24 files at r1.
Review status: 13 of 25 files reviewed at latest revision, 5 unresolved discussions.

^{python/servo/post_build_commands.py, line 185 [r1] (raw file):}
It would be nice if this used the same logic as CommandBase.get_binary_path to determine the default if neither dev nor release is set.

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-03T19:05:10Z

Review status: 13 of 25 files reviewed at latest revision, 5 unresolved discussions.

^{python/servo/post_build_commands.py, line 185 [r1] (raw file):}
Hrm, I might have to pull out the code in get_binary_path into something separate that determines if it's dev or release, then, b/c I need to know for how to set the env vars. Does that sound OK?

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-03T19:06:58Z

Review status: 13 of 25 files reviewed at latest revision, 5 unresolved discussions.

^{components/servo/build.rs, line 14 [r3] (raw file):}
Fixed

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-03T19:07:09Z

Review status: 13 of 25 files reviewed at latest revision, 5 unresolved discussions.

^{.gitmodules, line 3 [r1] (raw file):}
Fixed; deleted now.

Comments from the review on Reviewable.io

mbrubeck · 2015-11-03T19:07:40Z

Reviewed 6 of 24 files at r1.
Review status: 18 of 25 files reviewed at latest revision, 6 unresolved discussions.

^{python/servo/post_build_commands.py, line 185 [r1] (raw file):}

Hrm, I might have to pull out the code in get_binary_path into something separate

Yes, that sounds good.

^{support/android/apk/src/rust/glutin/MainActivity.java, line 1 [r1] (raw file):}
Any reason for this package name in particular?

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-03T19:11:10Z

Review status: 18 of 25 files reviewed at latest revision, 6 unresolved discussions.

^{support/android/apk/src/rust/glutin/MainActivity.java, line 1 [r1] (raw file):}
Oh! That was just a carry-over. I should make it com.mozilla.servo like I did with the default android package name. Good catch :-)

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-03T19:53:11Z

@mbrubeck OK, set to go (modulo having to open a bunch of E-Easy issues )

mbrubeck · 2015-11-04T22:23:42Z

Reviewed 1 of 25 files at r1, 5 of 5 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

^{support/android/build-apk/src/main.rs, line 99 [r1] (raw file):}
File a follow-up to reduce the duplication of this value?

Comments from the review on Reviewable.io

mbrubeck · 2015-11-04T22:24:35Z

r=mbrubeck, needs squash

larsbergstrom · 2015-11-04T22:27:30Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

^{support/android/build-apk/src/main.rs, line 99 [r1] (raw file):}
Duplication of which value? The line you commented on is the "android-18", but I only see it there once. Or do you mean that the target SDK platform should be passed in to build-apk from mach?

Comments from the review on Reviewable.io


        New Android suppport

mbrubeck · 2015-11-04T22:29:41Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

^{support/android/build-apk/src/main.rs, line 99 [r1] (raw file):}
The string "android-18" is hard-coded in three different files. It'd be nice to have it configured in one place.

Comments from the review on Reviewable.io

larsbergstrom · 2015-11-04T22:30:19Z

@bors-servo r=mbrubeck

bors-servo · 2015-11-04T22:30:20Z

📌 Commit 17a6cb5 has been approved by mbrubeck

bors-servo · 2015-11-04T23:10:23Z

⌛ Testing commit 17a6cb5 with merge 1ed335a...


        Auto merge of #8288 - larsbergstrom:new_android_build, r=mbrubeck

@mbrubeck

New Android suppport r/f? @mbrubeck No need to r+ urgently; I want to do a little bit more testing of the release build, but I'm hoping to land this bit (moving to a more sane build process) next week. The new version of building an APK: 1) Removes the glutin-based APK builder from the link step 2) Adds a build.rs step to the build of the final Servo library that adds the native code required by glutin's android_rs_glue (e.g., `ANativeActivity_onCreate` definition) 3) Replaces the link step with a `fake-ld.sh` script that instead creates a libservo.so 4) Adds a new mach `package` step to build the APK that has some Rust code that builds the library from a set of in-tree build files This setup fixes a number of problems: 1) We can use gdb, because we use `ndk-build`, which adds the .gdbserver info, plus we keep around all of the build files (also required by the ndk gdb) 2) We can add more Java code & hooks to handle Android intents 3) We no longer have any git submodules or the awkward two-step build with android-rs-glue Many other setups were tried (and failed). The most obvious ones is building a libservo.so from a `dylib` target from the servo build on Android. This doesn't work because you can't have a different default lib target on one platform than others in Cargo, and you also can't pass it in from the commandline (e.g., --lib does not have a dylib arg). Additionally, if you don't go through the intermediate libservo.rlib step (which removes unused symbols), then you end up with a TON of missing symbols because our -sys crates are super sloppy about that. I spent a few weeks beginning to clean them up, but since it's something we can't easily enforce (and new -sys packages will have this problem, too, since it's only an issue with the Android loader), it made more sense to me to just have the build set up to discard those unused bits of code before they ever get to the linker, much less the loader.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8288)

bors-servo · 2015-11-04T23:50:17Z

💔 Test failed - linux-rel

jdm · 2015-11-04T23:51:05Z

@bors-servo: retry

#8311

bors-servo · 2015-11-05T00:21:49Z

⌛ Testing commit 17a6cb5 with merge 0699d38...


        Auto merge of #8288 - larsbergstrom:new_android_build, r=mbrubeck

@mbrubeck

New Android suppport r/f? @mbrubeck No need to r+ urgently; I want to do a little bit more testing of the release build, but I'm hoping to land this bit (moving to a more sane build process) next week. The new version of building an APK: 1) Removes the glutin-based APK builder from the link step 2) Adds a build.rs step to the build of the final Servo library that adds the native code required by glutin's android_rs_glue (e.g., `ANativeActivity_onCreate` definition) 3) Replaces the link step with a `fake-ld.sh` script that instead creates a libservo.so 4) Adds a new mach `package` step to build the APK that has some Rust code that builds the library from a set of in-tree build files This setup fixes a number of problems: 1) We can use gdb, because we use `ndk-build`, which adds the .gdbserver info, plus we keep around all of the build files (also required by the ndk gdb) 2) We can add more Java code & hooks to handle Android intents 3) We no longer have any git submodules or the awkward two-step build with android-rs-glue Many other setups were tried (and failed). The most obvious ones is building a libservo.so from a `dylib` target from the servo build on Android. This doesn't work because you can't have a different default lib target on one platform than others in Cargo, and you also can't pass it in from the commandline (e.g., --lib does not have a dylib arg). Additionally, if you don't go through the intermediate libservo.rlib step (which removes unused symbols), then you end up with a TON of missing symbols because our -sys crates are super sloppy about that. I spent a few weeks beginning to clean them up, but since it's something we can't easily enforce (and new -sys packages will have this problem, too, since it's only an issue with the Android loader), it made more sense to me to just have the build set up to discard those unused bits of code before they ever get to the linker, much less the loader.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8288)

bors-servo · 2015-11-05T00:55:53Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt


        Auto merge of #154 - larsbergstrom:new_android_build, r=Manishearth

Add the new packaging step to the nightly builder This should wait for servo/servo#8288 to land; I'll ping the review then.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/154)


        Stop using unnecessary alloc_jemalloc feature

Added in servo#8288 for Android support, but should be unnecessary in current Rust (which uses `alloc_jemalloc` by default on Android and Linux).

highfive added the S-awaiting-review label Oct 31, 2015

larsbergstrom mentioned this pull request Oct 31, 2015

Can't reply to a comment on a deleted file Reviewable/Reviewable#197

Closed

eefriedman reviewed Nov 2, 2015
View changes

jdm assigned mbrubeck Nov 2, 2015

mbrubeck added S-needs-squash and removed S-awaiting-review labels Nov 4, 2015

New Android suppport

Loading status checks…

17a6cb5

larsbergstrom force-pushed the larsbergstrom:new_android_build branch from 11f806c to 17a6cb5 Nov 4, 2015

highfive added the S-awaiting-review label Nov 4, 2015

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Nov 4, 2015

larsbergstrom mentioned this pull request Nov 4, 2015

Add the new packaging step to the nightly builder servo/saltfs#154

Merged

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 4, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 5, 2015

bors-servo mentioned this pull request Nov 5, 2015

Get servo working on win32 #7878

Closed

mbrubeck mentioned this pull request Dec 7, 2015

android build should leave android project directory easily accessible #7235

Closed

larsbergstrom deleted the larsbergstrom:new_android_build branch Sep 29, 2016

mbrubeck added a commit to mbrubeck/servo that referenced this pull request Sep 20, 2017

Stop using unnecessary alloc_jemalloc feature

bd48899

Added in servo#8288 for Android support, but should be unnecessary in current Rust (which uses `alloc_jemalloc` by default on Android and Linux).

servo / servo

New Android suppport #8288

New Android suppport #8288

larsbergstrom commented Oct 31, 2015

eefriedman commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

eefriedman commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

eefriedman commented Nov 1, 2015

larsbergstrom commented Nov 1, 2015

This comment has been minimized.

eefriedman commented Nov 2, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 4, 2015

mbrubeck commented Nov 4, 2015

larsbergstrom commented Nov 4, 2015

mbrubeck commented Nov 4, 2015

larsbergstrom commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

jdm commented Nov 4, 2015

bors-servo commented Nov 5, 2015

bors-servo commented Nov 5, 2015

servo / servo

Join GitHub today

New Android suppport #8288

New Android suppport #8288

Conversation

larsbergstrom commented Oct 31, 2015

eefriedman commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

eefriedman commented Oct 31, 2015

larsbergstrom commented Oct 31, 2015

eefriedman commented Nov 1, 2015

larsbergstrom commented Nov 1, 2015

This comment has been minimized.

eefriedman Nov 2, 2015

eefriedman commented Nov 2, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

larsbergstrom commented Nov 3, 2015

mbrubeck commented Nov 4, 2015

mbrubeck commented Nov 4, 2015

larsbergstrom commented Nov 4, 2015

mbrubeck commented Nov 4, 2015

larsbergstrom commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

jdm commented Nov 4, 2015

bors-servo commented Nov 5, 2015

bors-servo commented Nov 5, 2015