Closed Bug 1220307 Opened 9 years ago Closed 8 years ago

Cross rustc for android builds

Categories

(Firefox Build System :: General, defect, P3)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(firefox45 affected, firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox49 --- fixed

People

(Reporter: rillian, Unassigned)

References

Details

Attachments

(3 files)

part 1 - tooltool packages for ARM Android builds
1.32 KB, patch
mshal
: review+
Details | Diff | Splinter Review
part 1a - use something other than vector<T>::data() in TestMP4Rust.cpp
2.20 KB, patch
rillian
: review+
Details | Diff | Splinter Review
part 2 - enable Rust in ARM Android nightly and debug builds
2.82 KB, patch
nalexander
: review+
Margaret
: review+
Details | Diff | Splinter Review 
In bug 1219983, Alex Crichton mentioned there had been progress on official toolchain support for android. Currently (nightly at least) is built with some cross code generation support, and there are builds of the rust std library for a variety of architectures, and when installed in lib/rustlib enables support for that platform triplet in --target.

However, one still needs a cross linker and system libraries for the target.

This bug is about figuring out the rest of what's needed and packaging it so we can compile rust code for arm-linux-android and mac from linux taskcluster jobs.
Here's an example of installing the alternate std libraries.

  wget https://static.rust-lang.org/dist/rust-std-nightly-x86_64-apple-darwin.tar.gz.asc
  wget https://static.rust-lang.org/dist/rust-std-nightly-x86_64-apple-darwin.tar.gz
  keybase verify rust-std-nightly-x86_64-apple-darwin.tar.gz.asc
  tar xf rust-std-nightly-x86_64-apple-darwin.tar.gz
  rust-std-nightly-x86_64-apple-darwin/install.sh --prefix=~/.local/rust-nightly

then cargo build --target=x86_64-apple-darwin' works for libraries.

Closest I've gotten for android is:

$ rustc --target=arm-linux-androideabi hello.rs -C linker=/data/giles/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/arm-linux-androideabi/bin/ld -C link-args="-L /data/giles/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/arm-linux-androideabi/lib -L /data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/"
note: /data/giles/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/arm-linux-androideabi/bin/ld: -Wl,--as-needed: unknown option

need a newer ndk maybe?
For Android the linker we typically pass is arm-linux-androideabi-gccc, but in general it currently needs to be a gcc-like linker rather than the literal linker itself (gcc finds startup objects and whatnot)
Ah, that makes sense for why it wouldn't like "-Wl,foo". That works for --crate-type staticlib!

For executables:

$ rustc hello.rs --target=arm-linux-androideabi -C linker=/data/giles/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc -C link-args="-L /data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib"

note: /data/giles/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld: error: cannot open crtbegin_dynamic.o: No such file or directory

$ find /data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib -name \*.o
/data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/crtbegin_so.o
/data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/crtbegin_dynamic.o
/data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/crtend_android.o
/data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/crtend_so.o
/data/giles/android/android-ndk-r10e/platforms/android-21/arch-arm/usr/lib/crtbegin_static.o
Hm I can't say that's an error I've ever seen before! Can you use gcc to produce an executable just calling it? I'd be a little surprised if that worked but rustc didn't, but there's perhaps something we're forgetting to do?
You're correct, I get the same error liking a C executable. So something's missing from my setup, or there's something else I need to pass in both cases. Anyway, library output should be sufficient for our current purpose.
Blocks: 1177599
Blocks: 1244261
No longer blocks: 1244261
Repurposing this just for android.
Summary: Cross rustc for mac and android builds → Cross rustc for android builds
Alex, I see from https://github.com/rust-lang/rust/blob/master/mk/cfg/arm-linux-androideabi.mk that there don't appear to be any special knobs set for CPU family, thumb, etc.  Whereas for something like the armv7 hard-float linux target there do: https://github.com/rust-lang/rust/blob/master/mk/cfg/armv7-unknown-linux-gnueabihf.mk  I assume that means the default arm-linux-androideabi runtime libraries are compiled with a lowest common denominator (something like armv5, soft-float, no thumb?)...do you know if that's correct?

Do you know how we might go about changing that and/or adding a new Rust target (is that possible)?  We can obviously twiddle flags for compiling Mozilla code, but it'd be nice if the things in the standard library were compiled for our preferred feature set (judging by http://mxr.mozilla.org/mozilla-central/source/build/autoconf/arch.m4#29, we only support armv7-a with thumb2, vfp instructions required, and use the soft-float ABI).

I don't think this inhibits our ability to use the distributed Android stdlib, but it's definitely something to think about as we consider making Rust a required part of Firefox.
Flags: needinfo?(acrichton)
Ah yeah, the actual definition (defaults in the compilers is here: https://github.com/rust-lang/rust/blob/master/src/librustc_back/target/arm_linux_androideabi.rs, so it looks like we at least enable v7 code generation in LLVM (I think that's what +v7 is?). That being said, I'm not actually sure what the other parameters are (e.g. float, thumb, etc). Also note that the new armv7 target should in theory not have any extra -C flags in the makefiles, those should be bake directly into the definitions in the compiler (I may go fix this soon).

Adding new targets is definitely possible! I suspect that if you want to download a pre-built standard library we'll basically have to do this to ensure that its code is generated correctly. That being said, if your requirements aren't too dark-age-esque, we can probably just change the default Android target (if that's what you want to use). I think that Servo is the other major user of the Android target and they're likely targeting the same devices as you are!

So I guess what we'd need to know is:

* What target-triple would you like to use? arm-linux-androideabi?
* A supported cpu of armv7-a is what you use?
* Do you know of other specific LLVM features to enable/disable? vfp/soft-float are good to know, but not sure if you call clang in crazy ways or something like that.

It'd probably be "best" to verify via something like `rustc my-custom-crate.rs -C target-cpu=.. -C target-feature=..` to double-check ahead of time what's right, but we can probably guess as to what's correct.
Flags: needinfo?(acrichton)
Ah, this is a fun error:

 09:19:30     INFO -  conftest.rs:1:1: 1:1 error: the crate `std` has been compiled with rustc 1.6.0 (c30b771ad 2016-01-19), which is incompatible with this version of rustc [E0514]
 09:19:30     INFO -  conftest.rs:1 pub extern fn hello() { println!("Hello world"); }
 09:19:30     INFO -                ^

(Android try push with linux64 rustc and repackaged stdlib for android from rust-lang.org)

Looks like if we're going to compile our own rustc, we also need to compile our own standard libraries for android.  One more motivation to use the standard packages, I guess?
Ah yeah whatever rustc is being used must have basically all libraries in use compiled at the same revision as the rustc itself.
Is that the whole story?  Judging by the error message, the android libraries were built from https://github.com/rust-lang/rust/commit/c30b771ad9d44ab84f8c88b80c25fcfde2433126 and I assume rillian built from the stable branch at the same revision, though I can't find any documentation in bug 1240982 or the logs from the taskcluster build to indicate whether that assumption holds.
Oh hm it looks like it's actually just doing a string comparison of the CFG_VERSION values at build-time. This I think includes the version information like 1.6.0 vs nightly. That means that if the android library is configured as built from a nightly compiler at a specific revision it's incompatible with a stable compiler at the same revision (as the version numbers are different). That... may be a bug in our compiler!
Also, while there appears to be support for x86 android in the rust tree, there are no released runtime libraries for that target (stable, beta, or nightly).
Ah yeah "support" in this case means that libstd was ported and compiled at *some* point in time, but there's not really any guarantee about what happens today. If you guys need and/or would like x86 android support, however, it would provide good impetus for bringing it back up to date! (e.g. adding releases)
What's the process for doing that?  How complicated is it?
For us it would probably look like:

1. Get libstd building again (e.g. ./configure --target=i686-android)
2. Submit a PR with any changes made to get things building
3. Update the rust-lang/rust-buildbot docker image to have build support for i686 android
4. Update rust-lang/rust-buildbot buildbot config to gate on and build i686 android nightlies

I can help take care of (3) and (4) if necessary, but I personally wouldn't know much about (1) as I don't even really know what it means to have i686 android!
(In reply to Alex Crichton [:acrichto] from comment #16)
> For us it would probably look like:
> 
> 1. Get libstd building again (e.g. ./configure --target=i686-android)

Tried this out today with a fresh Rust pull and everything worked fine.

> 2. Submit a PR with any changes made to get things building

There is no step 2!

> 3. Update the rust-lang/rust-buildbot docker image to have build support for
> i686 android

It looks like this is already there, assuming that:

https://github.com/rust-lang/rust-buildbot/blob/master/slaves/android/install-ndk.sh#L29

is the bit that runs on the docker image.

One wrinkle is that the above line sets things up to use android-18, whereas our android binaries use android-9.  I don't know how much different there is between the two platforms.  I suspect we could bump down the i686 android platform version, since it's only tier3 at the moment (moving up to tier as a result of this work?), but I'm not sure what bumping down the arm android platform version would do.  x86 builds fine with either android-18 or android-9, so I suspect arm would too?

> 4. Update rust-lang/rust-buildbot buildbot config to gate on and build i686
> android nightlies

This seems a little trickier, as things like:

https://github.com/rust-lang/rust-buildbot/blob/master/master/master.cfg#L862

suggest that the buildbot config isn't really set up to deal with multiple android architectures.  (It is entirely possible that I just don't understand the buildbot config.)  I could probably figure out what needs to be done here, but I'd need a bit of guidance writing it and/or testing it.
Ideally,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f717776f6fc

will come back with green builds and tests for ARM Android.  (The last build failed in TestMP4Rust.cpp, so I feel pretty good that the rustc part of the patch is working correctly.)
Linux 64 rustc + ARM Android libraries.
Attachment #8744055 - Flags: review?(mshal)
Our copy of STLport on Android is so old, it doesn't have support for
vector<T>::data().  The standard defines vector<T>::data() in terms of
vector<T>::front(), so this change should work everywhere.
Attachment #8744062 - Flags: review?(giles)
ARM only because x86 requires changes on the Rust side.
Attachment #8744064 - Flags: review?(nalexander)
Comment on attachment 8744055 [details] [diff] [review]
part 1 - tooltool packages for ARM Android builds

Looks fine. One thing that could be an issue is if rust-std-lib.tar.bz2 gets processed before rustc.tar.xz somehow, since tooltool will do an 'rm -rf rustc/' when it processes rustc.tar.xz. But if it works on try I guess it's fine :)
Attachment #8744055 - Flags: review?(mshal) → review+
Comment on attachment 8744064 [details] [diff] [review]
part 2 - enable Rust in ARM Android nightly and debug builds

Review of attachment 8744064 [details] [diff] [review]:
-----------------------------------------------------------------

Code is fine; if it's on try, it's fine by me.

However, I'd like to know the APK bloat; and I'd like margaret to know the state of things.  Hence, redirect.
Attachment #8744064 - Flags: review?(nalexander)
Attachment #8744064 - Flags: review?(margaret.leibovic)
Attachment #8744064 - Flags: review+
> > 1. Get libstd building again (e.g. ./configure --target=i686-android)
> 
> Tried this out today with a fresh Rust pull and everything worked fine.

Whoa, nice!

> One wrinkle is that the above line sets things up to use android-18, whereas
> our android binaries use android-9. 

Whoa holy cow, that's old! (I think?) We're certainly always willing to push compatibility backwards if it's needed, so this change would likely be fine by us. The Rust standard library will probably need to change as a result (e.g. we probably depend on some post-android-9 feature), but in theory it shouldn't be too too hard.


> > 4. Update rust-lang/rust-buildbot buildbot config to gate on and build i686
> > android nightlies
> 
> This seems a little trickier

Yeah I wouldn't necessarily expect you to try to take this on :). Our buildbot config is a Medusa-like you-turn-to-stone-as-you-look-at-it mess. It would be pretty easy, however, for us to at least *build* i686 Android to start out. That way we can at least guarantee that it links and we can also ship binaries.

We wouldn't quite be "tier 1" yet because we wouldn't be running tests, but tests would perhaps not be *too* far off!
Comment on attachment 8744064 [details] [diff] [review]
part 2 - enable Rust in ARM Android nightly and debug builds

Review of attachment 8744064 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Nick Alexander :nalexander from comment #23)
> Comment on attachment 8744064 [details] [diff] [review]
> part 2 - enable Rust in ARM Android nightly and debug builds
> 
> Review of attachment 8744064 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code is fine; if it's on try, it's fine by me.
> 
> However, I'd like to know the APK bloat; and I'd like margaret to know the
> state of things.  Hence, redirect.

If you do try builds with/without your patches, you should be able to see the impact on the APK size. If you click on the "B" you will see details directly in the "Job details" tab on perfherder. We will also get a perfherder alert if this regresses the APK size by more than 100kb.

If this is only going to be included in debug/Nightly builds, I'm less concerned about APK size. But please make sure these things are excluded at build time for beta/release builds (it looks like that's what's happening here).
Attachment #8744064 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #25)
> If you do try builds with/without your patches, you should be able to see
> the impact on the APK size. If you click on the "B" you will see details
> directly in the "Job details" tab on perfherder. We will also get a
> perfherder alert if this regresses the APK size by more than 100kb.
> 
> If this is only going to be included in debug/Nightly builds, I'm less
> concerned about APK size. But please make sure these things are excluded at
> build time for beta/release builds (it looks like that's what's happening
> here).

It looks like the total impact on APK size is in the single-digit thousands of bytes (~4K), which seems small to me, but perhaps the mp4 parser doesn't use a lot of the Rust standard library.  We'll eventually want to enable this for beta/release as well, but given the small impact, ideally that shouldn't be a problem.
(In reply to Alex Crichton [:acrichto] from comment #24)
> > One wrinkle is that the above line sets things up to use android-18, whereas
> > our android binaries use android-9. 
> 
> Whoa holy cow, that's old! (I think?) We're certainly always willing to push
> compatibility backwards if it's needed, so this change would likely be fine
> by us. The Rust standard library will probably need to change as a result
> (e.g. we probably depend on some post-android-9 feature), but in theory it
> shouldn't be too too hard.

Seems to build (at least for x86) just fine for android-18 and android-9.  FWIW, building Firefox with the released ARM binaries and running our standard suite of tests passes with flying colors.  So it doesn't look like that bit of a deal...but I would sleep better if everything was compiled with android-9, I think. :)  I'll try arm tomorrow, and submit a pull request if things look good?

Filed https://github.com/rust-lang/rust-buildbot/issues/98 for this, please let me know if I put it in the wrong place.  (Seems a little odd to have that sort of information in the rust-buildbot repository rather than the main rust repository, but if that's the way it works...)

> Yeah I wouldn't necessarily expect you to try to take this on :). Our
> buildbot config is a Medusa-like you-turn-to-stone-as-you-look-at-it mess.
> It would be pretty easy, however, for us to at least *build* i686 Android to
> start out. That way we can at least guarantee that it links and we can also
> ship binaries.

Hooray!  Filed https://github.com/rust-lang/rust-buildbot/issues/97 for this.

> We wouldn't quite be "tier 1" yet because we wouldn't be running tests, but
> tests would perhaps not be *too* far off!

I think we would be happy with whatever ARM Android is (tier 2?) so we could get official binaries from rust-lang.org.
> FWIW, building Firefox with the released ARM binaries and running our
> standard suite of tests passes with flying colors.

Wow now that is indeed surprising! I agree though that it'd be best to move ourselves back to make sure we don't leak in any accidental post-android-9-isms. PRs are indeed welcome! I can work on getting it deployed once it looks like it passes our own tests as well.

> Filed https://github.com/rust-lang/rust-buildbot/issues/98 for this, please
> let me know if I put it in the wrong place.

Ah no worries, that's a good location for it!

> I think we would be happy with whatever ARM Android is (tier 2?) so we could
> get official binaries from rust-lang.org.

Cool, sounds good to me! Thanks for the reports, and if you send a PR for moving Android back to 9 from 18 (seems like you wanna run some tests locally as well?) then I can work on getting these deployed (plus get the i686 Android standard library shipping)
Comment on attachment 8744062 [details] [diff] [review]
part 1a - use something other than vector<T>::data() in TestMP4Rust.cpp

Review of attachment 8744062 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, thanks for getting this working. Very exciting!

Re pre-C+11, ugh. :P
Attachment #8744062 - Flags: review?(giles) → review+
Blocks: 1266792
Blocks: 1266877
sorry had to back this out since this cause a merge conflict when merging to mozilla-central due to a change at fx-team for the same file that landed before on m-c

merging mobile/android/config/tooltool-manifests/android/releng.manifest
warning: conflicts while merging mobile/android/config/tooltool-manifests/android/releng.manifest! (edit, then use 'hg resolve --mark')
267 files updated, 0 files merged, 13 files removed, 1 files unresolved
Flags: needinfo?(nfroyd)
Yup, I'll rebase these and add Margaret as a reviewer.
Flags: needinfo?(nfroyd)
(In reply to Pulsebot from comment #35)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f168a1623065

Oops, I accidentally backed out the wrong three commit push for bustage. Relanded.
Part 3 was backed out to fix bug 1276860. Reopening this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/da10eecd0e76
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8744062 [details] [diff] [review]
part 1a - use something other than vector<T>::data() in TestMP4Rust.cpp

And by "part 3" I meant "the third patch" which is labelled "part 2" in the attachment description.
Attachment #8744062 - Flags: checkin+
Priority: -- → P3
This was taken care of in other bugs; most recently bug 1306438.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: