Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` #1378

ErichDonGubler · 2025-01-31T03:48:45Z

I'm not sure which version exactly broke this, but between 1.0.89 and latest releases, this case had broken for the primary tool family heuristic. This forces Firefox/mozilla-central to work around breakage in a cc upgrade a la D236305. We introduced it in #455, so we'd like to keep it working!

…t` for `--driver-mode=…` search

…ase_compiler` Do this by plumbing through `args` associated with the compiler wrapper, if any, instead of handling it in calling code.

…lexity`

madsmtm

Not too familiar with our compiler detection, but fine with it if @NobodyXu is.

I'm sorry that it broke, if you'd really like it to not break in the future, I'd suggest you add a test ;)

NobodyXu

Thank you!

I have some feedbacks for the code itself, I think putting args in caching is the right approach

CHANGELOG.md

src/tool.rs

ErichDonGubler · 2025-01-31T17:29:04Z

Not too familiar with our compiler detection, but fine with it if @NobodyXu is.

I'm sorry that it broke, if you'd really like it to not break in the future, I'd suggest you add a test ;)

❤️ I think this is a pitch-perfect response. I'm not familiar with how cc does testing, so I scoped my efforts towards just fixing things, but I definitely am interested in avoiding future breakage too, so I'll roll up my sleeves and see if I can write a meaningful test.

ErichDonGubler · 2025-01-31T18:06:19Z

I'm actually not sure how to test this without having a fully functional clang --driver-mode=cl in CI, since the thing being fixed relies on evaluating stdout from the provided compiler. 🤔

src/tool.rs

NobodyXu · 2025-01-31T18:10:58Z

I'm actually not sure how to test this without having a fully functional clang --driver-mode=cl in CI

Maybe you can try adding a new shim here?

cc-rs/tests/support/mod.rs

Line 69 in 65b4d9a

