Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve android-ndk property interface #102994

Closed
wants to merge 1 commit into from
Closed

Improve android-ndk property interface #102994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions config.example.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@ changelog-seen = 2
# this is not intended to be used during local development.
#metrics = false

# Specify the location of the Android NDK. Used when targeting Android.
#android-ndk = "/path/to/android-ndk-r25b"

# =============================================================================
# General install configuration options
# =============================================================================
Expand Down Expand Up @@ -735,12 +738,6 @@ changelog-seen = 2
# it must link to `libgcc_eh.a` to get a working output, and this option have no effect.
#llvm-libunwind = 'no' if Linux, 'in-tree' if Fuchsia

# If this target is for Android, this option will be required to specify where
# the NDK for the target lives. This is used to find the C compiler to link and
# build native code.
# See `src/bootstrap/cc_detect.rs` for details.
#android-ndk = <none> (path)

# Build the sanitizer runtimes for this target.
# This option will override the same option under [build] section.
#sanitizers = build.sanitizers (bool)
Expand Down
81 changes: 48 additions & 33 deletions src/bootstrap/cc_detect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use std::path::{Path, PathBuf};
use std::process::Command;
use std::{env, iter};

use crate::config::{Target, TargetSelection};
use crate::config::TargetSelection;
use crate::util::output;
use crate::{Build, CLang, GitRepo};

