From 249d2078f2dd16b61d3184d56d98d0bfe39a4bc9 Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 14 Feb 2025 13:43:52 +0100
Subject: [PATCH 1/3] 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.
Instead, we read all of the flags, but in a specific order such that
more specific flags override less specific ones.
---
src/lib.rs | 82 ++++++++++++++++++++++++++++++++-----------------
tests/cc_env.rs | 31 +++++++++++++++++++
tests/cflags.rs | 28 +++++++++++++++--
3 files changed, 110 insertions(+), 31 deletions(-)
diff --git a/src/lib.rs b/src/lib.rs
index 0a0bc98c0..086fe9e4d 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> {
+ /// Get a single-valued environment variable with target variants.
+ fn getenv_with_target_prefixes(&self, env: &str) -> Result<Arc<OsStr>, Error> {
let target = self.get_raw_target()?;
let kind = if self.get_is_cross_compile()? {
"TARGET"
@@ -3652,32 +3653,55 @@ impl Build {
};
let target_u = target.replace('-', "_");
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));
+ .getenv(&format!("{env}_{target}"))
+ .or_else(|| self.getenv(&format!("{env}_{target_u}")))
+ .or_else(|| self.getenv(&format!("{kind}_{env}")))
+ .or_else(|| self.getenv(env));
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())
+ /// Get values from CFLAGS-style environment variable.
+ fn envflags(&self, env: &str) -> Result<Option<Vec<String>>, Error> {
+ let target = self.get_raw_target()?;
+ let kind = if self.get_is_cross_compile()? {
+ "TARGET"
} else {
- Ok(env
- .split_ascii_whitespace()
- .map(ToString::to_string)
- .collect())
+ "HOST"
+ };
+ let target_u = target.replace('-', "_");
+
+ // 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 [
+ env,
+ &format!("{kind}_{env}"),
+ &format!("{env}_{target_u}"),
+ &format!("{env}_{target}"),
+ ] {
+ 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");
+}
From 94279dd8366cc55a3b568c36a397f6d0908cebf3 Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 14 Feb 2025 14:24:01 +0100
Subject: [PATCH 2/3] Deduplicate logic for finding CFLAGS-style flags
---
src/lib.rs | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/src/lib.rs b/src/lib.rs
index 086fe9e4d..761acd3a0 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3643,8 +3643,8 @@ impl Build {
})
}
- /// Get a single-valued environment variable with target variants.
- fn getenv_with_target_prefixes(&self, env: &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"
@@ -3652,11 +3652,19 @@ impl Build {
"HOST"
};
let target_u = target.replace('-', "_");
- let res = self
- .getenv(&format!("{env}_{target}"))
- .or_else(|| self.getenv(&format!("{env}_{target_u}")))
- .or_else(|| self.getenv(&format!("{kind}_{env}")))
- .or_else(|| self.getenv(env));
+
+ 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> {
+ let envs = self.target_envs(env)?;
+ let res = envs.iter().filter_map(|env| self.getenv(&env)).next();
match res {
Some(res) => Ok(res),
@@ -3669,26 +3677,13 @@ impl Build {
/// Get values from CFLAGS-style environment variable.
fn envflags(&self, env: &str) -> Result<Option<Vec<String>>, Error> {
- let target = self.get_raw_target()?;
- let kind = if self.get_is_cross_compile()? {
- "TARGET"
- } else {
- "HOST"
- };
- let target_u = target.replace('-', "_");
-
// 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 [
- env,
- &format!("{kind}_{env}"),
- &format!("{env}_{target_u}"),
- &format!("{env}_{target}"),
- ] {
+ for env in self.target_envs(env)?.iter().rev() {
if let Some(var) = self.getenv(env) {
any_set = true;
From 4f917fe74bb47e20e5ee85bd71bb6e671016fdce Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 14 Feb 2025 14:30:42 +0100
Subject: [PATCH 3/3] Appease Clippy
---
src/lib.rs | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/lib.rs b/src/lib.rs
index 761acd3a0..f059d4f44 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3663,8 +3663,12 @@ impl Build {
/// Get a single-valued environment variable with target variants.
fn getenv_with_target_prefixes(&self, env: &str) -> Result<Arc<OsStr>, Error> {
- let envs = self.target_envs(env)?;
- let res = envs.iter().filter_map(|env| self.getenv(&env)).next();
+ // Take from first environment variable in the environment.
+ let res = self
+ .target_envs(env)?
+ .iter()
+ .filter_map(|env| self.getenv(env))
+ .next();
match res {
Some(res) => Ok(res),