From 7d969937971c681f9817462bfafb243428d23ff0 Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Thu, 18 May 2023 20:12:57 +0100 Subject: [PATCH] Use cargo-ndk as a _LINKER wrapper to pass --target to Clang This is an alternative workaround for #92 that's similar to the solution used in ndk-build except that the `--target=` argument for Clang is injected by using cargo-ndk as a linker wrapper instead of trying to modify CARGO_ENCODED_RUSTFLAGS. It turned out to be practically impossible to be able to reliably set CARGO_ENCODED_RUSTFLAGS without risking trampling over rustflags that might be configured via Cargo (for example `target..rustflags` are especially difficult to read outside of cargo itself) This approach avoids hitting the command line length limitations of the current workaround and avoids needing temporary hard links or copying the cargo-ndk binary to the target/ directory. Even though we've only seen quoting issues with the Windows `.cmd` wrappers this change to avoid using the wrapper scripts is being applied consistently for all platforms. E.g. considering the upstream recommendation to avoid the wrapper scripts if possible: https://android-review.googlesource.com/c/platform/ndk/+/2134712 Fixes: #92 Fixes: #104 Addresses: https://github.com/android/ndk/issues/1856 --- Cargo.lock | 148 ++++---------------------------------------- Cargo.toml | 4 -- src/cargo.rs | 171 ++++++++++++++++++--------------------------------- src/main.rs | 79 +++++++++++------------- 4 files changed, 107 insertions(+), 295 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5dceb43..6dee7fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -43,10 +43,8 @@ dependencies = [ "log", "pathos", "serde", - "tempfile", "toml", "version_check", - "wargs", ] [[package]] @@ -111,7 +109,7 @@ checksum = "4bcfec3a70f97c962c307b2d2c56e358cf1d00b558d74262b5f929ee8cc7e73a" dependencies = [ "errno-dragonfly", "libc", - "windows-sys 0.48.0", + "windows-sys", ] [[package]] @@ -124,15 +122,6 @@ dependencies = [ "libc", ] -[[package]] -name = "fastrand" -version = "1.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e51093e27b0797c359783294ca4f0a911c270184cb10f85783b118614a1501be" -dependencies = [ - "instant", -] - [[package]] name = "fruity__bbqsrc" version = "0.2.0" @@ -174,15 +163,6 @@ version = "2.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9a3a5bfb195931eeb336b2a7b4d761daec841b97f947d34394601737a7bba5e4" -[[package]] -name = "instant" -version = "0.1.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7a5bbe824c507c5da5956355e86a746d82e0e1464f65d862cc5e71da70e94b2c" -dependencies = [ - "cfg-if", -] - [[package]] name = "io-lifetimes" version = "1.0.10" @@ -191,7 +171,7 @@ checksum = "9c66c74d2ae7e79a5a8f7ac924adbe38ee42a859c6539ad869eb51f0b52dc220" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.48.0", + "windows-sys", ] [[package]] @@ -213,7 +193,7 @@ dependencies = [ "hermit-abi", "io-lifetimes", "rustix", - "windows-sys 0.48.0", + "windows-sys", ] [[package]] @@ -318,15 +298,6 @@ dependencies = [ "proc-macro2", ] -[[package]] -name = "redox_syscall" -version = "0.3.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "567664f262709473930a4bf9e51bf2ebf3348f2e748ccc50dea20646858f8f29" -dependencies = [ - "bitflags", -] - [[package]] name = "regex" version = "1.8.1" @@ -355,7 +326,7 @@ dependencies = [ "io-lifetimes", "libc", "linux-raw-sys", - "windows-sys 0.48.0", + "windows-sys", ] [[package]] @@ -432,19 +403,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "tempfile" -version = "3.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b9fbec84f381d5795b08656e4912bec604d162bff9291d6189a78f4c8ab87998" -dependencies = [ - "cfg-if", - "fastrand", - "redox_syscall", - "rustix", - "windows-sys 0.45.0", -] - [[package]] name = "termcolor" version = "1.2.0" @@ -495,28 +453,12 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ca61eb27fa339aa08826a29f03e87b99b4d8f0fc2255306fd266bb1b6a9de498" -[[package]] -name = "utfx" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "133bf74f01486773317ddfcde8e2e20d2933cc3b68ab797e5d718bef996a81de" - [[package]] name = "version_check" version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" -[[package]] -name = "wargs" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07ce481e3a67bb5992990bb381bcf66e46ad94d214b0d151d3f970e39d25bd95" -dependencies = [ - "utfx", - "windows-sys 0.48.0", -] - [[package]] name = "winapi" version = "0.3.9" @@ -557,37 +499,13 @@ dependencies = [ "winapi", ] -[[package]] -name = "windows-sys" -version = "0.45.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" -dependencies = [ - "windows-targets 0.42.2", -] - [[package]] name = "windows-sys" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "677d2418bec65e3338edb076e806bc1ec15693c5d0104683f2efe857f61056a9" dependencies = [ - "windows-targets 0.48.0", -] - -[[package]] -name = "windows-targets" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e5180c00cd44c9b1c88adb3693291f1cd93605ded80c250a75d472756b4d071" -dependencies = [ - "windows_aarch64_gnullvm 0.42.2", - "windows_aarch64_msvc 0.42.2", - "windows_i686_gnu 0.42.2", - "windows_i686_msvc 0.42.2", - "windows_x86_64_gnu 0.42.2", - "windows_x86_64_gnullvm 0.42.2", - "windows_x86_64_msvc 0.42.2", + "windows-targets", ] [[package]] @@ -596,93 +514,51 @@ version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7b1eb6f0cd7c80c79759c929114ef071b87354ce476d9d94271031c0497adfd5" dependencies = [ - "windows_aarch64_gnullvm 0.48.0", - "windows_aarch64_msvc 0.48.0", - "windows_i686_gnu 0.48.0", - "windows_i686_msvc 0.48.0", - "windows_x86_64_gnu 0.48.0", - "windows_x86_64_gnullvm 0.48.0", - "windows_x86_64_msvc 0.48.0", + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", ] -[[package]] -name = "windows_aarch64_gnullvm" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "597a5118570b68bc08d8d59125332c54f1ba9d9adeedeef5b99b02ba2b0698f8" - [[package]] name = "windows_aarch64_gnullvm" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "91ae572e1b79dba883e0d315474df7305d12f569b400fcf90581b06062f7e1bc" -[[package]] -name = "windows_aarch64_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e08e8864a60f06ef0d0ff4ba04124db8b0fb3be5776a5cd47641e942e58c4d43" - [[package]] name = "windows_aarch64_msvc" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b2ef27e0d7bdfcfc7b868b317c1d32c641a6fe4629c171b8928c7b08d98d7cf3" -[[package]] -name = "windows_i686_gnu" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c61d927d8da41da96a81f029489353e68739737d3beca43145c8afec9a31a84f" - [[package]] name = "windows_i686_gnu" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "622a1962a7db830d6fd0a69683c80a18fda201879f0f447f065a3b7467daa241" -[[package]] -name = "windows_i686_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "44d840b6ec649f480a41c8d80f9c65108b92d89345dd94027bfe06ac444d1060" - [[package]] name = "windows_i686_msvc" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4542c6e364ce21bf45d69fdd2a8e455fa38d316158cfd43b3ac1c5b1b19f8e00" -[[package]] -name = "windows_x86_64_gnu" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8de912b8b8feb55c064867cf047dda097f92d51efad5b491dfb98f6bbb70cb36" - [[package]] name = "windows_x86_64_gnu" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ca2b8a661f7628cbd23440e50b05d705db3686f894fc9580820623656af974b1" -[[package]] -name = "windows_x86_64_gnullvm" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26d41b46a36d453748aedef1486d5c7a85db22e56aff34643984ea85514e94a3" - [[package]] name = "windows_x86_64_gnullvm" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7896dbc1f41e08872e9d5e8f8baa8fdd2677f29468c4e156210174edc7f7b953" -[[package]] -name = "windows_x86_64_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9aec5da331524158c6d1a4ac0ab1541149c0b9505fde06423b02f5ef0106b9f0" - [[package]] name = "windows_x86_64_msvc" version = "0.48.0" diff --git a/Cargo.toml b/Cargo.toml index a6fe1c4..fa3be00 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,3 @@ pathos = "0.3.0" serde = { version = "1.0.152", features = ["derive"] } toml = "0.5.10" version_check = "0.9.4" - -[target.'cfg(windows)'.dependencies] -tempfile = "3.5.0" -wargs = "0.1.0" diff --git a/src/cargo.rs b/src/cargo.rs index 31a87c5..b05ed0a 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -14,31 +14,16 @@ const ARCH: &str = "linux-x86_64"; #[cfg(target_os = "windows")] const ARCH: &str = "windows-x86_64"; -#[cfg(target_os = "windows")] -const CLANG_EXT: &str = ".cmd"; -#[cfg(not(target_os = "windows"))] -const CLANG_EXT: &str = ""; - -fn clang_suffix(triple: &str, arch: &str, platform: u8, postfix: &str) -> PathBuf { - let tool_triple = match triple { +fn clang_target(rust_target: &str, api_level: u8) -> String { + let target = match rust_target { "arm-linux-androideabi" => "armv7a-linux-androideabi", "armv7-linux-androideabi" => "armv7a-linux-androideabi", - _ => triple, + _ => rust_target, }; - - [ - "toolchains", - "llvm", - "prebuilt", - arch, - "bin", - &format!("{tool_triple}{platform}-clang{postfix}{CLANG_EXT}"), - ] - .iter() - .collect() + format!("--target={target}{api_level}") } -fn ndk23_tool(arch: &str, tool: &str) -> PathBuf { +fn ndk_tool(arch: &str, tool: &str) -> PathBuf { ["toolchains", "llvm", "prebuilt", arch, "bin", tool] .iter() .collect() @@ -54,13 +39,6 @@ fn cargo_env_target_cfg(triple: &str, key: &str) -> String { format!("CARGO_TARGET_{}_{}", &triple.replace('-', "_"), key).to_uppercase() } -#[cfg(windows)] -fn cargo_target_dir(out_dir: &Utf8PathBuf) -> PathBuf { - std::env::var("CARGO_TARGET_DIR") - .map(PathBuf::from) - .unwrap_or_else(|_| out_dir.clone().into_std_path_buf()) -} - #[allow(clippy::too_many_arguments)] pub(crate) fn run( dir: &Path, @@ -80,39 +58,58 @@ pub(crate) fn run( std::process::exit(1); } - let target_linker = ndk_home.join(clang_suffix(triple, ARCH, platform, "")); - let target_cxx = ndk_home.join(clang_suffix(triple, ARCH, platform, "++")); - let target_sysroot = ndk_home.join(sysroot_suffix(ARCH)); - let target_ar = ndk_home.join(ndk23_tool(ARCH, "llvm-ar")); - let target_ranlib = ndk_home.join(ndk23_tool(ARCH, "llvm-ranlib")); - + let clang_target = clang_target(triple, platform); + + // Note: considering that there is an upstream quoting bug in the clang .cmd + // wrappers on Windows we intentionally avoid any wrapper scripts and + // instead pass a `--target=` argument to clang by using + // cargo-ndk itself as a linker wrapper. + // + // See: https://github.com/android/ndk/issues/1856 + // + // Note: it's not possible to pass `-Clink-arg=` arguments via + // CARGO_ENCODED_RUSTFLAGS because that could trample rustflags that are + // configured for the project and there's no practical way to read all + // user-configured rustflags from outside of cargo itself. + // + let self_path = std::fs::canonicalize(std::env::args().next().unwrap()) + .expect("Failed to canonicalize absolute path to cargo-ndk"); + + // Environment variables for the `cc` crate let cc_key = format!("CC_{}", &triple); - let ar_key = format!("AR_{}", &triple); + let cflags_key = format!("CFLAGS_{}", &triple); let cxx_key = format!("CXX_{}", &triple); + let cxxflags_key = format!("CXXFLAGS_{}", &triple); + let ar_key = format!("AR_{}", &triple); let ranlib_key = format!("RANLIB_{}", &triple); + + // Environment variables for cargo + let cargo_ar_key = cargo_env_target_cfg(triple, "ar"); + let cargo_linker_key = cargo_env_target_cfg(triple, "linker"); let bindgen_clang_args_key = format!("BINDGEN_EXTRA_CLANG_ARGS_{}", &triple.replace('-', "_")); - let cargo_bin = env::var("CARGO").unwrap_or_else(|_| "cargo".into()); - log::debug!("cargo: {}", &cargo_bin); - log::debug!("{}={}", &ar_key, &target_ar.display()); - log::debug!("{}={}", &cc_key, &target_linker.display()); - log::debug!("{}={}", &cxx_key, &target_cxx.display()); - log::debug!("{}={}", &ranlib_key, &target_ranlib.display()); - log::debug!( - "{}={}", - cargo_env_target_cfg(triple, "ar"), - &target_ar.display() - ); - log::debug!( - "{}={}", - cargo_env_target_cfg(triple, "linker"), - &target_linker.display() - ); - log::debug!( - "{}={}", - &bindgen_clang_args_key, - &std::env::var(&bindgen_clang_args_key).unwrap_or_default() - ); + let cargo_bin = env::var("CARGO").unwrap_or_else(|_| "cargo".into()); + let target_cc = ndk_home.join(ndk_tool(ARCH, "clang")); + let target_cflags = clang_target.clone(); + let target_cxx = ndk_home.join(ndk_tool(ARCH, "clang++")); + let target_cxxflags = clang_target.clone(); + let target_sysroot = ndk_home.join(sysroot_suffix(ARCH)); + let target_ar = ndk_home.join(ndk_tool(ARCH, "llvm-ar")); + let target_ranlib = ndk_home.join(ndk_tool(ARCH, "llvm-ranlib")); + let target_linker = self_path; + let bindgen_clang_args = std::env::var(&bindgen_clang_args_key).unwrap_or_default(); + + log::debug!("{cc_key}={target_cc:?}"); + log::debug!("{cflags_key}={target_cflags}"); + log::debug!("{cxx_key}={target_cxx:?}"); + log::debug!("{cxxflags_key}={target_cxxflags}"); + log::debug!("{ar_key}={target_ar:?}"); + log::debug!("{ranlib_key}={target_ranlib:?}"); + + log::debug!("cargo: {cargo_bin}"); + log::debug!("{cargo_ar_key}={target_ar:?}"); + log::debug!("{cargo_linker_key}={target_linker:?}"); + log::debug!("{bindgen_clang_args_key}={bindgen_clang_args}"); log::debug!("Args: {:?}", &cargo_args); // Insert Cargo arguments before any `--` arguments. @@ -126,66 +123,16 @@ pub(crate) fn run( let mut cargo_cmd = Command::new(cargo_bin); - #[cfg(not(windows))] cargo_cmd .current_dir(dir) - .env(&ar_key, &target_ar) - .env(&cc_key, &target_linker) + .env(&cc_key, &target_cc) .env(&cxx_key, &target_cxx) + .env(&ar_key, &target_ar) .env(&ranlib_key, &target_ranlib) - .env(cargo_env_target_cfg(triple, "ar"), &target_ar) - .env(cargo_env_target_cfg(triple, "linker"), &target_linker); - - #[cfg(windows)] - let cargo_ndk_target_dir = - cargo_target_dir(out_dir).join(format!(".cargo-ndk-{}", env!("CARGO_PKG_VERSION"))); - - #[cfg(windows)] - { - let main = std::env::args().next().unwrap(); - if !cargo_ndk_target_dir.exists() { - std::fs::create_dir_all(&cargo_ndk_target_dir).unwrap(); - } - - for f in ["ar", "cc", "cxx", "ranlib", "triple-ar", "triple-linker"] { - let executable = cargo_ndk_target_dir.join(f).with_extension("exe"); - if executable.exists() { - continue; - } - - match std::fs::hard_link(&main, &executable) - .or_else(|_| std::fs::copy(&main, executable).map(|_| ())) - { - Ok(_) => {} - Err(e) => { - log::error!("Failed to create hardlink or copy for '{f}'."); - log::error!("{}", e); - std::process::exit(1); - } - } - } - - cargo_cmd - .current_dir(dir) - .env(&ar_key, cargo_ndk_target_dir.join("ar.exe")) - .env(&cc_key, cargo_ndk_target_dir.join("cc.exe")) - .env(&cxx_key, cargo_ndk_target_dir.join("cxx.exe")) - .env(&ranlib_key, cargo_ndk_target_dir.join("ranlib.exe")) - .env( - cargo_env_target_cfg(triple, "ar"), - cargo_ndk_target_dir.join("triple-ar.exe"), - ) - .env( - cargo_env_target_cfg(triple, "linker"), - cargo_ndk_target_dir.join("triple-linker.exe"), - ) - .env("CARGO_NDK_AR", &target_ar) - .env("CARGO_NDK_CC", &target_linker) - .env("CARGO_NDK_CXX", &target_cxx) - .env("CARGO_NDK_RANLIB", &target_ranlib) - .env("CARGO_NDK_TRIPLE_AR", &target_ar) - .env("CARGO_NDK_TRIPLE_LINKER", &target_linker); - } + .env(cargo_ar_key, &target_ar) + .env(cargo_linker_key, &target_linker) + .env("_CARGO_NDK_LINK_TARGET", &clang_target) // Recognized by main() so we know when we're acting as a wrapper + .env("_CARGO_NDK_LINK_CLANG", &target_cc); let extra_include = format!("{}/usr/include/{}", &target_sysroot.display(), triple); if bindgen { @@ -218,7 +165,7 @@ pub(crate) fn run( } pub(crate) fn strip(ndk_home: &Path, bin_path: &Path) -> std::process::ExitStatus { - let target_strip = ndk_home.join(ndk23_tool(ARCH, "llvm-strip")); + let target_strip = ndk_home.join(ndk_tool(ARCH, "llvm-strip")); log::debug!("strip: {}", &target_strip.display()); diff --git a/src/main.rs b/src/main.rs index 77a63bd..45aa2ee 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,28 +5,40 @@ mod cargo; mod cli; mod meta; -#[cfg(windows)] -fn args_hack(cmd: &str) -> anyhow::Result<()> { - use std::os::windows::process::CommandExt; - - let args = wargs::command_line_to_argv(None) - .skip(1) - .collect::>(); - - let mut process = std::process::Command::new(cmd) - .raw_arg(args.join(" ")) - .spawn()?; - - let status = process.wait()?; - - if status.success() { - Ok(()) - } else { - Err(anyhow::anyhow!( - "Errored with code {}", - status.code().unwrap() - )) - } +/// We are avoiding using the Clang wrapper scripts in the NDK because they have +/// a quoting bug on Windows (https://github.com/android/ndk/issues/1856) and +/// for consistency on other platforms, considering it's now generally +/// recommended to avoid relying on these wrappers: +/// https://android-review.googlesource.com/c/platform/ndk/+/2134712 +/// +/// Instead; we set cargo-ndk up as our rustc `_LINKER` as a way to be able to pass +/// --target= +/// +/// We do it this way because we can't modify rustflags before running `cargo +/// build` without potentially trampling over flags that are configured via +/// Cargo. +fn clang_linker_wrapper() -> ! { + let mut args = std::env::args_os(); + let _first = args.next().unwrap(); // ignore arg[0] + let clang = std::env::var("_CARGO_NDK_LINK_CLANG") + .expect("cargo-ndk rustc linker: didn't find _CARGO_NDK_LINK_CLANG env var"); + let target = std::env::var("_CARGO_NDK_LINK_TARGET") + .expect("cargo-ndk rustc linker: didn't find _CARGO_NDK_LINK_TARGET env var"); + + let mut child = std::process::Command::new(&clang) + .arg(target) + .args(args) + .spawn() + .unwrap_or_else(|err| { + eprintln!("cargo-ndk: Failed to spawn {clang:?} as linker: {err}"); + std::process::exit(1) + }); + let status = child.wait().unwrap_or_else(|err| { + eprintln!("cargo-ndk (as linker): Failed to wait for {clang:?} to complete: {err}"); + std::process::exit(1); + }); + + std::process::exit(status.code().unwrap_or(1)) } fn main() -> anyhow::Result<()> { @@ -37,27 +49,8 @@ fn main() -> anyhow::Result<()> { exit(1); } - #[cfg(windows)] - { - let main_arg = std::env::args().next().unwrap(); - let main_arg = std::path::Path::new(&main_arg) - .file_stem() - .unwrap() - .to_str() - .unwrap(); - - if main_arg != "cargo-ndk" { - let maybe = main_arg.to_uppercase().replace('-', "_"); - let app = match std::env::var(format!("CARGO_NDK_{maybe}")) { - Ok(cmd) => cmd, - Err(err) => { - log::error!("{}", err); - panic!("{}", err); - } - }; - log::debug!("Running command: {app}"); - return args_hack(&app); - } + if std::env::var("_CARGO_NDK_LINK_TARGET").is_ok() { + clang_linker_wrapper(); } let args = std::env::args().skip(2).collect::>();