corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.1k stars 106 forks source link

clang,windows: fix passing link flags when using clang in GNU frontend mode on windows #566

Closed russelltg closed 1 month ago

russelltg commented 1 month ago

Followup to #511 but adding support for GNU-style clang flags.

The condition is really CMAKE_CXX_COMPILER_FRONTEND_VARIANT==GNU && CMAKE_CXX_LINK_EXECUTABLE starts with ${CMAKE_CXX_COMPILER}, but they are always set together, and I doubt cmake would change that.

For msvc frontends, cmake directly invokes the linker so the passing the flags verbatim is still what we want to do.

jschwe commented 1 month ago

Looks good to me, but it wouldn't harm to add a CI test for clang with gnu frontend targetting msvc abi. How does CMake need to be configured compared to when using clang-cl?

russelltg commented 1 month ago

You just use clang as compiler instead of clang-cl and use GNU style flags. I'll take a look at adding a test

russelltg commented 1 month ago

This is only required in 1.81+, so temporarily bumping rust version in CI to make sure it fails with the expected error, then i'll un-revert

russelltg commented 1 month ago

I'm realizing if we had just updated corrosion it would have worked fine because of #537, oops.

I still think this is a good change, but I'll fix it's interaction with 537

jschwe commented 1 month ago

I'm realizing if we had just updated corrosion it would have worked fine because of https://github.com/corrosion-rs/corrosion/pull/537, oops.

I'm confused now - Is the -Wl, not needed after all? There are also other libraries that rustc requires us to link for msvc, so I would have expected to see a CI failure if Wl, was required (on the "bump again?" commit).

russelltg commented 1 month ago

The -Wl, is required when using clang to invoke the linker.

This fix isn't necessary in my case because #537 removes /defaultlib:msvcrt entirely from the link flags, and it was the only link flag being passed, so there are no flags that need prefixing with -Wl,.

So this change is still correct, just not needed on the current rust compiler which /defaultlib:msvcrt is the only flag it reports.