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

Implement autodetection for default compiler from NDK #495

Merged
merged 1 commit into from
May 14, 2020
Merged

Implement autodetection for default compiler from NDK #495

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
111 changes: 87 additions & 24 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,6 +1370,15 @@ impl Build {
cmd.push_opt_unless_duplicate(format!("-O{}", opt_level).into());
}

if cmd.family == ToolFamily::Clang && target.contains("android") {
// For compatibility with code that doesn't use pre-defined `__ANDROID__` macro.
// If compiler used via ndk-build or cmake (officially supported build methods)
// this macros is defined.
// See https://android.googlesource.com/platform/ndk/+/refs/heads/ndk-release-r21/build/cmake/android.toolchain.cmake#456
// https://android.googlesource.com/platform/ndk/+/refs/heads/ndk-release-r21/build/core/build-binary.mk#141
cmd.push_opt_unless_duplicate("-DANDROID".into());
Dushistov marked this conversation as resolved.
Show resolved Hide resolved
}

if !target.contains("-ios") {
cmd.push_cc_arg("-ffunction-sections".into());
cmd.push_cc_arg("-fdata-sections".into());
Expand Down Expand Up @@ -1406,7 +1415,11 @@ impl Build {
// Target flags
match cmd.family {
ToolFamily::Clang => {
cmd.args.push(format!("--target={}", target).into());
if !(target.contains("android")
&& android_clang_compiler_uses_target_arg_internally(&cmd.path))
{
cmd.args.push(format!("--target={}", target).into());
}
}
ToolFamily::Msvc { clang_cl } => {
// This is an undocumented flag from MSVC but helps with making
Expand Down Expand Up @@ -1974,29 +1987,7 @@ impl Build {
format!("{}.exe", gnu)
}
} else if target.contains("android") {
let target = target
.replace("armv7neon", "arm")
.replace("armv7", "arm")
.replace("thumbv7neon", "arm")
.replace("thumbv7", "arm");
let gnu_compiler = format!("{}-{}", target, gnu);
let clang_compiler = format!("{}-{}", target, clang);
// On Windows, the Android clang compiler is provided as a `.cmd` file instead
// of a `.exe` file. `std::process::Command` won't run `.cmd` files unless the
// `.cmd` is explicitly appended to the command name, so we do that here.
let clang_compiler_cmd = format!("{}-{}.cmd", target, clang);

// Check if gnu compiler is present
// if not, use clang
if Command::new(&gnu_compiler).output().is_ok() {
gnu_compiler
} else if host.contains("windows")
&& Command::new(&clang_compiler_cmd).output().is_ok()
{
clang_compiler_cmd
} else {
clang_compiler
}
autodetect_android_compiler(&target, &host, gnu, clang)
} else if target.contains("cloudabi") {
format!("{}-{}", target, traditional)
} else if target == "wasm32-wasi"
Expand Down Expand Up @@ -2722,3 +2713,75 @@ fn command_add_output_file(
cmd.arg("-o").arg(&dst);
}
}

// Use by default minimum available API level
// See note about naming here
// https://android.googlesource.com/platform/ndk/+/refs/heads/ndk-release-r21/docs/BuildSystemMaintainers.md#Clang
static NEW_STANDALONE_ANDROID_COMPILERS: [&str; 4] = [
"aarch64-linux-android21-clang",
"armv7a-linux-androideabi16-clang",
"i686-linux-android16-clang",
"x86_64-linux-android21-clang",
Dushistov marked this conversation as resolved.
Show resolved Hide resolved
];

// New "standalone" C/C++ cross-compiler executables from recent Android NDK
// are just shell scripts that call main clang binary (from Android NDK) with
// proper `--target` argument.
//
// For example, armv7a-linux-androideabi16-clang passes
// `--target=armv7a-linux-androideabi16` to clang.
// So to construct proper command line check if
// `--target` argument would be passed or not to clang
fn android_clang_compiler_uses_target_arg_internally(clang_path: &Path) -> bool {
NEW_STANDALONE_ANDROID_COMPILERS.iter().any(|x| {
let x: &OsStr = x.as_ref();
x == clang_path.as_os_str()
})
}

fn autodetect_android_compiler(target: &str, host: &str, gnu: &str, clang: &str) -> String {
let new_clang_key = match target {
"aarch64-linux-android" => Some("aarch64"),
"armv7-linux-androideabi" => Some("armv7a"),
"i686-linux-android" => Some("i686"),
"x86_64-linux-android" => Some("x86_64"),
_ => None,
};

let new_clang = new_clang_key
.map(|key| {
NEW_STANDALONE_ANDROID_COMPILERS
.iter()
.find(|x| x.starts_with(key))
})
.unwrap_or(None);

if let Some(new_clang) = new_clang {
if Command::new(new_clang).output().is_ok() {
return (*new_clang).into();
}
}

let target = target
.replace("armv7neon", "arm")
.replace("armv7", "arm")
.replace("thumbv7neon", "arm")
.replace("thumbv7", "arm");
Copy link
Member

Choose a reason for hiding this comment

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

FWIW this normalization isn't used when trying to find a new compiler, could this be refactored a bit such that these rustc targets would still find the armv7a android compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally did not touch this code. Tier 1,2 platforms have only linux+android support. So it is hard test for Windows and MacOS, but code bellow works with windows, so I avoid changing anything that I can not test.

Plus I don't think that this is refactoring is possible. IMHO new target naming that include android API level was introduced recently, so if somebody do not want update Android NDK and uses old clang it is easy enough to break things for them.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that the armv7-linux-androideabi target, if folks are using it, will not hit the new clang compiler when it very likely should. I don't think there's a lot of risk here, it's basically just moving this let target block up to the top of the function instead of having it at the bottom

Copy link
Contributor Author

@Dushistov Dushistov May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My point is that the armv7-linux-androideabi target, if folks are using it, will not hit the new
clang compiler when it very likely should.

@alexcrichton, not sure that I understand you. armv7-linux-androideabi mapped to Some("armv7a") and then to armv7a-linux-androideabi16-clang call.
So armv7-linux-androideabi hit the new clang compiler, it is my main testing configuration,
I built all my code with cargo build --target=armv7-linux-androideabi and ran on android device before submitting this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I think I was misreading a bit. In any case we can always tweak this later!

let gnu_compiler = format!("{}-{}", target, gnu);
let clang_compiler = format!("{}-{}", target, clang);

// On Windows, the Android clang compiler is provided as a `.cmd` file instead
// of a `.exe` file. `std::process::Command` won't run `.cmd` files unless the
// `.cmd` is explicitly appended to the command name, so we do that here.
let clang_compiler_cmd = format!("{}-{}.cmd", target, clang);

// Check if gnu compiler is present
// if not, use clang
if Command::new(&gnu_compiler).output().is_ok() {
gnu_compiler
} else if host.contains("windows") && Command::new(&clang_compiler_cmd).output().is_ok() {
clang_compiler_cmd
} else {
clang_compiler
}
}