Improve android-ndk property interface

[pcc]

PR #102332 added support for NDK r25b, and removed support for r15. Since the switch to r25b would have broken existing r15 users anyway, let's take the opportunity to make the interface more user friendly.

Firstly move the android-ndk property to [build] instead of the targets. This is possible now that the NDK has obsoleted the concept of target-specific toolchains.

Also make the property take the NDK root directory instead of the "toolchains/llvm/prebuilt/" subdirectory.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[pcc]

r? @jyn514

cc @chriswailes

[chriswailes]

This looks good to me. It's much cleaner now.

[jyn514]

Let's hold off on merging this until we address the open questions on #102332.

[bors]

☔ The latest upstream changes (presumably #103797) made this pull request unmergeable. Please resolve the merge conflicts.

[jyn514]

Small update: #103673 tracks the upgrade to NDK r25b.

[chriswailes]

The NDK update has now been re-mereged and is targeted for release in 1.68.

[jyn514]

I don't have time for reviews for the foreseeable future.

r? @Mark-Simulacrum

[pcc]

Ping.

[jyn514]

@pcc this is marked as blocked on #103673 which is why you haven't gotten a review yet. Has something changed there?

[pcc]

This is no longer blocked on #103673; I just updated that bug to state that we've already moved to r25b and accepted the breakage. So I think we can remove the blocking label (I don't think I have permission to do it myself).

[Mark-Simulacrum]

I'm trying to confirm whether the removal of this if condition (to default_compiler above) is intentional - it seems like it would change behavior for at least some targets? Can you comment on your rationale?

[pcc]

Yes, it's needed to keep the tests passing on Android, because it turns out that $CXX needs to be set correctly for the issue-36710 test to pass. If I bring back something like the original logic like so:

diff --git a/src/bootstrap/cc_detect.rs b/src/bootstrap/cc_detect.rs
index 7b84e990ae5..714ec6284c1 100644
--- a/src/bootstrap/cc_detect.rs
+++ b/src/bootstrap/cc_detect.rs
@@ -125,8 +125,12 @@ pub fn find(build: &mut Build) {
             .and_then(|c| c.cxx.clone())
             .or_else(|| default_compiler(&mut cfg, Language::CPlusPlus, target, build))
         {
-            cfg.compiler(cxx);
-            true
+            if build.hosts.contains(&target) || build.build == target {
+                cfg.compiler(cxx);
+                true
+            } else {
+                cfg.try_get_compiler().is_ok()
+            }
         } else {
             // Use an auto-detected compiler (or one configured via `CXX_target_triple` env vars).
             cfg.try_get_compiler().is_ok()

And run the tests like so (with #102757 patched in to make the std tests pass):

python3 x.py test --target aarch64-linux-android --exclude src/tools/linkchecker --exclude tests/debuginfo --force-rerun

I see a failure later:

--- stdout -------------------------------
aarch64-linux-android-clang++ -DANDROID -ffunction-sections -fdata-sections -fPIC -c -o /mnt/disk2/pcc/rust3/build/x86_64-unknown-linux-gnu/test/run-make/issue-36710/issue-36710/libfoo.o foo.cpp
------------------------------------------
--- stderr -------------------------------
/bin/dash: 1: aarch64-linux-android-clang++: not found
make: *** [Makefile:17: /mnt/disk2/pcc/rust3/build/x86_64-unknown-linux-gnu/test/run-make/issue-36710/issue-36710/libfoo.o] Error 127
------------------------------------------

This change makes the compiler detection for C++ consistent with C, which also first tries the logic in default_compiler() to find the compiler in all cases and then tries cc::Build::get_base_compiler() via try_get_compiler() if that fails. Previously for C++ the default_compiler() (previously set_compiler()) call would be omitted for targets that are cross-compiling the standard library, leading to the test failure on Android.

I think the circumstances where this change would break a build are:

1. When cross-compiling the standard library, get_base_compiler() returns a working C++ compiler but default_compiler() returns a broken one.
2. When cross-compiling the standard library and get_base_compiler() returns None for the C++ compiler for the target, causing the get_compiler() call in default_compiler() to fail, aborting the bootstrap program.

Looking at the individual non-Android cases:

• openbsd: in some cases replaces gcc with egcc, as well as replacing g++ with eg++. This seems right, there appears to be a compiler named eg++ on some versions of OpenBSD. So I suspect this could actually fix (to some degree) cross-compiling to OpenBSD if eg++ is available as a cross compiler. OpenBSD has a C++ compiler in the base system, so the call to get_compiler() seems okay.
• mips*-unknown-linux-musl replaces gcc with mips*-linux-musl-gcc, which will have no effect in the C++ case. Other musl targets replace the compiler with musl-gcc regardless of language. This isn't right for C++ and there doesn't seem to be such a thing as musl-g++ in a normal musl installation (at least the musl sources don't mention it) and it seems plausible that a musl installation would support C but not C++, so get_base_compiler() could fail. The right fix seems to be to guard all of the musl clauses with a check that compiler == Language::C. That means we will fall back to try_get_compiler() for C++ musl targets, as we were doing before.

I've made the proposed fix for musl targets in the latest update of the PR.

[bors]

☔ The latest upstream changes (presumably #109056) made this pull request unmergeable. Please resolve the merge conflicts.

[pcc]

Ping.

[Mark-Simulacrum]

@rustbot ready (this is something you can do as well, otherwise it's in waiting-on-author and I don't see it in my queue).

[Mark-Simulacrum]

r=me with a note added to the bootstrap changelog about this change, also marking as relnotes so we can include in compat notes there. (This helps give people an explanation for the change rather than an opaque error about unknown fields in config.toml).

[slanterns]

Does it mean we should add a note to https://github.com/rust-lang/rust/blob/master/src/bootstrap/CHANGELOG.md first, then it can be merged?

[bors]

☔ The latest upstream changes (presumably #112418) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

@pcc

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[Dylan-DPC]

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

[pcc]

Created #116998