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

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

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