Skip to content
This repository was archived by the owner on Nov 21, 2018. It is now read-only.

android version downgrade for arm and x86 #99

Closed

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Apr 22, 2016

Compiling Rust's std works just fine with API level 9. I didn't see anything too unusual in Rust's system-dependent code when I looked for possible gotchas, but I also didn't test this beyond compiling things.

Two interesting things to note that I discovered this morning:

  1. Firefox is currently in the middle of changing its API level requirements. Nightly Firefox only supports ICS (API level 15); I'm unsure of where Beta and Developer Edition are with regards to the API level change. I'd like Rust stable to be usable with both arm-linux-androideabi and i686-linux-android sooner rather than later, though, and lowering the requirements as far as possible seems like a good thing in any event.
  2. Servo's wiki instructions for building for Android state that you should use API level 18, which I presume is why 18 was chosen for the buildbot configs. But I don't think this should hold back moving the Rust requirements backwards; compiling Servo with API level 18 using a Rust std compiled against API level 9 ought to work just fine.

This pull request also sneaks in what looks like a cleanup: making sure we only have one ARM standalone toolchain built from the NDK. It's entirely possible there are constraints here that I'm not aware of (extra tools? multilibs?), so please do tell me if that commit is bogus, and I'll happily drop it.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Contributor

Firefox is currently in the middle of changing its API level requirements. Nightly Firefox only supports ICS (API level 15);

Interesting! Could we get away with 15 in that case? If Android support for Rust in Firefox landed today, it'd only be available on nightly anyway, right?

Servo's wiki instructions for building for Android state that you should use API level 18, which I presume is why 18 was chosen for the buildbot configs.

Spot on!

I'd be surprised if an android-9 (or android-15) compiled standard library were incompatible with android-18. And even if so, we can likely work around it like in rust-lang/rust#32415

This pull request also sneaks in what looks like a cleanup: making sure we only have one ARM standalone toolchain built from the NDK.

Ah yeah actually this docker image is used for two different purposes.

  • One NDK (ndk-arm-18) is used to produce the Rust standard library binaries.
  • Another NDK (ndk-arm) is meant to be the most up to date and is used for testing the libc crate to ensure we have access to the most up-to-date APIs on Android.

Could you leave ndk-arm as-is, and just remove ndk-arm-18 in favor of ndk-arm-9? (or if you're out of time, I can also do this).

@alexcrichton
Copy link
Contributor

While I'm waiting on some builds elsewhere, I'm gonna give this a run through dev to see what happens to our testing bot

@alexcrichton
Copy link
Contributor

Looks like the build runs ok but the tests fail unfortunately:

note: /buildslave/rust-buildbot/slave/auto-linux-64-x-android-t/build/obj/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib/libstd-2f39a9bd.so: error: undefined reference to 'ftruncate64'
/buildslave/rust-buildbot/slave/auto-linux-64-x-android-t/build/obj/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib/libstd-2f39a9bd.so: error: undefined reference to 'log2'
/buildslave/rust-buildbot/slave/auto-linux-64-x-android-t/build/obj/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib/libstd-2f39a9bd.so: error: undefined reference to 'log2f'

Basically, ftruncate64, log2, and log2f are all missing symbols in android-9.

Perhaps some are available in android-15 though?

@alexcrichton
Copy link
Contributor

FWIW the building x86 stuff worked out just fine, so I pushed up e5ac28a and we can deploy whenever basically

@froydnj
Copy link
Contributor Author

froydnj commented Apr 25, 2016

ftruncate64 is available as early as android-12. log2 and log2f are not available until android-18. So we may not be able to push back the requirements at all, at least not without some work on the Rust side?

FWIW, we did some work on the Firefox side to actually properly enable our Rust code on Android and found that std's dependence on log2 prevents Firefox from linking with its current android-9 platform. I'll try to write that up as a Rust issue once I'm confident about what's happening there.

@alexcrichton
Copy link
Contributor

Hm ok, so log2 and log2f may be harder to work around, but we can finagle something perhaps. The ftruncate64 symbol we can just do dynamic detection of. I suspect that ftruncate exists on earlier Android versions.

Would it be possible to use android-15 though? Then we'd only have to solve log2 and log2f :)

@rillian
Copy link
Contributor

rillian commented Apr 25, 2016

log2(x) can be implemented as log(x)/log(2). We could either do that in libstd with yet another #cfg version conditional, or patch llvm to provide a fallback in the intrinsic.

@froydnj
Copy link
Contributor Author

froydnj commented Apr 25, 2016

ftruncate does exist on all Android versions, so that shouldn't be a problem. But we can probably make the ftruncate64 issue go away on the Firefox side by just using android-15.

I dunno, I think you'd want to use log2 if it existed at runtime, so you'd have to do a runtime check? That gets pretty complicated. I'm also not sure if you'd run into precision issues when using log(x) / log(2).

@alexcrichton
Copy link
Contributor

We can probably get by with a shim on Android for now, but I'll send some PRs to the main repo. May as well take care of ftruncate64 if we can!

@alexcrichton
Copy link
Contributor

Note that the PR for libstd itself is here: rust-lang/rust#33211

@froydnj froydnj force-pushed the android-version-downgrade branch from e471673 to 9e6ebef Compare May 9, 2016 16:27
This change makes them more compatible with Firefox releases, which
currently use API level 9 as their baseline.

Fixes rust-lang-deprecated#98.
@froydnj
Copy link
Contributor Author

froydnj commented May 9, 2016

Pushed a new head for just the android-9 version change. r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson May 9, 2016
@froydnj
Copy link
Contributor Author

froydnj commented May 9, 2016

Ah, whoops, I forgot to rebase to HEAD and I see that there has been some shuffling; the install-ndk.sh script now installs for android-9, but the master.cfg file is still using the android-18 NDK toolchain location?

@froydnj froydnj force-pushed the android-version-downgrade branch from 9e6ebef to 7937124 Compare May 9, 2016 16:45
@alexcrichton
Copy link
Contributor

Yeah this is somewhat inconsistent unfortunately. We're not using the current image as listed but rather an older one. The buildmaster config is configured for that one as well. We tried to deploy android-9 but we failed some tests and I haven't had a chance to investigate yet, unfortunately. That is, we tried this out but it unfortunately caused failures.

I think the failures were related to posix_memalign, but I'm not sure if the failures would stop there.

@alexcrichton
Copy link
Contributor

Ok, I'm working through what is hopefully the last few failures. In addition to posix_memalign the r11c NDK also stopped shipping arm-linux-androideabi-gdb wrappers so our debuginfo tests were failing. I hope to get this merged soon though, and I'll push the final updates when it's ready. Thanks regardless though for the PR @froydnj!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants