Add prefer_clang_cl_over_msvc

[dennisameling]

Closes #882

This could probably replace #1367. Now that #1506 has been merged, windows_registry_find_tool can find clang-cl.exe correctly.

This PR also adds a fix for something I missed in find_tools as part of #1506: clang_cl should be set to true when using that toolchain.

Lastly, this PR adds some tests to ensure that the logic that @briansmith described here works as expected.

[dennisameling]

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 }`.

[NobodyXu]

clang_cl should only set to true for clang-cl

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

[dennisameling]

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?

[NobodyXu]

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

[dennisameling]

The failing tests in CI seem to be caused by ca81dcc, which has the same failing tests on the main branch.

[NobodyXu]

#1426

Confirmed that nightly has removed env in favor of abi, so it's not caused by this PR or that one

[NobodyXu]

Fixed in #1517, just rebasing would fix it

[NobodyXu]

Thanks!

I think you need to modify line 177 to add clang-cl to the array?

[NobodyXu]

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

[dennisameling]

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.

[dennisameling]

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,

[NobodyXu]

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

[NobodyXu]

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