search

rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.79k stars 434 forks source link

fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

Closed russelltg closed 4 months ago

russelltg commented 5 months ago

This PR contains several clang on windows improvements:

  1. properly detect clang vs clang-cl by invoking with -?, instead of checking for _MSC_VER. This is what cmake does, and _MSC_VER is always defined for msvc ABI targets, regardless of the frontend variant used
  2. Fix detection of which output assembler flag to use. It used to use complex logic, including if the C compiler is clang, which was incorrect. Use the flag based on if it's using the msvc assembler
  3. Add clang-cl/clang CI for windows
  4. Detect usage of llvm-ar and pass GNU style flags to it

Original description:

this is somewhat a nasty hack here, but it does work. when (cross) compiling for msvc and you use llvm-ar as the archiver, then you need to use gnu-style flags.

currently it attemps to run llvm-ar like:

running: "/usr/bin/llvm-ar" "-out:/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/liblink-cplusplus.a" "-nologo" "/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/0466faf07398e270-dummy.o"

which doesn't work, as llvm-ar takes in GNU-style ar flags.

i'm definitely open to a more robust way to get the archiver "Family" (maybe something similar to how we detect compiler family? idk) if that would be better

I also moved some APIs to to use Path as it avoided a string->path conversion. Let me know if that's not of interest.

With this and the other PR, I can now cross-compile to windows using clang from linux :)

EDIT: also fixes the path to the archiver not being set on when env_tool finds it

russelltg commented 5 months ago

While working on that CI pass, I found that the clang-cl detection doesn't work at all....

Clang defines _MSC_VER when target is *msvc, and actually has no relation to the frontend variant. That combined with us not passing C[XX]FLAGS to the -E invocation just means that all it detects is if the host system is windows, lol....

russelltg commented 5 months ago

While I'm working on clang cross compilation for linux->windows, I was going to add the CI pass to windows so I don't have to worry about getting windows SDK/msvc installed in Linux. But since the host is windows it ends up thinking clang is clang-cl!

russelltg commented 5 months ago

I know you're waiting on this to release--I think it's still a good change of course, but that CI is not going to work yet. Want me to rip out CI and open a new PR with hopefully some way to fix clang-cl detection, that can be merged after the release?

NobodyXu commented 5 months ago

Want me to rip out CI and open a new PR with hopefully some way to fix clang-cl detection, that can be merged after the release?

I would prefer the clang-cl to be fixed as well before cutting a new release to avoid regression.

NobodyXu commented 5 months ago

BTW, can you please update the PR title and description?

Thanks!!

russelltg commented 5 months ago

Okay, this seems to work now 😅

It ended up being more changes than I thought! Do you want me to split into mutiple PRs? Im also happy to rebase it into a cleaner incremental history for future bisecting

russelltg commented 5 months ago

Hmm it's hard to be sure that the env vars are actually being set....

russelltg commented 5 months ago

Ah, I don't think it's possible to emit multiple env vars from GITHUB_ENV? At least I can't figure it out.

which is kinda wild

russelltg commented 5 months ago

Ok, not quite ready I guess. Will look tomorrow

NobodyXu commented 5 months ago

Ah, I don't think it's possible to emit multiple env vars from GITHUB_ENV? At least I can't figure it out.

The doc says just put them in multiple lines, i.e. echo -e "A=...\nB=..." >> "$GITHUB_ENV"

NobodyXu commented 4 months ago

Updated CI and code, still doesn't work

russelltg commented 4 months ago

Ok, phew, there was some more cases to handle here, but it seems to be working.

russelltg commented 4 months ago

I'm not sure if you want to keep the changes you made, notably the CI formatting changes. I'm happy to simplify that diff if you want.

NobodyXu commented 4 months ago

I'm not sure if you want to keep the changes you made, notably the CI formatting changes. I'm happy to simplify that diff if you want.

That'd be ok for me, thanks for yhe fix!

russelltg commented 4 months ago

Opened https://github.com/llvm/llvm-project/pull/87209 to address clang-cl not taking /Fo: but let's definitely stick with /Fo for maximum compatibility.

NobodyXu commented 4 months ago

That said, maybe we should squash-land this so that we can revert more easily if it causes problems.

Yeah, cc-rs defaults to squash merge so it should be pretty easy to revert.