From 7a651f74aedef60439bbac11bef9e63dc91b1cab Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 14 Feb 2025 15:50:15 +0100
Subject: [PATCH] Change flag ordering

To allow flags set on the builder to override other flags, and to allow
CFLAGS to override _all_ other flags.
---
 src/lib.rs           | 78 +++++++++++++++++++++++++-------------------
 tests/cflags.rs      | 36 +++++++++++++++-----
 tests/support/mod.rs |  4 +--
 3 files changed, 73 insertions(+), 45 deletions(-)

diff --git a/src/lib.rs b/src/lib.rs
index f059d4f4..6f39e0e3 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1892,13 +1892,39 @@ impl Build {
 
         let mut cmd = self.get_base_compiler()?;
 
+        // The flags below are added in roughly the following order:
+        // 1. Default flags
+        //   - Controlled by `cc-rs`.
+        // 2. `rustc`-inherited flags
+        //   - Controlled by `rustc`.
+        // 3. Builder flags
+        //   - Controlled by the developer using `cc-rs` in e.g. their `build.rs`.
+        // 4. Environment flags
+        //   - Controlled by the end user.
+        //
+        // This is important to allow later flags to override previous ones.
+
+        // Copied from <https://github.com/rust-lang/rust/blob/5db81020006d2920fc9c62ffc0f4322f90bffa04/compiler/rustc_codegen_ssa/src/back/linker.rs#L27-L38>
+        //
+        // Disables non-English messages from localized linkers.
+        // Such messages may cause issues with text encoding on Windows
+        // and prevent inspection of msvc output in case of errors, which we occasionally do.
+        // This should be acceptable because other messages from rustc are in English anyway,
+        // and may also be desirable to improve searchability of the compiler diagnostics.
+        if matches!(cmd.family, ToolFamily::Msvc { clang_cl: false }) {
+            cmd.env.push(("VSLANG".into(), "1033".into()));
+        } else {
+            cmd.env.push(("LC_ALL".into(), "C".into()));
+        }
+
         // Disable default flag generation via `no_default_flags` or environment variable
         let no_defaults = self.no_default_flags || self.getenv_boolean("CRATE_CC_NO_DEFAULTS");
-
         if !no_defaults {
             self.add_default_flags(&mut cmd, &target, &opt_level)?;
         }
 
+        // Specify various flags that are not considered part of the default flags above.
+        // FIXME(madsmtm): Should these be considered part of the defaults? If no, why not?
         if let Some(ref std) = self.std {
             let separator = match cmd.family {
                 ToolFamily::Msvc { .. } => ':',
@@ -1906,44 +1932,40 @@ impl Build {
             };
             cmd.push_cc_arg(format!("-std{}{}", separator, std).into());
         }
-
         for directory in self.include_directories.iter() {
             cmd.args.push("-I".into());
             cmd.args.push(directory.as_os_str().into());
         }
-
-        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());
-            }
+        if self.warnings_into_errors {
+            let warnings_to_errors_flag = cmd.family.warnings_to_errors_flag().into();
+            cmd.push_cc_arg(warnings_to_errors_flag);
         }
 
         // If warnings and/or extra_warnings haven't been explicitly set,
         // then we set them only if the environment doesn't already have
         // CFLAGS/CXXFLAGS, since those variables presumably already contain
         // the desired set of warnings flags.
