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

Disable linker flag detection on MSVC/ClangCL. #3569

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

tru
Copy link
Contributor

@tru tru commented Mar 22, 2023

This fixes compilation with clang-cl on Windows. There
is a bug in cmake so that check_linker_flag() doesn't give
the correct result when using link.exe/lld-link.exe.

Details in CMake's gitlab: https://gitlab.kitware.com/cmake/cmake/-/issues/22023

Fixes #3522

This fixes compilation with clang-cl on Windows. There
is a bug in cmake so that check_linker_flag() doesn't give
the correct result when using link.exe/lld-link.exe.

Details in CMake's gitlab: https://gitlab.kitware.com/cmake/cmake/-/issues/22023

Fixes facebook#3522
@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 23, 2023

The fix looks fine,
but I wonder if we could add, or update, a test,
that would trigger this problem without this fix.
This way, we would ensure that this property remains respected in the future.

I suspected we used to have a Windows clang-cl test on Appveyor,
but somehow it was lost in the transition towards Github Actions.

@tru
Copy link
Contributor Author

tru commented Mar 23, 2023

When you say test - do you mean a build bot in this case that would build zstd with clang-cl? I am not very familiar with the github actions workers and configurations, maybe someone else could help with that?

@Cyan4973
Copy link
Contributor

When you say test - do you mean a build bot in this case that would build zstd with clang-cl? I am not very familiar with the github actions workers and configurations, maybe someone else could help with that?

I'll look into it

@Cyan4973 Cyan4973 self-assigned this Mar 23, 2023
Cyan4973 added a commit that referenced this pull request Mar 29, 2023
If I understand correctly,
this should trigger the issue notified in #3569.
Cyan4973 added a commit that referenced this pull request Mar 29, 2023
If I understand correctly,
this should trigger the issue notified in #3569.
@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 29, 2023

I added a Clang-CL test in #3579, and it fails at link stage, as expected.

Now, I wonder if the exact way it fails is the expected one, as I don't recognise the gitlab issue :
https://github.com/facebook/zstd/actions/runs/4549108695/jobs/8020846731?pr=3579

lld-link : warning : ignoring unknown argument '-z' [D:\a\zstd\zstd\build\cmake\build\programs\zstd.vcxproj]
lld-link : error : could not open 'noexecstack': no such file or directory [D:\a\zstd\zstd\build\cmake\build\programs\zstd.vcxproj]

If yes, then it means that merging this PR, then rebasing #3579 on top of it, should fix the problem.

@Cyan4973 Cyan4973 merged commit 871f3a4 into facebook:dev Mar 29, 2023
Cyan4973 added a commit that referenced this pull request Mar 29, 2023
If I understand correctly,
this should trigger the issue notified in #3569.
@tru
Copy link
Contributor Author

tru commented Mar 29, 2023

Great! Thanks for the CI build, that hopefully means that our configuration will continue to work in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building 1.5.4 with clang-cl and lld-link fails because faulty linker checks in CMake.
3 participants