Skip to content

Add prefer_clang_cl_over_msvc #1516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ pub struct Build {
shell_escaped_flags: Option<bool>,
build_cache: Arc<BuildCache>,
inherit_rustflags: bool,
prefer_clang_cl_over_msvc: bool,
}

/// Represents the types of errors that may occur while using cc-rs.
Expand Down Expand Up @@ -479,6 +480,7 @@ impl Build {
shell_escaped_flags: None,
build_cache: Arc::default(),
inherit_rustflags: true,
prefer_clang_cl_over_msvc: false,
}
}

Expand Down Expand Up @@ -1290,6 +1292,14 @@ impl Build {
self
}

/// Prefer to use clang-cl over msvc.
///
/// This option defaults to `false`.
pub fn prefer_clang_cl_over_msvc(&mut self, prefer_clang_cl_over_msvc: bool) -> &mut Build {
self.prefer_clang_cl_over_msvc = prefer_clang_cl_over_msvc;
self
}

#[doc(hidden)]
pub fn __set_env<A, B>(&mut self, a: A, b: B) -> &mut Build
where
Expand Down Expand Up @@ -2871,10 +2881,17 @@ impl Build {
}
let target = self.get_target()?;
let raw_target = self.get_raw_target()?;
let (env, msvc, gnu, traditional, clang) = if self.cpp {
("CXX", "cl.exe", "g++", "c++", "clang++")

let msvc = if self.prefer_clang_cl_over_msvc {
"clang-cl.exe"
} else {
"cl.exe"
};

let (env, gnu, traditional, clang) = if self.cpp {
("CXX", "g++", "c++", "clang++")
} else {
("CC", "cl.exe", "gcc", "cc", "clang")
("CC", "gcc", "cc", "clang")
};

// On historical Solaris systems, "cc" may have been Sun Studio, which
Expand All @@ -2887,7 +2904,7 @@ impl Build {
traditional
};

let cl_exe = self.windows_registry_find_tool(&target, "cl.exe");
let cl_exe = self.windows_registry_find_tool(&target, msvc);

let tool_opt: Option<Tool> = self
.env_tool(env)
Expand Down
3 changes: 2 additions & 1 deletion src/windows/find_tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ mod impl_ {

use super::{EnvGetter, TargetArch, MSVC_FAMILY};
use crate::Tool;
use crate::ToolFamily;

struct MsvcTool {
tool: PathBuf,
Expand Down Expand Up @@ -555,7 +556,7 @@ mod impl_ {
base_path.push(tool);
base_path
.is_file()
.then(|| Tool::with_family(base_path, MSVC_FAMILY))
.then(|| Tool::with_family(base_path, ToolFamily::Msvc { clang_cl: true }))
Comment on lines -558 to +559
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this bit in my other PR #1506. MSVC_FAMILY basically sets ToolFamily::Msvc { clang_cl: false }, which isn't accurate when looking for clang-cl.exe.

Question:

use cc::windows_registry::find;

let clang = find("x64", "clang.exe"); // Will return `ToolFamily::Msvc { clang_cl: true }`. Is that OK, or should it only be set to `true` when we're explicitly looking for `clang-cl.exe`?
let clang = find("x64", "clang-cl.exe"); // Will return `ToolFamily::Msvc { clang_cl: true }`.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang_cl should only set to true for clang-cl

For clang, I think it'd count as family clang instead of msvc?

})
.next()
}
Expand Down
12 changes: 11 additions & 1 deletion tests/support/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub struct Test {
pub td: TempDir,
pub gcc: PathBuf,
pub msvc: bool,
pub msvc_autodetect: bool,
}

pub struct Execution {
Expand Down Expand Up @@ -53,6 +54,7 @@ impl Test {
td,
gcc,
msvc: false,
msvc_autodetect: false,
}
}

Expand All @@ -69,6 +71,14 @@ impl Test {
t
}

// For msvc_autodetect, don't explicitly set the compiler - let the build system discover it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 115-118 explicitly set the compiler to cl when msvc is set to true, which makes our use case much harder to test. We could just rely on the build system to discover it, given that GitHub Actions runners have MSVC + Clang installed anyway. I think this would make the tests fail on Windows systems that don't have MSVC + Clang installed though. Is that an acceptable trade-off, or should I try to mock things better and not rely on MSVC + Clang actually being installed when running the tests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a cfg(not(CC_TEST_DISABLE_CLANG_CL_TESTS)) just in case someone needs it

pub fn msvc_autodetect() -> Test {
let mut t = Test::new();
t.shim("cl").shim("clang-cl.exe").shim("lib.exe");
t.msvc_autodetect = true;
t
}

pub fn clang() -> Test {
let t = Test::new();
t.shim("clang").shim("clang++").shim("ar");
Expand All @@ -87,7 +97,7 @@ impl Test {

pub fn gcc(&self) -> cc::Build {
let mut cfg = cc::Build::new();
let target = if self.msvc {
let target = if self.msvc || self.msvc_autodetect {
"x86_64-pc-windows-msvc"
} else if cfg!(target_os = "macos") {
"x86_64-apple-darwin"
Expand Down
94 changes: 94 additions & 0 deletions tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -851,3 +851,97 @@ fn clang_android() {
test.cmd(0).must_not_have("--target=arm-linux-androideabi");
}
}

#[test]
#[cfg(windows)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to put them in a mod to avoid repeated cfg

fn msvc_prefer_clang_cl_over_msvc_disabled_by_default() {
reset_env();

let test = Test::msvc_autodetect();

// When prefer_clang_cl_over_msvc is not called (default false), should use MSVC
let compiler = test
.gcc()
.try_get_compiler()
.expect("Failed to get compiler");

// By default, should be using MSVC (cl.exe) and NOT clang-cl
assert!(compiler.is_like_msvc(), "Should use MSVC by default");
assert!(
!compiler.is_like_clang_cl(),
"Should not use clang-cl by default"
);
}

#[test]
#[cfg(windows)]
fn msvc_prefer_clang_cl_over_msvc_enabled() {
reset_env();

let test = Test::msvc_autodetect();

let compiler = test
.gcc()
// When prefer_clang_cl_over_msvc is true, should use clang-cl.exe
.prefer_clang_cl_over_msvc(true)
.try_get_compiler()
.expect("Failed to get compiler");

assert!(
compiler.is_like_clang_cl(),
"clang-cl.exe should be identified as clang-cl-like, got {:?}",
compiler
);
assert!(
compiler.is_like_msvc(),
"clang-cl should still be MSVC-like"
);
}

#[test]
#[cfg(windows)]
fn msvc_prefer_clang_cl_over_msvc_respects_explicit_cc_env() {
reset_env();

let test = Test::msvc_autodetect();
let compiler = test
.gcc()
// We can't set the CC=cl.exe environment variable directly in the test as it's removed
// in mod.rs, so we simulate it by setting the compiler directly
Comment on lines +909 to +910
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a hidden cc::Build::__set_env function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I just tried using that as follows:

    let compiler = test
        .gcc()
        // If the user has set CC to cl.exe, we should respect that
        .__set_env("CC", "cl.exe")
        .prefer_clang_cl_over_msvc(true)
        .try_get_compiler()
        .expect("Failed to get compiler");

However, it's not being picked up because get_base_compiler calls env_tool which - if I understand the code correctly - looks at the actual environment variables that are available to the current process, and not at the ones that were set through the hidden __set_env method. So even though it looks for the CC env var, it doesn't find anything because it's not set at the process level.

I'm curious if you're aware of any alternatives though! I also tried std::env::set_var("CC", "cl.exe"); which works when I run the test in isolation, but not when running the entire test suite, which runs many tests in parallel and probably shares the same process-level env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could update the getenv function to first look at self.env if set, and otherwise fall back to process-level env variables using env::var_os, which it was already doing. I'm by no means proficient in Rust so I'm sure there might be a more efficient way to do this, but the code below seems to work and the entire test suite (ran with cargo test) also passes with it:

diff --git a/src/lib.rs b/src/lib.rs
index 8a74141..24a7bbb 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3795,7 +3795,11 @@ impl Build {
             self.cargo_output
                 .print_metadata(&format_args!("cargo:rerun-if-env-changed={v}"));
         }
-        let r = env::var_os(v).map(Arc::from);
+        let r = self.env.iter().find(|(k, _)| k.as_ref() == v).cloned().map(|(_, v)| v);
+        let r = match r {
+            Some(ref val) if !val.is_empty() => Some(val.clone()),
+            _ => env::var_os(v).map(Arc::from),
+        };
         self.cargo_output.print_metadata(&format_args!(
             "{} = {}",
             v,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks pretty good except for the empty checking and unnecessary cloning.

        let r = self.env
            .iter()
            .find(|(k, value)| k.as_ref() == v)
            .map(|(_, value)| value.clone())
            .or_else(|| env::var_os(v).map(Arc::from));

.compiler("cl.exe")
.prefer_clang_cl_over_msvc(true)
.try_get_compiler()
.expect("Failed to get compiler");

// The preference should not override explicit compiler setting
assert!(compiler.is_like_msvc(), "Should still be MSVC-like");
assert!(
!compiler.is_like_clang_cl(),
"Should NOT use clang-cl when CC is explicitly set to cl.exe, got {:?}",
compiler
);
}

#[test]
#[cfg(windows)]
fn msvc_prefer_clang_cl_over_msvc_cpp_mode() {
reset_env();

let test = Test::msvc_autodetect();
let compiler = test
.gcc()
.cpp(true)
.prefer_clang_cl_over_msvc(true)
.try_get_compiler()
.expect("Failed to get compiler");

// Verify clang-cl.exe works correctly in C++ mode
assert!(
compiler.is_like_clang_cl(),
"clang-cl.exe should be identified as clang-cl-like in C++ mode"
);
assert!(
compiler.is_like_msvc(),
"clang-cl should still be MSVC-like in C++ mode"
);
}
Loading