-
-        if self.warnings.unwrap_or(flags.is_none()) {
+        let envflags = self.envflags(if self.cpp { "CXXFLAGS" } else { "CFLAGS" })?;
+        if self.warnings.unwrap_or(envflags.is_none()) {
             let wflags = cmd.family.warnings_flags().into();
             cmd.push_cc_arg(wflags);
         }
-
-        if self.extra_warnings.unwrap_or(flags.is_none()) {
+        if self.extra_warnings.unwrap_or(envflags.is_none()) {
             if let Some(wflags) = cmd.family.extra_warnings_flags() {
                 cmd.push_cc_arg(wflags.into());
             }
         }
 
-        for flag in self.flags.iter() {
-            cmd.args.push((**flag).into());
-        }
-
-        // Add cc flags inherited from matching rustc flags
+        // Add cc flags inherited from matching rustc flags.
         if self.inherit_rustflags {
             self.add_inherited_rustflags(&mut cmd, &target)?;
         }
 
+        // Set flags configured in the builder (do this second-to-last, to allow these to override
+        // everything above).
+        for flag in self.flags.iter() {
+            cmd.args.push((**flag).into());
+        }
         for flag in self.flags_supported.iter() {
             if self
                 .is_flag_supported_inner(flag, &cmd, &target)
@@ -1952,7 +1974,6 @@ impl Build {
                 cmd.push_cc_arg((**flag).into());
             }
         }
-
         for (key, value) in self.definitions.iter() {
             if let Some(ref value) = *value {
                 cmd.args.push(format!("-D{}={}", key, value).into());
@@ -1961,22 +1982,11 @@ impl Build {
             }
         }
 
-        if self.warnings_into_errors {
-            let warnings_to_errors_flag = cmd.family.warnings_to_errors_flag().into();
-            cmd.push_cc_arg(warnings_to_errors_flag);
-        }
-
-        // Copied from <https://github.com/rust-lang/rust/blob/5db81020006d2920fc9c62ffc0f4322f90bffa04/compiler/rustc_codegen_ssa/src/back/linker.rs#L27-L38>
-        //
-        // Disables non-English messages from localized linkers.
-        // Such messages may cause issues with text encoding on Windows
-        // and prevent inspection of msvc output in case of errors, which we occasionally do.
-        // This should be acceptable because other messages from rustc are in English anyway,
-        // and may also be desirable to improve searchability of the compiler diagnostics.
-        if matches!(cmd.family, ToolFamily::Msvc { clang_cl: false }) {
-            cmd.env.push(("VSLANG".into(), "1033".into()));
-        } else {
-            cmd.env.push(("LC_ALL".into(), "C".into()));
+        // Set flags from the environment (do this last, to allow these to override everything else).
+        if let Some(flags) = &envflags {
+            for arg in flags {
+                cmd.push_cc_arg(arg.into());
+            }
         }
 
         Ok(cmd)
diff --git a/tests/cflags.rs b/tests/cflags.rs
index e93c2907..06bdb737 100644
--- a/tests/cflags.rs
+++ b/tests/cflags.rs
@@ -19,21 +19,39 @@ fn gnu_no_warnings_if_cflags() {
     test.cmd(0).must_not_have("-Wall").must_not_have("-Wextra");
 }
 
-/// Test the ordering of `CFLAGS*` variables.
+/// Test the ordering of flags.
+///
+/// 1. Default flags
+/// 2. Rustflags.
+/// 3. Builder flags.
+/// 4. Environment flags.
 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") };
+    // FIXME(madsmtm): Re-enable once `is_flag_supported` works in CI regardless of `target`.
+    // unsafe { std::env::set_var("CARGO_ENCODED_RUSTFLAGS", "-Cdwarf-version=5") };
+
+    unsafe { env::set_var("CFLAGS", "-Larbitrary1") };
+    unsafe { env::set_var("HOST_CFLAGS", "-Larbitrary2") };
+    unsafe { env::set_var("TARGET_CFLAGS", "-Larbitrary2") };
+    unsafe { env::set_var("CFLAGS_x86_64_unknown_none", "-Larbitrary3") };
+    unsafe { env::set_var("CFLAGS_x86_64-unknown-none", "-Larbitrary4") };
+
     let test = Test::gnu();
     test.gcc()
         .target("x86_64-unknown-none")
+        .static_flag(true)
+        .flag("-Lbuilder-flag1")
+        .flag("-Lbuilder-flag2")
         .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");
+        // .must_have_in_order("-static", "-gdwarf-5")
+        // .must_have_in_order("-gdwarf-5", "-Lbuilder-flag1")
+        .must_have_in_order("-static", "-Lbuilder-flag1")
+        .must_have_in_order("-Lbuilder-flag1", "-Lbuilder-flag2")
+        .must_have_in_order("-Lbuilder-flag2", "-Larbitrary1")
+        .must_have_in_order("-Larbitrary1", "-Larbitrary2")
+        .must_have_in_order("-Larbitrary1", "-Larbitrary2")
+        .must_have_in_order("-Larbitrary2", "-Larbitrary3")
+        .must_have_in_order("-Larbitrary3", "-Larbitrary4");
 }
diff --git a/tests/support/mod.rs b/tests/support/mod.rs
index 67516a3b..0ca29ebb 100644
--- a/tests/support/mod.rs
+++ b/tests/support/mod.rs
@@ -160,8 +160,8 @@ impl Execution {
         match (before_position, after_position) {
             (Some(b), Some(a)) if b < a => {}
             (b, a) => panic!(
-                "{:?} (last position: {:?}) did not appear before {:?} (last position: {:?})",
-                before, b, after, a
+                "{:?} (last position: {:?}) did not appear before {:?} (last position: {:?}): {:?}",
+                before, b, after, a, self.args
             ),
         };
         self