Splits Android NDK path configuration. #27173
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (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. 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. |
|
next level bash in here. 👍 |
|
Can you keep |
| @@ -650,7 +650,8 @@ CTEST_COMMON_ARGS$(1)-T-$(2)-H-$(3) := \ | |||
| --python $$(CFG_PYTHON) \ | |||
| --gdb-version="$(CFG_GDB_VERSION)" \ | |||
| --lldb-version="$(CFG_LLDB_VERSION)" \ | |||
| --android-cross-path=$(CFG_ANDROID_CROSS_PATH) \ | |||
| --aarch64-linux-android-ndk=$(CFG_AARCH64_LINUX_ANDROID_NDK) \ | |||
| --arm-linux-androideabi-ndk=$(CFG_ARM_LINUX_ANDROIDEABI_NDK) \ | |||
There was a problem hiding this comment.
I think this will also have to update compiletest as these options aren't currently recognized by compiletest.
There was a problem hiding this comment.
This can probably be left as --android-cross path and these lines unchanged. compiletest is never going to be used to test against two android ndks simultaneously.
|
cc @brson |
| fi | ||
| if [ ! -f $CFG_ANDROID_CROSS_PATH/bin/arm-linux-androideabi-ar ] | ||
| *android*) | ||
| upperSnakeTarget=$(echo $i | tr '[:lower:]' '[:upper:]' | tr '\-' '\_') |
There was a problem hiding this comment.
Stylistically our configure script tends to use underscores instead of camel case, e.g. upper_snake_target
|
I begrudgingly accept the reality that this is the best way to do this presently, but I hope it doesn't become a pattern of encoding very specific toolchain dependencies into our configure flags. |
I agree, this approach isn't ideal. If there is a preferred solution for making Rust able to build multi-Android-targets, please let me know as I'm willing to contribute effort towards it. The approach within this PR allows additional android targets to be added with minimal This PR fixes an existing issue where the aarch64-linux-android NDK tools are not tested for within the If Rust does not wish to support the building of multi-Android-target compilers (outright or via this solution), the PR could be modified to test the targets for this multi-Android-target scenario and explicitly fail with an informative error message. Thus, at the least this PR (once modified) can fix the aarch64-linux-android NDK tool test omission and maybe improve the UX of configuring Rust. |
|
@bors: r+ 393c9695dd15eb55bafca70650ec1d1b14875c53 |
|
⌛ Testing commit 393c969 with merge 94ea13d... |
|
💔 Test failed - auto-win-gnu-64-opt |
|
@bors: retry On Thu, Jul 23, 2015 at 4:32 PM, bors notifications@github.com wrote:
|
|
⌛ Testing commit 393c969 with merge b450e90... |
|
💔 Test failed - auto-win-gnu-64-nopt-t |
|
@bors: retry On Thu, Jul 23, 2015 at 4:38 PM, bors notifications@github.com wrote:
|
|
⌛ Testing commit 393c969 with merge 32f26ac... |
|
💔 Test failed - auto-linux-64-x-android-t |
|
Build failure is genuine. |
|
Stdout is saying:
Perhaps something on the build server uses the result of the |
|
@alexcrichton this needs another r+ |
|
Ah yes, thanks! Feel free to ping a PR whenever you force-push it as unfortunately github doesn't send out notifications otherwise. While we're at it, though, can you squash this as well? |
mark-buer
force-pushed
the
split-android-ndks
branch
from
cdad540 to
5dca876
Compare
|
@alexcrichton Now squashed. |
|
@bors: r+ 5dca8764145721a18698cfba76da3d2472a841af Thanks! |
|
⌛ Testing commit 5dca876 with merge ec70397... |
|
💔 Test failed - auto-linux-64-x-android-t |
mark-buer
force-pushed
the
split-android-ndks
branch
from
5dca876 to
33a7e67
Compare
|
PR updated to fix my brain fart that caused auto-linux-64-x-android-t test failures. Should be good now. (sorry) |
Allows a multi-Android-target Rust compiler to be built. Without these (or similar) changes, only a single-Android-target Rust compiler is possible. Please see https://internals.rust-lang.org/t/dual-target-android-rust-compiler/2382/3 for additional context.
Allows a multi-Android-target Rust compiler to be built.
Without these (or similar) changes, only a single-Android-target Rust compiler is possible.
Please see https://internals.rust-lang.org/t/dual-target-android-rust-compiler/2382/3 for additional context.