Repository: rust-lang/cc-rs
Pull request #1401 (2025-02-14 12:58:12)

Always read from all `CFLAGS`-style flags
Reading from just one of these means that users setting different environment variables in different parts of the build process would not get flags from the other parts.

This tripped me up in https://github.com/rust-lang/rust/pull/133092 / https://github.com/rust-lang/rust/issues/136984, where I thought that since I was extending `CFLAGS_*` instead of overwriting it, I was allowing `cc-rs` to also read from `CFLAGS`.

Instead, we now read all of the flags, but in order such that more specific flags override less specific ones.

So e.g. when users set `CFLAGS=-flag1 CFLAGS_aarch64_apple_darwin=-flag2 cargo build`, we end up passing `-flag1 -flag2` to the compiler (in that order specifically).

NOTE: This has the slight chance of breaking builds where users assume that `CFLAGS_$TARGET` overwrites `CFLAGS`. I will argue that that is likely to be the minority, relative to users that do want flags from all `CFLAGS*` env vars.
To fix such cases, users should pass the negative flag in `CFLAGS_$TARGET` that they don't want `CFLAGS` to set (e.g. if `CFLAGS` contains `-pthread`, users should pass `CFLAGS_$TARGET=-no-pthread`).


Patch:
diff --git a/src/lib.rs b/src/lib.rs
index 0a0bc98..f059d4f 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<Command, Error> {
         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<Command, Error> {
         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<Arc<OsStr>, 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<Arc<OsStr>, 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<Vec<String>, 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<Option<Vec<String>>, 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> {


Test patch:
diff --git a/tests/cc_env.rs b/tests/cc_env.rs
index a493873..7eed954 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 caec6ea..e93c290 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");
+}