Expand Down Expand Up @@ -100,10 +100,11 @@ pub fn find(build: &mut Build) {
for target in targets.into_iter() {
let mut cfg = new_cc_build(build, target);
let config = build.config.target_config.get(&target);
if let Some(cc) = config.and_then(|c| c.cc.as_ref()) {
if let Some(cc) = config
.and_then(|c| c.cc.clone())
.or_else(|| default_compiler(&mut cfg, Language::C, target, build))
{
cfg.compiler(cc);
} else {
set_compiler(&mut cfg, Language::C, target, config, build);
}

let compiler = cfg.get_compiler();
Expand All @@ -120,12 +121,12 @@ pub fn find(build: &mut Build) {
// We'll need one anyways if the target triple is also a host triple
let mut cfg = new_cc_build(build, target);
cfg.cpp(true);
let cxx_configured = if let Some(cxx) = config.and_then(|c| c.cxx.as_ref()) {
let cxx_configured = if let Some(cxx) = config
.and_then(|c| c.cxx.clone())
.or_else(|| default_compiler(&mut cfg, Language::CPlusPlus, target, build))
{
cfg.compiler(cxx);
true
} else if build.hosts.contains(&target) || build.build == target {
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Mar 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

set_compiler(&mut cfg, Language::CPlusPlus, target, config, build);
true
} else {
// Use an auto-detected compiler (or one configured via `CXX_target_triple` env vars).
cfg.try_get_compiler().is_ok()
Expand Down Expand Up @@ -155,68 +156,70 @@ pub fn find(build: &mut Build) {
}
}

fn set_compiler(
fn default_compiler(
cfg: &mut cc::Build,
compiler: Language,
target: TargetSelection,
config: Option<&Target>,
build: &Build,
) {
) -> Option<PathBuf> {
match &*target.triple {
// When compiling for android we may have the NDK configured in the
// config.toml in which case we look there. Otherwise the default
// compiler already takes into account the triple in question.
t if t.contains("android") => {
if let Some(ndk) = config.and_then(|c| c.ndk.as_ref()) {
cfg.compiler(ndk_compiler(compiler, &*target.triple, ndk));
}
}
t if t.contains("android") => build
.config
.android_ndk
.as_ref()
.map(|ndk| ndk_compiler(compiler, &*target.triple, ndk)),

// The default gcc version from OpenBSD may be too old, try using egcc,
// which is a gcc version from ports, if this is the case.
t if t.contains("openbsd") => {
let c = cfg.get_compiler();
let gnu_compiler = compiler.gcc();
if !c.path().ends_with(gnu_compiler) {
return;
return None;
}

let output = output(c.to_command().arg("--version"));
let i = match output.find(" 4.") {
Some(i) => i,
None => return,
};
let i = output.find(" 4.")?;
match output[i + 3..].chars().next().unwrap() {
'0'..='6' => {}
_ => return,
_ => return None,
}
let alternative = format!("e{}", gnu_compiler);
if Command::new(&alternative).output().is_ok() {
cfg.compiler(alternative);
Some(PathBuf::from(alternative))
} else {
None
}
}

"mips-unknown-linux-musl" => {
"mips-unknown-linux-musl" if compiler == Language::C => {
if cfg.get_compiler().path().to_str() == Some("gcc") {
cfg.compiler("mips-linux-musl-gcc");
Some(PathBuf::from("mips-linux-musl-gcc"))
} else {
None
}
}
"mipsel-unknown-linux-musl" => {
"mipsel-unknown-linux-musl" if compiler == Language::C => {
if cfg.get_compiler().path().to_str() == Some("gcc") {
cfg.compiler("mipsel-linux-musl-gcc");
Some(PathBuf::from("mipsel-linux-musl-gcc"))
} else {
None
}
}

t if t.contains("musl") => {
t if t.contains("musl") && compiler == Language::C => {
if let Some(root) = build.musl_root(target) {
let guess = root.join("bin/musl-gcc");
if guess.exists() {
cfg.compiler(guess);
}
if guess.exists() { Some(guess) } else { None }
} else {
None
}
}

_ => {}
_ => None,
}
}

Expand All @@ -237,10 +240,22 @@ pub(crate) fn ndk_compiler(compiler: Language, triple: &str, ndk: &Path) -> Path
let api_level =
if triple.contains("aarch64") || triple.contains("x86_64") { "21" } else { "19" };
let compiler = format!("{}{}-{}", triple_translated, api_level, compiler.clang());
ndk.join("bin").join(compiler)
let host_tag = if cfg!(target_os = "macos") {
// The NDK uses universal binaries, so this is correct even on ARM.
"darwin-x86_64"
} else if cfg!(target_os = "windows") {
"windows-x86_64"
} else {
// NDK r25b only has official releases for macOS, Windows and Linux.
// Try the Linux directory everywhere else, on the assumption that the OS has an
// emulation layer that can cope (e.g. BSDs).
"linux-x86_64"
};
ndk.join("toolchains").join("llvm").join("prebuilt").join(host_tag).join("bin").join(compiler)
}

/// The target programming language for a native compiler.
#[derive(PartialEq)]
pub(crate) enum Language {
/// The compiler is targeting C.
C,
Expand Down
17 changes: 5 additions & 12 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use std::str::FromStr;

use crate::builder::TaskPath;
use crate::cache::{Interned, INTERNER};
use crate::cc_detect::{ndk_compiler, Language};
use crate::channel::{self, GitInfo};
pub use crate::flags::Subcommand;
use crate::flags::{Color, Flags};
Expand Down Expand Up @@ -85,6 +84,7 @@ pub struct Config {
pub color: Color,
pub patch_binaries_for_nix: bool,
pub stage0_metadata: Stage0Metadata,
pub android_ndk: Option<PathBuf>,

pub stdout_is_tty: bool,
pub stderr_is_tty: bool,
Expand Down Expand Up @@ -452,7 +452,6 @@ pub struct Target {
pub ranlib: Option<PathBuf>,
pub default_linker: Option<PathBuf>,
pub linker: Option<PathBuf>,
pub ndk: Option<PathBuf>,
pub sanitizers: Option<bool>,
pub profiler: Option<bool>,
pub crt_static: Option<bool>,
Expand Down Expand Up @@ -654,6 +653,7 @@ define_config! {
patch_binaries_for_nix: Option<bool> = "patch-binaries-for-nix",
// NOTE: only parsed by bootstrap.py, `--feature build-metrics` enables metrics unconditionally
metrics: Option<bool> = "metrics",
android_ndk: Option<PathBuf> = "android-ndk",
}
}

Expand Down Expand Up @@ -797,7 +797,6 @@ define_config! {
llvm_has_rust_patches: Option<bool> = "llvm-has-rust-patches",
llvm_filecheck: Option<String> = "llvm-filecheck",
llvm_libunwind: Option<String> = "llvm-libunwind",
android_ndk: Option<String> = "android-ndk",
sanitizers: Option<bool> = "sanitizers",
profiler: Option<bool> = "profiler",
crt_static: Option<bool> = "crt-static",
Expand Down Expand Up @@ -1044,6 +1043,7 @@ impl Config {
config.python = build.python.map(PathBuf::from);
config.reuse = build.reuse.map(PathBuf::from);
config.submodules = build.submodules;
config.android_ndk = build.android_ndk;
set(&mut config.low_priority, build.low_priority);
set(&mut config.compiler_docs, build.compiler_docs);
set(&mut config.library_docs_private_items, build.library_docs_private_items);
Expand Down Expand Up @@ -1279,18 +1279,11 @@ impl Config {
.llvm_libunwind
.as_ref()
.map(|v| v.parse().expect("failed to parse rust.llvm-libunwind"));
if let Some(ref s) = cfg.android_ndk {
target.ndk = Some(config.src.join(s));
}
if let Some(s) = cfg.no_std {
target.no_std = s;
}
target.cc = cfg.cc.map(PathBuf::from).or_else(|| {
target.ndk.as_ref().map(|ndk| ndk_compiler(Language::C, &triple, ndk))
});
target.cxx = cfg.cxx.map(PathBuf::from).or_else(|| {
target.ndk.as_ref().map(|ndk| ndk_compiler(Language::CPlusPlus, &triple, ndk))
});
target.cc = cfg.cc.map(PathBuf::from);
target.cxx = cfg.cxx.map(PathBuf::from);
target.ar = cfg.ar.map(PathBuf::from);
target.ranlib = cfg.ranlib.map(PathBuf::from);
target.linker = cfg.linker.map(PathBuf::from);
Expand Down
15 changes: 1 addition & 14 deletions src/bootstrap/configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,7 @@ def v(*args):
v("llvm-config", None, "set path to llvm-config")
v("llvm-filecheck", None, "set path to LLVM's FileCheck utility")
v("python", "build.python", "set path to python")
v("android-cross-path", "target.arm-linux-androideabi.android-ndk",
"Android NDK standalone path (deprecated)")
v("i686-linux-android-ndk", "target.i686-linux-android.android-ndk",
"i686-linux-android NDK standalone path")
v("arm-linux-androideabi-ndk", "target.arm-linux-androideabi.android-ndk",
"arm-linux-androideabi NDK standalone path")
v("armv7-linux-androideabi-ndk", "target.armv7-linux-androideabi.android-ndk",
"armv7-linux-androideabi NDK standalone path")
v("thumbv7neon-linux-androideabi-ndk", "target.thumbv7neon-linux-androideabi.android-ndk",
"thumbv7neon-linux-androideabi NDK standalone path")
v("aarch64-linux-android-ndk", "target.aarch64-linux-android.android-ndk",
"aarch64-linux-android NDK standalone path")
v("x86_64-linux-android-ndk", "target.x86_64-linux-android.android-ndk",
"x86_64-linux-android NDK standalone path")
v("android-ndk", "build.android-ndk", "set path to Android NDK")
v("musl-root", "target.x86_64-unknown-linux-musl.musl-root",
"MUSL root installation directory (deprecated)")
v("musl-root-x86_64", "target.x86_64-unknown-linux-musl.musl-root",
Expand Down
2 changes: 1 addition & 1 deletion src/ci/docker/host-x86_64/arm-android/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ ENV PATH=$PATH:/android/sdk/platform-tools

ENV TARGETS=arm-linux-androideabi

ENV RUST_CONFIGURE_ARGS --arm-linux-androideabi-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/
ENV RUST_CONFIGURE_ARGS --android-ndk=/android/ndk/

ENV SCRIPT python3 ../x.py --stage 2 test --host='' --target $TARGETS

Expand Down
7 changes: 1 addition & 6 deletions src/ci/docker/host-x86_64/dist-android/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@ ENV TARGETS=$TARGETS,x86_64-linux-android
ENV RUST_CONFIGURE_ARGS \
--enable-extended \
--enable-profiler \
--arm-linux-androideabi-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--armv7-linux-androideabi-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--thumbv7neon-linux-androideabi-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--i686-linux-android-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--aarch64-linux-android-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--x86_64-linux-android-ndk=/android/ndk/toolchains/llvm/prebuilt/linux-x86_64/ \
--android-ndk=/android/ndk/ \
--disable-docs

ENV SCRIPT python3 ../x.py dist --host='' --target $TARGETS
Expand Down