Skip to content
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

Fix compiler family detection issue with clang-cl on macOS #1328

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

NobodyXu
Copy link
Collaborator

Fixed #1327

@NobodyXu NobodyXu requested a review from madsmtm December 22, 2024 13:57
@NobodyXu
Copy link
Collaborator Author

I can't know for sure if the compiler is clang-cl, so can't apply the workaround reliably.

Alternatively, we could combine cfg(macos) with accept cl flags check.

if both are true then it's likely clang-cl on macOS, which needs workaroud.

@NobodyXu NobodyXu requested a review from thomcc December 23, 2024 00:13
@madsmtm madsmtm added the bug label Jan 1, 2025
Copy link
Collaborator

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

It is not clear to me why we even support clang-cl on macOS and Linux? cl.exe is not something you'd run on those platforms, so why would you ever run clang-cl? (Not a blocker for fixing this, just something I'd like to understand ;) )

cfg(macos)

That's a bad idea IMO, since this problem is also present on Linux and Windows hosts, just less prominently since their root paths usually start with a lowercase letter (on Linux), or prefixed with the drive letter (on Windows), but that's not a requirement!

Actually, the -- separator seems to be supported by normal Clang too, so why don't we just do this workaround everywhere?

This would also avoid issues if the user did e.g. .file("-l").

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Jan 2, 2025

It is not clear to me why we even support clang-cl on macOS and Linux? cl.exe is not something you'd run on those platforms, so why would you ever run clang-cl?

Based on the original issue, I believe they are using cargo-xwin to cross compile to msvc windows from a unix environment.

Actually, the -- separator seems to be supported by normal Clang too, so why don't we just do this workaround everywhere?

Does gcc and other compiler support it?

If they mostly do, we can add -- to compiler detection.

This would also avoid issues if the user did e.g. .file("-l").

Good idea

@madsmtm
Copy link
Collaborator

madsmtm commented Jan 2, 2025

Does gcc and other compiler support it?

$ touch foo.c
$ clang -c -- foo.c
$ gcc -c -- foo.c
gcc: error: unrecognized command-line option '--'

Doesn't seem like it?

NobodyXu and others added 2 commits January 7, 2025 00:50
Fixed #1327

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
to disambiguous path from arguments.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu requested a review from madsmtm January 6, 2025 14:20
"Expects macro `__clang__`, `__GNUC__` or `__EMSCRIPTEN__`, `__VXWORKS__` or accepts cl style flag `-?`, but found none",
))
}
if stdout.contains("-Wslash-u-filename") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This... Seems brittle :/.

Not that I have a better solution (yet).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like it either, but I really can't think of an alternative solution that is better

Copy link
Collaborator

@madsmtm madsmtm Jan 7, 2025

Choose a reason for hiding this comment

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

What does CMake do? (if we're already following in their footsteps)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure , I didn't check about cmake.

I did remember the -? solution is copied from it, so perhaps it uses that as a hint?

Copy link
Collaborator

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I don't feel I know enough to really gauge whether this is the right approach or not, so I'll leave that up to you.

I don't think GitHub allows me to make a neutral review after having requested changes, so just so that won't block you, I've approved it.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Jan 8, 2025

Thanks, let me merge it and try shipping it, if it doesn't work we have to revert and find another solution

@NobodyXu NobodyXu merged commit b886474 into main Jan 8, 2025
104 checks passed
@NobodyXu NobodyXu deleted the NobodyXu-patch-1 branch January 8, 2025 23:13
@github-actions github-actions bot mentioned this pull request Jan 10, 2025
@messense
Copy link

Unfortunately it seems to broke cargo-zigbuild badly: https://github.com/rust-cross/cargo-zigbuild/actions/runs/12723439506/job/35468553100

error occurred in cc-rs: Command LC_ALL="C" "/home/runner/.cache/cargo-zigbuild/0.19.7/zigcc-aarch64-unknown-linux-gnu-6363.sh" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-gdwarf-4" "-fno-omit-frame-pointer" "--target=aarch64-unknown-linux-gnu" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-o" "/home/runner/work/cargo-zigbuild/cargo-zigbuild/tests/zstd-rs/target/aarch64-unknown-linux-gnu/debug/build/zstd-sys-c42cdd11ab2b8d1c/out/7faed3f8272f2313-huf_decompress_amd64.o" "-c" "--" "zstd/lib/decompress/huf_decompress_amd64.S" with args zigcc-aarch64-unknown-linux-gnu-6363.sh did not execute successfully (status code exit status: 1).

@messense
Copy link

And it does not seem to resolve the clang-cl issue either:

warning: libz-ng-sys@1.1.16: Compiler family detection failed due to error: ToolExecError: Command "clang-cl" "-E" "/Users/messense/Projects/uv/target/aarch64-pc-windows-msvc/debug/build/libz-ng-sys-e348506bb081d40e/out/442624705732975273detect_compiler_family.c" with args clang-cl did not execute successfully (status code exit status: 1).

@tokatoka
Copy link

I think this will break if people uses cc with their own compiler wrappers

For our case, we use a compiler wrapper based on clang and in our wrapper we didn't handle argument -- .
This fix automatically adds "--" automatically, and we had to handle it else it broke.

@NobodyXu
Copy link
Collaborator Author

@messense @tokatoka I've opened #1359, can you try it out please?

@tbetcke
Copy link

tbetcke commented Jan 11, 2025

@NobodyXu I have the same issue with mpicc as compiler wrapper (e.g. used by [rsmpi])(https://github.com/rsmpi/rsmpi). Version 1.2.7 of cc-rs works fine. Version 1.2.8 fails with the -- issue. I have tried #1359. But the error still persists.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2025
…<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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler family detection issue with clang-cl on macOS
5 participants