pub fn clang() -> Test {

We currently has a gcc-shim binary in cc-rs https://github.com/rust-lang/cc-rs/blob/main/src/bin/gcc-shim.rs

Maybe we could have another clang-shim that does the testing?

This can be particularly significant for compilers that can dynamically change what options they accept based on arguments, like `clang --driver-mode=cl`.

NobodyXu

Thanks, LGTM!

If you are still working on the test, you can open a separate PR for it.

ErichDonGubler · 2025-01-31T19:09:56Z

Thanks, LGTM!

If you are still working on the test, you can open a separate PR for it.

Yes, that would be good! I'm doing a bit more refactoring to accommodate a new shim in Test than I think is appropriate for this fix, but I have written a test locally that fails before and succeeds after this PR. So, I think I'm on the right track for a good follow-up PR. Thanks for the coaching!

One question: When this lands, is there a rough timeline for a release to come out?

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉

madsmtm · 2025-01-31T19:34:09Z

Re release, see #1377. If not in that, then next week.

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…etection of `clang --driver-mode=cl` r=gfx-reviewers,nical With the current version of `cc`, we depend on behavior where, if `CC` is set to something like `clang --driver-mode=cl`, we expect to be able to use arguments on the command line a la MSVC's `cl.exe`. We were actually the original contributors of a heuristic to detect this in the `cc` crate, and it's served us well. In `cc` upstream since 1.0.89, a new heuristic for detecting compiler families in `cc::Tool` was introduced which does not have the desired behavior, and misclassifies the above case as being `clang`-like, rather than `cl`-like. The heuristic we originally submitted upstream is now in a fallback path which does not get used for our case. This causes `cc`'s default flags and APIs like `cc::Tool::is_like_msvc` to be incorrect. `swgl`, in particular, breaks because of this, since it's opinionated on the arguments it wants to provide to compilers. Work around the above regression by detecting checking `Tool`s' base command and "wrapper arguments" to see if we have a wrapper argument matching `--driver-mode=cl`. If so, provide `cl`-like arguments in `swgl`, rather than `clang`-like arguments. This behavior has been fixed upstream; see <rust-lang/cc-rs#1378>. Once released, we can consume it and revert this patch. Differential Revision: https://phabricator.services.mozilla.com/D236305

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…etection of `clang --driver-mode=cl` r=gfx-reviewers,nical With the current version of `cc`, we depend on behavior where, if `CC` is set to something like `clang --driver-mode=cl`, we expect to be able to use arguments on the command line a la MSVC's `cl.exe`. We were actually the original contributors of a heuristic to detect this in the `cc` crate, and it's served us well. In `cc` upstream since 1.0.89, a new heuristic for detecting compiler families in `cc::Tool` was introduced which does not have the desired behavior, and misclassifies the above case as being `clang`-like, rather than `cl`-like. The heuristic we originally submitted upstream is now in a fallback path which does not get used for our case. This causes `cc`'s default flags and APIs like `cc::Tool::is_like_msvc` to be incorrect. `swgl`, in particular, breaks because of this, since it's opinionated on the arguments it wants to provide to compilers. Work around the above regression by detecting checking `Tool`s' base command and "wrapper arguments" to see if we have a wrapper argument matching `--driver-mode=cl`. If so, provide `cl`-like arguments in `swgl`, rather than `clang`-like arguments. This behavior has been fixed upstream; see <rust-lang/cc-rs#1378>. Once released, we can consume it and revert this patch. Differential Revision: https://phabricator.services.mozilla.com/D236305

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…etection of `clang --driver-mode=cl` r=gfx-reviewers,nical With the current version of `cc`, we depend on behavior where, if `CC` is set to something like `clang --driver-mode=cl`, we expect to be able to use arguments on the command line a la MSVC's `cl.exe`. We were actually the original contributors of a heuristic to detect this in the `cc` crate, and it's served us well. In `cc` upstream since 1.0.89, a new heuristic for detecting compiler families in `cc::Tool` was introduced which does not have the desired behavior, and misclassifies the above case as being `clang`-like, rather than `cl`-like. The heuristic we originally submitted upstream is now in a fallback path which does not get used for our case. This causes `cc`'s default flags and APIs like `cc::Tool::is_like_msvc` to be incorrect. `swgl`, in particular, breaks because of this, since it's opinionated on the arguments it wants to provide to compilers. Work around the above regression by detecting checking `Tool`s' base command and "wrapper arguments" to see if we have a wrapper argument matching `--driver-mode=cl`. If so, provide `cl`-like arguments in `swgl`, rather than `clang`-like arguments. This behavior has been fixed upstream; see <rust-lang/cc-rs#1378>. Once released, we can consume it and revert this patch. Differential Revision: https://phabricator.services.mozilla.com/D236305

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…etection of `clang --driver-mode=cl` r=gfx-reviewers,nical With the current version of `cc`, we depend on behavior where, if `CC` is set to something like `clang --driver-mode=cl`, we expect to be able to use arguments on the command line a la MSVC's `cl.exe`. We were actually the original contributors of a heuristic to detect this in the `cc` crate, and it's served us well. In `cc` upstream since 1.0.89, a new heuristic for detecting compiler families in `cc::Tool` was introduced which does not have the desired behavior, and misclassifies the above case as being `clang`-like, rather than `cl`-like. The heuristic we originally submitted upstream is now in a fallback path which does not get used for our case. This causes `cc`'s default flags and APIs like `cc::Tool::is_like_msvc` to be incorrect. `swgl`, in particular, breaks because of this, since it's opinionated on the arguments it wants to provide to compilers. Work around the above regression by detecting checking `Tool`s' base command and "wrapper arguments" to see if we have a wrapper argument matching `--driver-mode=cl`. If so, provide `cl`-like arguments in `swgl`, rather than `clang`-like arguments. This behavior has been fixed upstream; see <rust-lang/cc-rs#1378>. Once released, we can consume it and revert this patch. Differential Revision: https://phabricator.services.mozilla.com/D236305

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…fying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` (rust-lang#1378)

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

… r=#gfx-reviewers This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

…<try> Bump bootstrap cc to 1.2.14 and cmake to 0.1.54 ## Summary Bump bootstrap's `cc` and `cmake` deps: 1. To make it easier to add new/unofficial targets. In particular, `cc` v1.2.4+ allows using env vars to pass target parameters to the `cc` crate. This was previously attempted in rust-lang#134344 but ran into macos-cross-to-iOS problems with `cmake` (and also rust-lang#136440, rust-lang#136720). See also discussions in rust-lang/cc-rs#1317. 2. Fix some flag inheritance warnings. Fixes rust-lang#136338. ## `cc` changelogs between `1.2.0` and `1.2.14` > ## [1.2.14](rust-lang/cc-rs@cc-v1.2.13...cc-v1.2.14) - 2025-02-14 > > ### Other > > - Regenerate target info ([rust-lang#1398](rust-lang/cc-rs#1398)) > - Add support for setting `-gdwarf-{version}` based on RUSTFLAGS ([rust-lang#1395](rust-lang/cc-rs#1395)) > - Add support for alternative network stack io-sock on QNX 7.1 aarch64 and x86_64 ([rust-lang#1312](rust-lang/cc-rs#1312)) > > ## [1.2.13](rust-lang/cc-rs@cc-v1.2.12...cc-v1.2.13) - 2025-02-08 > > ### Other > > - Fix cross-compiling for Apple platforms ([rust-lang#1389](rust-lang/cc-rs#1389)) > > ## [1.2.12](rust-lang/cc-rs@cc-v1.2.11...cc-v1.2.12) - 2025-02-04 > > ### Other > > - Split impl Build ([rust-lang#1382](rust-lang/cc-rs#1382)) > - Don't specify both `-target` and `-mtargetos=` on Apple targets ([rust-lang#1384](rust-lang/cc-rs#1384)) > > ## [1.2.11](rust-lang/cc-rs@cc-v1.2.10...cc-v1.2.11) - 2025-01-31 > > ### Other > > - Fix more flag inheritance ([rust-lang#1380](rust-lang/cc-rs#1380)) > - Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` ([rust-lang#1378](rust-lang/cc-rs#1378)) > - Constrain `-Clto` and `-Cembed-bitcode` flag inheritance to be `clang`-only ([rust-lang#1379](rust-lang/cc-rs#1379)) > - Pass deployment target with `-m*-version-min=` ([rust-lang#1339](rust-lang/cc-rs#1339)) > - Regenerate target info ([rust-lang#1376](rust-lang/cc-rs#1376)) > > ## [1.2.10](rust-lang/cc-rs@cc-v1.2.9...cc-v1.2.10) - 2025-01-17 > > ### Other > > - Fix CC_FORCE_DISABLE=0 evaluating to true ([rust-lang#1371](rust-lang/cc-rs#1371)) > - Regenerate target info ([rust-lang#1369](rust-lang/cc-rs#1369)) > - Make hidden lifetimes explicit. ([rust-lang#1366](rust-lang/cc-rs#1366)) > > ## [1.2.9](rust-lang/cc-rs@cc-v1.2.8...cc-v1.2.9) - 2025-01-12 > > ### Other > > - Don't pass inherited PGO flags to GNU compilers (rust-lang#1363) > - Adjusted zig cc judgment and avoided zigbuild errors([rust-lang#1360](rust-lang/cc-rs#1360)) ([rust-lang#1361](rust-lang/cc-rs#1361)) > - Fix compilation on macOS using clang and fix compilation using zig-cc ([rust-lang#1364](rust-lang/cc-rs#1364)) > > ## [1.2.8](rust-lang/cc-rs@cc-v1.2.7...cc-v1.2.8) - 2025-01-11 > > ### Other > > - Add `is_like_clang_cl()` getter (rust-lang#1357) > - Fix clippy error in lib.rs ([rust-lang#1356](rust-lang/cc-rs#1356)) > - Regenerate target info ([rust-lang#1352](rust-lang/cc-rs#1352)) > - Fix compiler family detection issue with clang-cl on macOS ([rust-lang#1328](rust-lang/cc-rs#1328)) > - Update `windows-bindgen` dependency ([rust-lang#1347](rust-lang/cc-rs#1347)) > - Fix clippy warnings ([rust-lang#1346](rust-lang/cc-rs#1346)) > > ## [1.2.7](rust-lang/cc-rs@cc-v1.2.6...cc-v1.2.7) - 2025-01-03 > > ### Other > > - Regenerate target info ([rust-lang#1342](rust-lang/cc-rs#1342)) > - Document new supported architecture names in windows::find > - Make is_flag_supported_inner take an &Tool ([rust-lang#1337](rust-lang/cc-rs#1337)) > - Fix is_flag_supported on msvc ([rust-lang#1336](rust-lang/cc-rs#1336)) > - Allow using Visual Studio target names in `find_tool` ([rust-lang#1335](rust-lang/cc-rs#1335)) > > ## [1.2.6](rust-lang/cc-rs@cc-v1.2.5...cc-v1.2.6) - 2024-12-27 > > ### Other > > - Don't inherit the `/Oy` flag for 64-bit targets ([rust-lang#1330](rust-lang/cc-rs#1330)) > > ## [1.2.5](rust-lang/cc-rs@cc-v1.2.4...cc-v1.2.5) - 2024-12-19 > > ### Other > > - Check linking when testing if compiler flags are supported ([rust-lang#1322](rust-lang/cc-rs#1322)) > > ## [1.2.4](rust-lang/cc-rs@cc-v1.2.3...cc-v1.2.4) - 2024-12-13 > > ### Other > > - Add support for C/C++ compiler for Neutrino QNX: `qcc` ([rust-lang#1319](rust-lang/cc-rs#1319)) > - use -maix64 instead of -m64 ([rust-lang#1307](rust-lang/cc-rs#1307)) > > ## [1.2.3](rust-lang/cc-rs@cc-v1.2.2...cc-v1.2.3) - 2024-12-06 > > ### Other > > - Improve detection of environment when compiling from msbuild or msvc ([rust-lang#1310](rust-lang/cc-rs#1310)) > - Better error message when failing on unknown targets ([rust-lang#1313](rust-lang/cc-rs#1313)) > - Optimize RustcCodegenFlags ([rust-lang#1305](rust-lang/cc-rs#1305)) > > ## [1.2.2](rust-lang/cc-rs@cc-v1.2.1...cc-v1.2.2) - 2024-11-29 > > ### Other > > - Inherit flags from rustc ([rust-lang#1279](rust-lang/cc-rs#1279)) > - Add support for using sccache wrapper with cuda/nvcc ([rust-lang#1304](rust-lang/cc-rs#1304)) > - Fix msvc stdout not shown on error ([rust-lang#1303](rust-lang/cc-rs#1303)) > - Regenerate target info ([rust-lang#1301](rust-lang/cc-rs#1301)) > - Fix compilation of C++ code for armv7-unknown-linux-gnueabihf ([rust-lang#1298](rust-lang/cc-rs#1298)) > - Fetch target info from Cargo even if `Build::target` is manually set ([rust-lang#1299](rust-lang/cc-rs#1299)) > - Fix two files with different extensions having the same object name ([rust-lang#1295](rust-lang/cc-rs#1295)) > - Allow disabling cc's ability to compile via env var CC_FORCE_DISABLE ([rust-lang#1292](rust-lang/cc-rs#1292)) > - Regenerate target info ([rust-lang#1293](rust-lang/cc-rs#1293)) > > ## [1.2.1](rust-lang/cc-rs@cc-v1.2.0...cc-v1.2.1) - 2024-11-14 > > ### Other > > - When invoking `cl -?`, set stdin to null ([rust-lang#1288](rust-lang/cc-rs#1288)) ## `cmake` changelogs `0.1.51` to `0.1.54` > ## [0.1.54](rust-lang/cmake-rs@v0.1.53...v0.1.54) - 2025-02-10 > > ### Other > > - Remove workaround for broken `cc-rs` versions ([rust-lang#235](rust-lang/cmake-rs#235)) > - Be more precise in the description of `register_dep` ([rust-lang#238](rust-lang/cmake-rs#238)) > > ## [0.1.53](rust-lang/cmake-rs@v0.1.52...v0.1.53) - 2025-01-27 > > ### Other > > - Disable broken Make jobserver support on OSX to fix parallel builds ([rust-lang#229](rust-lang/cmake-rs#229)) > > ## [0.1.52](rust-lang/cmake-rs@v0.1.51...v0.1.52) - 2024-11-25 > > ### Other > > - Expose cc-rs no_default_flags for hassle-free cross-compilation ([rust-lang#225](rust-lang/cmake-rs#225)) > - Add a `success` job to CI > - Change `--build` to use an absolute path > - Merge pull request [rust-lang#195](rust-lang/cmake-rs#195) from meowtec/feat/improve-fail-hint > - Improve hint for cmake not installed in Linux (code 127) > > ## [0.1.51](rust-lang/cmake-rs@v0.1.50...v0.1.51) - 2024-08-15 > > ### Added > > - Add JOM generator support ([rust-lang#183](rust-lang/cmake-rs#183)) > - Improve visionOS support ([rust-lang#209](rust-lang/cmake-rs#209)) > - Use `Generic` for bare-metal systems ([rust-lang#187](rust-lang/cmake-rs#187)) > > ### Fixed > > - Fix cross compilation on android armv7 and x86 ([rust-lang#186](rust-lang/cmake-rs#186)) try-job: dist-apple-various

… r=gfx-reviewers,lsalzman This is no longer needed with an update to a `cc` consuming <rust-lang/cc-rs#1378>. 🎉 Differential Revision: https://phabricator.services.mozilla.com/D236650

refactor: use find_map-strip_suffix instead of find-map-`spli…

4e1c4e6

…t` for `--driver-mode=…` search

ErichDonGubler force-pushed the restore-clang-cl-detection branch from 9eefc45 to 78469d1 Compare January 31, 2025 04:03

refactor: move both sides of --driver-mode=cl check to `Tool::get_b…

dfb5491

…ase_compiler` Do this by plumbing through `args` associated with the compiler wrapper, if any, instead of handling it in calling code.

ErichDonGubler force-pushed the restore-clang-cl-detection branch from 78469d1 to e50b416 Compare January 31, 2025 04:06

chore: type CompilerFamilyLookupCache to preempt `clippy::type_comp…

50a3f70

…lexity`

ErichDonGubler force-pushed the restore-clang-cl-detection branch from e50b416 to c5c1a91 Compare January 31, 2025 04:08

madsmtm mentioned this pull request Jan 31, 2025

chore: release v1.2.11 #1377

Merged

madsmtm reviewed Jan 31, 2025

View reviewed changes

NobodyXu requested changes Jan 31, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/tool.rs Outdated Show resolved Hide resolved

src/tool.rs Outdated Show resolved Hide resolved

ErichDonGubler force-pushed the restore-clang-cl-detection branch 2 times, most recently from ffe0e41 to 17c805a Compare January 31, 2025 17:26

ErichDonGubler requested a review from NobodyXu January 31, 2025 17:27

NobodyXu requested changes Jan 31, 2025

View reviewed changes

src/tool.rs Outdated Show resolved Hide resolved

src/tool.rs Outdated Show resolved Hide resolved

src/tool.rs Outdated Show resolved Hide resolved

ErichDonGubler added 2 commits January 31, 2025 13:19

fix: include wrapper args. in stdout family heuristics

002c6d8

This can be particularly significant for compilers that can dynamically change what options they accept based on arguments, like `clang --driver-mode=cl`.

test: cfg Test import behind not(windows)

5d29081

ErichDonGubler force-pushed the restore-clang-cl-detection branch from 17c805a to 5d29081 Compare January 31, 2025 18:19

ErichDonGubler requested a review from NobodyXu January 31, 2025 18:21

NobodyXu approved these changes Jan 31, 2025

View reviewed changes

NobodyXu merged commit 3a4a86d into rust-lang:main Jan 31, 2025
52 checks passed

ErichDonGubler deleted the restore-clang-cl-detection branch January 31, 2025 19:45

ErichDonGubler mentioned this pull request Feb 4, 2025

test: ensure clang --driver-mode=cl is MSVC- and clang-cl-like #1381

Merged

ErichDonGubler added a commit to erichdongubler-contrib/cc-rs that referenced this pull request Feb 9, 2025

Include wrapper args. in stdout family heuristics to restore classi…

750671b

…fying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` (rust-lang#1378)

jieyouxu mentioned this pull request Feb 14, 2025

Bump bootstrap cc to 1.2.14 and cmake to 0.1.54 rust-lang/rust#137022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` #1378

Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` #1378

ErichDonGubler commented Jan 31, 2025 •

edited

Loading

madsmtm left a comment

NobodyXu left a comment

ErichDonGubler commented Jan 31, 2025

ErichDonGubler commented Jan 31, 2025

NobodyXu commented Jan 31, 2025

NobodyXu left a comment •

edited

Loading

ErichDonGubler commented Jan 31, 2025

madsmtm commented Jan 31, 2025

Include wrapper args. in stdout family heuristics to restore classifying clang --driver-mode=cl as Msvc { clang_cl: true } #1378

Include wrapper args. in stdout family heuristics to restore classifying clang --driver-mode=cl as Msvc { clang_cl: true } #1378

Conversation

ErichDonGubler commented Jan 31, 2025 • edited Loading

madsmtm left a comment

Choose a reason for hiding this comment

NobodyXu left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Jan 31, 2025

ErichDonGubler commented Jan 31, 2025

NobodyXu commented Jan 31, 2025

NobodyXu left a comment • edited Loading

Choose a reason for hiding this comment

ErichDonGubler commented Jan 31, 2025

madsmtm commented Jan 31, 2025

Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` #1378

Include wrapper args. in `stdout` family heuristics to restore classifying `clang --driver-mode=cl` as `Msvc { clang_cl: true }` #1378

ErichDonGubler commented Jan 31, 2025 •

edited

Loading

NobodyXu left a comment •

edited

Loading