From 0dd683ca4c57fff1d8701d5a780749fcf09a5e75 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 14 Feb 2025 14:35:21 +0100 Subject: [PATCH] Always read from all `CFLAGS`-style flags (#1401) --- src/lib.rs | 83 +++++++++++++++++++++++++++++++------------------ tests/cc_env.rs | 31 ++++++++++++++++++ tests/cflags.rs | 28 +++++++++++++++-- 3 files changed, 110 insertions(+), 32 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0a0bc98c0..f059d4f44 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -653,7 +653,12 @@ impl Build { /// ``` /// pub fn try_flags_from_environment(&mut self, environ_key: &str) -> Result<&mut Build, Error> { - let flags = self.envflags(environ_key)?; + let flags = self.envflags(environ_key)?.ok_or_else(|| { + Error::new( + ErrorKind::EnvVarNotFound, + format!("could not find environment variable {environ_key}"), + ) + })?; self.flags.extend( flags .into_iter() @@ -1907,7 +1912,8 @@ impl Build { cmd.args.push(directory.as_os_str().into()); } - if let Ok(flags) = self.envflags(if self.cpp { "CXXFLAGS" } else { "CFLAGS" }) { + let flags = self.envflags(if self.cpp { "CXXFLAGS" } else { "CFLAGS" })?; + if let Some(flags) = &flags { for arg in flags { cmd.push_cc_arg(arg.into()); } @@ -1918,12 +1924,12 @@ impl Build { // CFLAGS/CXXFLAGS, since those variables presumably already contain // the desired set of warnings flags. - if self.warnings.unwrap_or(!self.has_flags()) { + if self.warnings.unwrap_or(flags.is_none()) { let wflags = cmd.family.warnings_flags().into(); cmd.push_cc_arg(wflags); } - if self.extra_warnings.unwrap_or(!self.has_flags()) { + if self.extra_warnings.unwrap_or(flags.is_none()) { if let Some(wflags) = cmd.family.extra_warnings_flags() { cmd.push_cc_arg(wflags.into()); } @@ -2443,12 +2449,6 @@ impl Build { Ok(()) } - fn has_flags(&self) -> bool { - let flags_env_var_name = if self.cpp { "CXXFLAGS" } else { "CFLAGS" }; - let flags_env_var_value = self.getenv_with_target_prefixes(flags_env_var_name); - flags_env_var_value.is_ok() - } - fn msvc_macro_assembler(&self) -> Result { let target = self.get_target()?; let tool = if target.arch == "x86_64" { @@ -3115,8 +3115,8 @@ impl Build { fn try_get_archiver_and_flags(&self) -> Result<(Command, PathBuf, bool), Error> { let (mut cmd, name) = self.get_base_archiver()?; let mut any_flags = false; - if let Ok(flags) = self.envflags("ARFLAGS") { - any_flags |= !flags.is_empty(); + if let Some(flags) = self.envflags("ARFLAGS")? { + any_flags = true; cmd.args(flags); } for flag in &self.ar_flags { @@ -3162,7 +3162,7 @@ impl Build { /// see [`Self::get_ranlib`] for the complete description. pub fn try_get_ranlib(&self) -> Result { let mut cmd = self.get_base_ranlib()?; - if let Ok(flags) = self.envflags("RANLIBFLAGS") { + if let Some(flags) = self.envflags("RANLIBFLAGS")? { cmd.args(flags); } Ok(cmd) @@ -3643,7 +3643,8 @@ impl Build { }) } - fn getenv_with_target_prefixes(&self, var_base: &str) -> Result, Error> { + /// The list of environment variables to check for a given env, in order of priority. + fn target_envs(&self, env: &str) -> Result<[String; 4], Error> { let target = self.get_raw_target()?; let kind = if self.get_is_cross_compile()? { "TARGET" @@ -3651,33 +3652,55 @@ impl Build { "HOST" }; let target_u = target.replace('-', "_"); + + Ok([ + format!("{env}_{target}"), + format!("{env}_{target_u}"), + format!("{kind}_{env}"), + env.to_string(), + ]) + } + + /// Get a single-valued environment variable with target variants. + fn getenv_with_target_prefixes(&self, env: &str) -> Result, Error> { + // Take from first environment variable in the environment. let res = self - .getenv(&format!("{}_{}", var_base, target)) - .or_else(|| self.getenv(&format!("{}_{}", var_base, target_u))) - .or_else(|| self.getenv(&format!("{}_{}", kind, var_base))) - .or_else(|| self.getenv(var_base)); + .target_envs(env)? + .iter() + .filter_map(|env| self.getenv(env)) + .next(); match res { Some(res) => Ok(res), None => Err(Error::new( ErrorKind::EnvVarNotFound, - format!("Could not find environment variable {}.", var_base), + format!("could not find environment variable {env}"), )), } } - fn envflags(&self, name: &str) -> Result, Error> { - let env_os = self.getenv_with_target_prefixes(name)?; - let env = env_os.to_string_lossy(); - - if self.get_shell_escaped_flags() { - Ok(Shlex::new(&env).collect()) - } else { - Ok(env - .split_ascii_whitespace() - .map(ToString::to_string) - .collect()) + /// Get values from CFLAGS-style environment variable. + fn envflags(&self, env: &str) -> Result>, Error> { + // Collect from all environment variables, in reverse order as in + // `getenv_with_target_prefixes` precedence (so that `CFLAGS_$TARGET` + // can override flags in `TARGET_CFLAGS`, which overrides those in + // `CFLAGS`). + let mut any_set = false; + let mut res = vec![]; + for env in self.target_envs(env)?.iter().rev() { + if let Some(var) = self.getenv(env) { + any_set = true; + + let var = var.to_string_lossy(); + if self.get_shell_escaped_flags() { + res.extend(Shlex::new(&var)); + } else { + res.extend(var.split_ascii_whitespace().map(ToString::to_string)); + } + } } + + Ok(if any_set { Some(res) } else { None }) } fn fix_env_for_apple_os(&self, cmd: &mut Command) -> Result<(), Error> { diff --git a/tests/cc_env.rs b/tests/cc_env.rs index a4938732e..7eed9542a 100644 --- a/tests/cc_env.rs +++ b/tests/cc_env.rs @@ -16,6 +16,7 @@ fn main() { path_to_ccache(); more_spaces(); clang_cl(); + env_var_alternatives_override(); } fn ccache() { @@ -126,3 +127,33 @@ fn clang_cl() { test_compiler(test.gcc()); } } + +fn env_var_alternatives_override() { + let compiler1 = format!("clang1{}", env::consts::EXE_SUFFIX); + let compiler2 = format!("clang2{}", env::consts::EXE_SUFFIX); + let compiler3 = format!("clang3{}", env::consts::EXE_SUFFIX); + let compiler4 = format!("clang4{}", env::consts::EXE_SUFFIX); + + let test = Test::new(); + test.shim(&compiler1); + test.shim(&compiler2); + test.shim(&compiler3); + test.shim(&compiler4); + + env::set_var("CC", &compiler1); + let compiler = test.gcc().target("x86_64-unknown-none").get_compiler(); + assert_eq!(compiler.path(), Path::new(&compiler1)); + + env::set_var("HOST_CC", &compiler2); + env::set_var("TARGET_CC", &compiler2); + let compiler = test.gcc().target("x86_64-unknown-none").get_compiler(); + assert_eq!(compiler.path(), Path::new(&compiler2)); + + env::set_var("CC_x86_64_unknown_none", &compiler3); + let compiler = test.gcc().target("x86_64-unknown-none").get_compiler(); + assert_eq!(compiler.path(), Path::new(&compiler3)); + + env::set_var("CC_x86_64-unknown-none", &compiler4); + let compiler = test.gcc().target("x86_64-unknown-none").get_compiler(); + assert_eq!(compiler.path(), Path::new(&compiler4)); +} diff --git a/tests/cflags.rs b/tests/cflags.rs index caec6ea4e..e93c29078 100644 --- a/tests/cflags.rs +++ b/tests/cflags.rs @@ -1,11 +1,16 @@ +//! This test is in its own module because it modifies the environment and would affect other tests +//! when run in parallel with them. mod support; use crate::support::Test; use std::env; -/// This test is in its own module because it modifies the environment and would affect other tests -/// when run in parallel with them. #[test] +fn cflags() { + gnu_no_warnings_if_cflags(); + cflags_order(); +} + fn gnu_no_warnings_if_cflags() { env::set_var("CFLAGS", "-arbitrary"); let test = Test::gnu(); @@ -13,3 +18,22 @@ fn gnu_no_warnings_if_cflags() { test.cmd(0).must_not_have("-Wall").must_not_have("-Wextra"); } + +/// Test the ordering of `CFLAGS*` variables. +fn cflags_order() { + unsafe { env::set_var("CFLAGS", "-arbitrary1") }; + unsafe { env::set_var("HOST_CFLAGS", "-arbitrary2") }; + unsafe { env::set_var("TARGET_CFLAGS", "-arbitrary2") }; + unsafe { env::set_var("CFLAGS_x86_64_unknown_none", "-arbitrary3") }; + unsafe { env::set_var("CFLAGS_x86_64-unknown-none", "-arbitrary4") }; + let test = Test::gnu(); + test.gcc() + .target("x86_64-unknown-none") + .file("foo.c") + .compile("foo"); + + test.cmd(0) + .must_have_in_order("-arbitrary1", "-arbitrary2") + .must_have_in_order("-arbitrary2", "-arbitrary3") + .must_have_in_order("-arbitrary3", "-arbitrary4"); +}