From 1a183cd67308a033c150eeca4809e17855716dc7 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Mon, 12 Dec 2022 15:01:49 -0800 Subject: [PATCH 1/3] Expose get_archiver and get_ranlib This PR exposes cc's inferred archiver through a similar mechanism as `get_compiler`: with `get_archiver`/`try_get_archiver`. As part of this, I also realized that cc wouldn't pick up `ARFLAGS`, so I added support for that. Through that, I also realized that there was currently one place where we didn't respect `Build::ar_flags` when ar was called, so I fixed that too. Since ranlib is exposed more or less exactly like ar, I also added a `get_ranlib`/`try_get_ranlib` combination. This is, in part, to enable `openssl-src` to move to re-use `cc`'s AR (and now RANLIB) detection rather than rolling its own. See alexcrichton/openssl-src-rs#164. Note that I didn't re-use `Tool` for the return value for these getter functions, and instead just exposed `Command` directly. `Tool` has a number of relatively compiler-specific things, so it felt wrong to use. One important caveat to this is that `Command::get_program` was only added in Rust 1.57.0, so users on older versions of Rust won't have a way to recover the inferred `ar` program path, but that feels like it's probably okay given that we didn't even expose `get_archive` until now. --- src/lib.rs | 259 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 189 insertions(+), 70 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1ebd2cc7a..4b5bff3a4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,6 +115,7 @@ pub struct Build { env: Vec<(OsString, OsString)>, compiler: Option, archiver: Option, + ranlib: Option, cargo_metadata: bool, link_lib_modifiers: Vec, pic: Option, @@ -320,6 +321,7 @@ impl Build { env: Vec::new(), compiler: None, archiver: None, + ranlib: None, cargo_metadata: true, link_lib_modifiers: Vec::new(), pic: None, @@ -916,6 +918,17 @@ impl Build { self.archiver = Some(archiver.as_ref().to_owned()); self } + + /// Configures the tool used to index archives. + /// + /// This option is automatically determined from the target platform or a + /// number of environment variables, so it's not required to call this + /// function. + pub fn ranlib>(&mut self, ranlib: P) -> &mut Build { + self.ranlib = Some(ranlib.as_ref().to_owned()); + self + } + /// Define whether metadata should be emitted for cargo allowing it to /// automatically link the binary. Defaults to `true`. /// @@ -2109,9 +2122,6 @@ impl Build { let mut out = OsString::from("-out:"); out.push(dst); cmd.arg(out).arg("-nologo"); - for flag in self.ar_flags.iter() { - cmd.arg(flag); - } // If the library file already exists, add the library name // as an argument to let lib.exe know we are appending the objs. if dst.exists() { @@ -2145,9 +2155,6 @@ impl Build { // In any case if this doesn't end up getting read, it shouldn't // cause that many issues! ar.env("ZERO_AR_DATE", "1"); - for flag in self.ar_flags.iter() { - ar.arg(flag); - } run(ar.arg("cq").arg(dst).args(objs), &cmd)?; } @@ -2644,80 +2651,192 @@ impl Build { } fn get_ar(&self) -> Result<(Command, String), Error> { - if let Some(ref p) = self.archiver { - let name = p.file_name().and_then(|s| s.to_str()).unwrap_or("ar"); - return Ok((self.cmd(p), name.to_string())); + let cmd = self.try_get_archiver()?; + let name = Path::new(cmd.get_program()) + .file_name() + .ok_or_else(|| Error::new(ErrorKind::IOError, "Failed to get archiver path."))? + .to_string_lossy() + .into_owned(); + Ok((cmd, name)) + } + + /// Get the archiver (ar) that's in use for this configuration. + /// + /// You can use [`Command::get_program`] to get just the path to the command. + /// + /// This method will take into account all configuration such as debug + /// information, optimization level, include directories, defines, etc. + /// Additionally, the compiler binary in use follows the standard + /// conventions for this path, e.g. looking at the explicitly set compiler, + /// environment variables (a number of which are inspected here), and then + /// falling back to the default configuration. + /// + /// # Panics + /// + /// Panics if an error occurred while determining the architecture. + pub fn get_archiver(&self) -> Command { + match self.try_get_archiver() { + Ok(tool) => tool, + Err(e) => fail(&e.message), } - if let Ok(p) = self.get_var("AR") { - return Ok((self.cmd(&p), p)); + } + + /// Get the archiver that's in use for this configuration. + /// + /// This will return a result instead of panicing; + /// see [`get_archiver()`] for the complete description. + pub fn try_get_archiver(&self) -> Result { + let mut cmd = self.get_base_archiver()?; + cmd.args(self.envflags("ARFLAGS")); + for flag in &self.ar_flags { + cmd.arg(flag); } - let target = self.get_target()?; - let default_ar = "ar".to_string(); - let program = if target.contains("android") { - format!("{}-ar", target.replace("armv7", "arm")) - } else if target.contains("emscripten") { - // Windows use bat files so we have to be a bit more specific - if cfg!(windows) { - let mut cmd = self.cmd("cmd"); - cmd.arg("/c").arg("emar.bat"); - return Ok((cmd, "emar.bat".to_string())); - } + Ok(cmd) + } - "emar".to_string() - } else if target.contains("msvc") { - let compiler = self.get_base_compiler()?; - let mut lib = String::new(); - if compiler.family == (ToolFamily::Msvc { clang_cl: true }) { - // See if there is 'llvm-lib' next to 'clang-cl' - // Another possibility could be to see if there is 'clang' - // next to 'clang-cl' and use 'search_programs()' to locate - // 'llvm-lib'. This is because 'clang-cl' doesn't support - // the -print-search-dirs option. - if let Some(mut cmd) = which(&compiler.path) { - cmd.pop(); - cmd.push("llvm-lib.exe"); - if let Some(llvm_lib) = which(&cmd) { - lib = llvm_lib.to_str().unwrap().to_owned(); + fn get_base_archiver(&self) -> Result { + if let Some(ref a) = self.archiver { + return Ok(self.cmd(a)); + } + + self.get_base_archiver_variant("AR", "ar") + } + + /// Get the ranlib that's in use for this configuration. + /// + /// You can use [`Command::get_program`] to get just the path to the command. + /// + /// This method will take into account all configuration such as debug + /// information, optimization level, include directories, defines, etc. + /// Additionally, the compiler binary in use follows the standard + /// conventions for this path, e.g. looking at the explicitly set compiler, + /// environment variables (a number of which are inspected here), and then + /// falling back to the default configuration. + /// + /// # Panics + /// + /// Panics if an error occurred while determining the architecture. + pub fn get_ranlib(&self) -> Command { + match self.try_get_ranlib() { + Ok(tool) => tool, + Err(e) => fail(&e.message), + } + } + + /// Get the ranlib that's in use for this configuration. + /// + /// This will return a result instead of panicing; + /// see [`get_ranlib()`] for the complete description. + pub fn try_get_ranlib(&self) -> Result { + let mut cmd = self.get_base_ranlib()?; + cmd.args(self.envflags("RANLIBFLAGS")); + Ok(cmd) + } + + fn get_base_ranlib(&self) -> Result { + if let Some(ref r) = self.ranlib { + return Ok(self.cmd(r)); + } + + self.get_base_archiver_variant("RANLIB", "ranlib") + } + + fn get_base_archiver_variant(&self, env: &str, tool: &str) -> Result { + let target = self.get_target()?; + let tool_opt: Option = self + .env_tool(env) + .map(|(tool, _wrapper, args)| { + let mut cmd = self.cmd(tool); + cmd.args(args); + cmd + }) + .or_else(|| { + if target.contains("emscripten") { + // Windows use bat files so we have to be a bit more specific + if cfg!(windows) { + let mut cmd = self.cmd("cmd"); + cmd.arg("/c").arg(format!("em{}.bat", tool)); + Some(cmd) + } else { + Some(self.cmd(format!("em{}", tool))) } + } else { + None } - } - if lib.is_empty() { - lib = match windows_registry::find(&target, "lib.exe") { - Some(t) => return Ok((t, "lib.exe".to_string())), - None => "lib.exe".to_string(), - } - } - lib - } else if target.contains("illumos") { - // The default 'ar' on illumos uses a non-standard flags, - // but the OS comes bundled with a GNU-compatible variant. - // - // Use the GNU-variant to match other Unix systems. - "gar".to_string() - } else if self.get_host()? != target { - match self.prefix_for_target(&target) { - Some(p) => { - // GCC uses $target-gcc-ar, whereas binutils uses $target-ar -- try both. - // Prefer -ar if it exists, as builds of `-gcc-ar` have been observed to be - // outright broken (such as when targetting freebsd with `--disable-lto` - // toolchain where the archiver attempts to load the LTO plugin anyway but - // fails to find one). - let mut ar = default_ar; - for &infix in &["", "-gcc"] { - let target_ar = format!("{}{}-ar", p, infix); - if Command::new(&target_ar).output().is_ok() { - ar = target_ar; - break; + }); + + let default = tool.to_string(); + let tool = match tool_opt { + Some(t) => t, + None => { + if target.contains("android") { + self.cmd(format!("{}-{}", target.replace("armv7", "arm"), tool)) + } else if target.contains("msvc") { + // NOTE: There isn't really a ranlib on msvc, so arguably we should return + // `None` somehow here. But in general, callers will already have to be aware + // of not running ranlib on Windows anyway, so it feels okay to return lib.exe + // here. + + let compiler = self.get_base_compiler()?; + let mut lib = String::new(); + if compiler.family == (ToolFamily::Msvc { clang_cl: true }) { + // See if there is 'llvm-lib' next to 'clang-cl' + // Another possibility could be to see if there is 'clang' + // next to 'clang-cl' and use 'search_programs()' to locate + // 'llvm-lib'. This is because 'clang-cl' doesn't support + // the -print-search-dirs option. + if let Some(mut cmd) = which(&compiler.path) { + cmd.pop(); + cmd.push("llvm-lib.exe"); + if let Some(llvm_lib) = which(&cmd) { + lib = llvm_lib.to_str().unwrap().to_owned(); + } + } + } + + if lib.is_empty() { + match windows_registry::find(&target, "lib.exe") { + Some(t) => t, + None => self.cmd("lib.exe"), + } + } else { + self.cmd(lib) + } + } else if target.contains("illumos") { + // The default 'ar' on illumos uses a non-standard flags, + // but the OS comes bundled with a GNU-compatible variant. + // + // Use the GNU-variant to match other Unix systems. + self.cmd(format!("g{}", tool)) + } else if self.get_host()? != target { + match self.prefix_for_target(&target) { + Some(p) => { + // GCC uses $target-gcc-ar, whereas binutils uses $target-ar -- try both. + // Prefer -ar if it exists, as builds of `-gcc-ar` have been observed to be + // outright broken (such as when targetting freebsd with `--disable-lto` + // toolchain where the archiver attempts to load the LTO plugin anyway but + // fails to find one). + // + // The same applies to ranlib. + let mut chosen = default; + for &infix in &["", "-gcc"] { + let target_p = format!("{}{}-{}", p, infix, tool); + if Command::new(&target_p).output().is_ok() { + chosen = target_p; + break; + } + } + self.cmd(chosen) } + None => self.cmd(default), } - ar + } else { + self.cmd(default) } - None => default_ar, } - } else { - default_ar }; - Ok((self.cmd(&program), program)) + + Ok(tool) } fn prefix_for_target(&self, target: &str) -> Option { From 7a0c2afeffa63e1679f58714f8d0e2e824687450 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Mon, 23 Jan 2023 09:55:10 -0800 Subject: [PATCH 2/3] Don't pass our own flags if ARFLAGS/ar_flag is used --- src/lib.rs | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4b5bff3a4..4e6fb2df3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2107,7 +2107,11 @@ impl Build { // Non-msvc targets (those using `ar`) need a separate step to add // the symbol table to archives since our construction command of // `cq` doesn't add it for us. - let (mut ar, cmd) = self.get_ar()?; + let (mut ar, cmd, _any_flags) = self.get_ar()?; + + // NOTE: We add `s` even if flags were passed using $ARFLAGS/ar_flag, because `s` + // here represents a _mode_, not an arbitrary flag. Further discussion of this choice + // can be seen in https://github.com/rust-lang/cc-rs/pull/763. run(ar.arg("s").arg(dst), &cmd)?; } @@ -2118,10 +2122,17 @@ impl Build { let target = self.get_target()?; if target.contains("msvc") { - let (mut cmd, program) = self.get_ar()?; + let (mut cmd, program, any_flags) = self.get_ar()?; + // NOTE: -out: here is an I/O flag, and so must be included even if $ARFLAGS/ar_flag is + // in use. -nologo on the other hand is just a regular flag, and one that we'll skip if + // the caller has explicitly dictated the flags they want. See + // https://github.com/rust-lang/cc-rs/pull/763 for further discussion. let mut out = OsString::from("-out:"); out.push(dst); - cmd.arg(out).arg("-nologo"); + cmd.arg(out); + if !any_flags { + cmd.arg("-nologo"); + } // If the library file already exists, add the library name // as an argument to let lib.exe know we are appending the objs. if dst.exists() { @@ -2130,7 +2141,7 @@ impl Build { cmd.args(objs); run(&mut cmd, &program)?; } else { - let (mut ar, cmd) = self.get_ar()?; + let (mut ar, cmd, _any_flags) = self.get_ar()?; // Set an environment variable to tell the OSX archiver to ensure // that all dates listed in the archive are zero, improving @@ -2155,6 +2166,10 @@ impl Build { // In any case if this doesn't end up getting read, it shouldn't // cause that many issues! ar.env("ZERO_AR_DATE", "1"); + + // NOTE: We add cq here regardless of whether $ARFLAGS/ar_flag have been used because + // it dictates the _mode_ ar runs in, which the setter of $ARFLAGS/ar_flag can't + // dictate. See https://github.com/rust-lang/cc-rs/pull/763 for further discussion. run(ar.arg("cq").arg(dst).args(objs), &cmd)?; } @@ -2650,14 +2665,14 @@ impl Build { } } - fn get_ar(&self) -> Result<(Command, String), Error> { - let cmd = self.try_get_archiver()?; + fn get_ar(&self) -> Result<(Command, String, bool), Error> { + let (cmd, any_flags) = self.try_get_archiver_and_flags()?; let name = Path::new(cmd.get_program()) .file_name() .ok_or_else(|| Error::new(ErrorKind::IOError, "Failed to get archiver path."))? .to_string_lossy() .into_owned(); - Ok((cmd, name)) + Ok((cmd, name, any_flags)) } /// Get the archiver (ar) that's in use for this configuration. @@ -2686,12 +2701,19 @@ impl Build { /// This will return a result instead of panicing; /// see [`get_archiver()`] for the complete description. pub fn try_get_archiver(&self) -> Result { + Ok(self.try_get_archiver_and_flags()?.0) + } + + fn try_get_archiver_and_flags(&self) -> Result<(Command, bool), Error> { let mut cmd = self.get_base_archiver()?; - cmd.args(self.envflags("ARFLAGS")); + let flags = self.envflags("ARFLAGS"); + let mut any_flags = !flags.is_empty(); + cmd.args(flags); for flag in &self.ar_flags { + any_flags = true; cmd.arg(flag); } - Ok(cmd) + Ok((cmd, any_flags)) } fn get_base_archiver(&self) -> Result { From de69fc7547783af3b2b3c03688c54a7d3061e7b5 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Mon, 23 Jan 2023 15:23:54 -0800 Subject: [PATCH 3/3] Avoid CommandBuilder::get_program for MSRV --- src/lib.rs | 52 +++++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4e6fb2df3..be3075748 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2666,13 +2666,7 @@ impl Build { } fn get_ar(&self) -> Result<(Command, String, bool), Error> { - let (cmd, any_flags) = self.try_get_archiver_and_flags()?; - let name = Path::new(cmd.get_program()) - .file_name() - .ok_or_else(|| Error::new(ErrorKind::IOError, "Failed to get archiver path."))? - .to_string_lossy() - .into_owned(); - Ok((cmd, name, any_flags)) + self.try_get_archiver_and_flags() } /// Get the archiver (ar) that's in use for this configuration. @@ -2704,8 +2698,8 @@ impl Build { Ok(self.try_get_archiver_and_flags()?.0) } - fn try_get_archiver_and_flags(&self) -> Result<(Command, bool), Error> { - let mut cmd = self.get_base_archiver()?; + fn try_get_archiver_and_flags(&self) -> Result<(Command, String, bool), Error> { + let (mut cmd, name) = self.get_base_archiver()?; let flags = self.envflags("ARFLAGS"); let mut any_flags = !flags.is_empty(); cmd.args(flags); @@ -2713,12 +2707,12 @@ impl Build { any_flags = true; cmd.arg(flag); } - Ok((cmd, any_flags)) + Ok((cmd, name, any_flags)) } - fn get_base_archiver(&self) -> Result { + fn get_base_archiver(&self) -> Result<(Command, String), Error> { if let Some(ref a) = self.archiver { - return Ok(self.cmd(a)); + return Ok((self.cmd(a), a.to_string_lossy().into_owned())); } self.get_base_archiver_variant("AR", "ar") @@ -2760,11 +2754,12 @@ impl Build { return Ok(self.cmd(r)); } - self.get_base_archiver_variant("RANLIB", "ranlib") + Ok(self.get_base_archiver_variant("RANLIB", "ranlib")?.0) } - fn get_base_archiver_variant(&self, env: &str, tool: &str) -> Result { + fn get_base_archiver_variant(&self, env: &str, tool: &str) -> Result<(Command, String), Error> { let target = self.get_target()?; + let mut name = String::new(); let tool_opt: Option = self .env_tool(env) .map(|(tool, _wrapper, args)| { @@ -2777,10 +2772,12 @@ impl Build { // Windows use bat files so we have to be a bit more specific if cfg!(windows) { let mut cmd = self.cmd("cmd"); - cmd.arg("/c").arg(format!("em{}.bat", tool)); + name = format!("em{}.bat", tool); + cmd.arg("/c").arg(&name); Some(cmd) } else { - Some(self.cmd(format!("em{}", tool))) + name = format!("em{}", tool); + Some(self.cmd(&name)) } } else { None @@ -2792,7 +2789,8 @@ impl Build { Some(t) => t, None => { if target.contains("android") { - self.cmd(format!("{}-{}", target.replace("armv7", "arm"), tool)) + name = format!("{}-{}", target.replace("armv7", "arm"), tool); + self.cmd(&name) } else if target.contains("msvc") { // NOTE: There isn't really a ranlib on msvc, so arguably we should return // `None` somehow here. But in general, callers will already have to be aware @@ -2817,19 +2815,22 @@ impl Build { } if lib.is_empty() { + name = String::from("lib.exe"); match windows_registry::find(&target, "lib.exe") { Some(t) => t, None => self.cmd("lib.exe"), } } else { - self.cmd(lib) + name = lib; + self.cmd(&name) } } else if target.contains("illumos") { // The default 'ar' on illumos uses a non-standard flags, // but the OS comes bundled with a GNU-compatible variant. // // Use the GNU-variant to match other Unix systems. - self.cmd(format!("g{}", tool)) + name = format!("g{}", tool); + self.cmd(&name) } else if self.get_host()? != target { match self.prefix_for_target(&target) { Some(p) => { @@ -2848,17 +2849,22 @@ impl Build { break; } } - self.cmd(chosen) + name = chosen; + self.cmd(&name) + } + None => { + name = default; + self.cmd(&name) } - None => self.cmd(default), } } else { - self.cmd(default) + name = default; + self.cmd(&name) } } }; - Ok(tool) + Ok((tool, name)) } fn prefix_for_target(&self, target: &str) -> Option {