Skip to content

Commit

Permalink
ndk-build: Invoke clang directly instead of through wrapper scripts
Browse files Browse the repository at this point in the history
The NDK's wrapper scripts for `clang`/`clang++` currently only, and will
only ever pass `--target` to the compiler.  That is something we can do
ourselves, especially now that we're already updating `RUSTFLAGS`
anyway.

[Upstream even clarified] that it is recommended to pass `--target`
yourself instead of incurring extra overhead while going through the
wrapper scripts (especially costly on Windows), further guaranteeing
that we won't miss out on any flags possibly being added to the wrapper
scripts in the future.

[Upstream even clarified]: https://r.android.com/2134712
  • Loading branch information
MarijnS95 committed Jul 4, 2022
1 parent b3c21ee commit bc6eb22
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 49 deletions.
1 change: 1 addition & 0 deletions ndk-build/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Unreleased

- Allow NDK r23 `-lgcc` workaround to work for target directories containing spaces. ([#298](https://github.com/rust-windowing/android-ndk-rs/pull/298))
- Invoke `clang` directly instead of through the NDK's wrapper scripts. ([#306](https://github.com/rust-windowing/android-ndk-rs/pull/306))

# 0.6.0 (2022-06-11)

Expand Down
2 changes: 1 addition & 1 deletion ndk-build/src/apk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl<'a> UnalignedApk<'a> {
search_paths: &[&Path],
) -> Result<(), NdkError> {
let abi_dir = path.join(target.android_abi());
for entry in fs::read_dir(&abi_dir).map_err(|e| NdkError::IoPathError(e, abi_dir))? {
for entry in fs::read_dir(&abi_dir).map_err(|e| NdkError::IoPathError(abi_dir, e))? {
let entry = entry?;
let path = entry.path();
if path.extension() == Some(OsStr::new("so")) {
Expand Down
59 changes: 39 additions & 20 deletions ndk-build/src/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,35 @@ pub fn cargo_ndk(
target_dir: impl AsRef<Path>,
) -> Result<Command, NdkError> {
let triple = target.rust_triple();
let clang_target = format!("--target={}{}", target.ndk_llvm_triple(), sdk_version);
let mut cargo = Command::new("cargo");

let (clang, clang_pp) = ndk.clang(target, sdk_version)?;
// Read initial RUSTFLAGS
let mut rustflags = match std::env::var("CARGO_ENCODED_RUSTFLAGS") {
Ok(val) => val,
Err(std::env::VarError::NotPresent) => "".to_string(),
Err(std::env::VarError::NotUnicode(_)) => {
panic!("RUSTFLAGS environment variable contains non-unicode characters")
}
};

let (clang, clang_pp) = ndk.clang()?;

// Configure cross-compiler for `cc` crate
// https://github.com/rust-lang/cc-rs#external-configuration-via-environment-variables
cargo.env(format!("CC_{}", triple), &clang);
cargo.env(format!("CFLAGS_{}", triple), &clang_target);
cargo.env(format!("CXX_{}", triple), &clang_pp);
cargo.env(format!("CXXFLAGS_{}", triple), &clang_target);

// Configure LINKER for `rustc`
// https://doc.rust-lang.org/beta/cargo/reference/environment-variables.html#configuration-environment-variables
cargo.env(cargo_env_target_cfg("LINKER", triple), &clang);
if !rustflags.is_empty() {
rustflags.push('\x1f');
}
rustflags.push_str("-Clink-arg=");
rustflags.push_str(&clang_target);

let ar = ndk.toolchain_bin("ar", target)?;
cargo.env(format!("AR_{}", triple), &ar);
Expand All @@ -32,32 +55,28 @@ pub fn cargo_ndk(
let cargo_apk_link_dir = target_dir
.as_ref()
.join("cargo-apk-temp-extra-link-libraries");
std::fs::create_dir_all(&cargo_apk_link_dir)?;
std::fs::write(cargo_apk_link_dir.join("libgcc.a"), "INPUT(-lunwind)")
.expect("Failed to write");
std::fs::create_dir_all(&cargo_apk_link_dir)
.map_err(|e| NdkError::IoPathError(cargo_apk_link_dir.clone(), e))?;
let libgcc = cargo_apk_link_dir.join("libgcc.a");
std::fs::write(&libgcc, "INPUT(-lunwind)").map_err(|e| NdkError::IoPathError(libgcc, e))?;

// cdylibs in transitive dependencies still get built and also need this
// workaround linker flag, yet arguments passed to `cargo rustc` are only
// forwarded to the final compiler invocation rendering our workaround ineffective.
// The cargo page documenting this discrepancy (https://doc.rust-lang.org/cargo/commands/cargo-rustc.html)
// suggests to resort to RUSTFLAGS, which are updated below:
let mut rustflags = match std::env::var("CARGO_ENCODED_RUSTFLAGS") {
Ok(val) => val,
Err(std::env::VarError::NotPresent) => "".to_string(),
Err(std::env::VarError::NotUnicode(_)) => {
panic!("RUSTFLAGS environment variable contains non-unicode characters")
}
};
if !rustflags.is_empty() {
rustflags.push('\x1f');
}
rustflags += "-L\x1f";
rustflags += cargo_apk_link_dir
.to_str()
.expect("Target dir must be valid UTF-8");
cargo.env("CARGO_ENCODED_RUSTFLAGS", rustflags);
// suggests to resort to RUSTFLAGS.
// Note that `rustflags` will never be empty because of an unconditional `.push_str` above,
// so we can safely start with appending \x1f here.
rustflags.push_str("\x1f-L\x1f");
rustflags.push_str(
cargo_apk_link_dir
.to_str()
.expect("Target dir must be valid UTF-8"),
);
}

cargo.env("CARGO_ENCODED_RUSTFLAGS", rustflags);

Ok(cargo)
}

Expand Down
4 changes: 2 additions & 2 deletions ndk-build/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ pub enum NdkError {
UnsupportedHost(String),
#[error(transparent)]
Io(#[from] IoError),
#[error("{0:?}: `{1}`")]
IoPathError(#[source] IoError, PathBuf),
#[error("IoError on `{0:?}`: {1}")]
IoPathError(PathBuf, #[source] IoError),
#[error("Invalid semver")]
InvalidSemver,
#[error("Command `{}` had a non-zero exit code.", format!("{:?}", .0).replace('"', ""))]
Expand Down
28 changes: 14 additions & 14 deletions ndk-build/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
macro_rules! bin {
($bin:expr) => {{
#[cfg(not(target_os = "windows"))]
let bin = $bin;
#[cfg(target_os = "windows")]
let bin = concat!($bin, ".exe");
bin
}};
($bin:expr) => {
if cfg!(target_os = "windows") {
concat!($bin, ".exe")
} else {
$bin
}
};
}

macro_rules! bat {
($bat:expr) => {{
#[cfg(not(target_os = "windows"))]
let bat = $bat;
#[cfg(target_os = "windows")]
let bat = concat!($bat, ".bat");
bat
}};
($bat:expr) => {
if cfg!(target_os = "windows") {
concat!($bat, ".bat")
} else {
$bat
}
};
}

pub mod apk;
Expand Down
25 changes: 13 additions & 12 deletions ndk-build/src/ndk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,21 +228,21 @@ impl Ndk {
Ok(toolchain_dir)
}

pub fn clang(&self, target: Target, platform: u32) -> Result<(PathBuf, PathBuf), NdkError> {
#[cfg(target_os = "windows")]
let ext = ".cmd";
#[cfg(not(target_os = "windows"))]
let ext = "";
pub fn clang(&self) -> Result<(PathBuf, PathBuf), NdkError> {
let ext = if cfg!(target_os = "windows") {
"exe"
} else {
""
};

let bin_name = format!("{}{}-clang", target.ndk_llvm_triple(), platform);
let bin_path = self.toolchain_dir()?.join("bin");

let clang = bin_path.join(format!("{}{}", &bin_name, ext));
let clang = bin_path.join("clang").with_extension(ext);
if !clang.exists() {
return Err(NdkError::PathNotFound(clang));
}

let clang_pp = bin_path.join(format!("{}++{}", &bin_name, ext));
let clang_pp = bin_path.join("clang++").with_extension(ext);
if !clang_pp.exists() {
return Err(NdkError::PathNotFound(clang_pp));
}
Expand All @@ -251,10 +251,11 @@ impl Ndk {
}

pub fn toolchain_bin(&self, name: &str, target: Target) -> Result<PathBuf, NdkError> {
#[cfg(target_os = "windows")]
let ext = ".exe";
#[cfg(not(target_os = "windows"))]
let ext = "";
let ext = if cfg!(target_os = "windows") {
".exe"
} else {
""
};

let toolchain_path = self.toolchain_dir()?.join("bin");

Expand Down

0 comments on commit bc6eb22

Please sign in to comment.