Always read from all CFLAGS-style flags (#1401)

Author: madsmtm

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,41 +3643,64 @@ 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"
         } else {
             "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> {

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));
+}

tests/cflags.rs

@@ -1,15 +1,39 @@
+//! 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();
     test.gcc().file("foo.c").compile("foo");
 
     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");
+}