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

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

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

russelltg
Copy link
Contributor

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
Copy link
Collaborator

jschwe commented Oct 6, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

jschwe commented Oct 7, 2024

I'm realizing if we had just updated corrosion it would have worked fine because of #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
Copy link
Contributor Author

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.

@jschwe jschwe merged commit 3ec73d8 into corrosion-rs:master Oct 8, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants