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

Compiling broken on clang-cl #18

Closed
angelfor3v3r opened this issue Feb 19, 2023 · 4 comments · Fixed by #19
Closed

Compiling broken on clang-cl #18

angelfor3v3r opened this issue Feb 19, 2023 · 4 comments · Fixed by #19

Comments

@angelfor3v3r
Copy link
Contributor

angelfor3v3r commented Feb 19, 2023

While testing #13 I've ran into the issue where the library refuses to build under clang-cl. It seems like commit 3393ced is the cause of it.

The error seems to stem from the use of -pedantic:

clang-cl: error: unknown argument ignored in clang-cl: '-pedantic' [-Werror,-Wunknown-argument]

The other thing I noticed is clang-cl gets picked up as both "msvc" and "clang" here, so both compile flags are used from these sections.
This is most likely just the fault of cmkr not having great predefined conditions: https://cmkr.build/cmake-toml/#predefined-conditions

msvc.private-compile-options = ["/WX", "/permissive-", "/W4", "/w14640"]
clang.private-compile-options = ["-Werror", "-Wall", "-Wextra", "-Wshadow", "-Wnon-virtual-dtor", "-pedantic"]

# Both of these pass on clang-cl:
if(MSVC) # msvc
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_C_COMPILER_ID MATCHES "Clang") # clang

The final flags on my compiler look like this (this is only from Builder.cpp but it happens for all source files):

clang-cl.exe  /nologo -TP -DNOMINMAX -DZYDIS_STATIC_BUILD -I[REDACTED]\_deps\safetyhook-src\include -m32 /O2 /Ob2 /DNDEBUG -MD /WX /permissive- /W4 /w14640 -Werror -Wall -Wextra -Wshadow -Wnon-virtual-dtor -pedantic -std:c++17 /showIncludes /Fo_deps\safetyhook-build\CMakeFiles\safetyhook.dir\src\Builder.cpp.obj /Fd_deps\safetyhook-build\CMakeFiles\safetyhook.dir\safetyhook.pdb -c -- [REDACTED]\_deps\safetyhook-src\src\Builder.cpp

From what I know, clang flags passed to clang-cl must be passed like so: /clang:<flag>. However, It seems like most flags in the MSVC section would work fine. The only ones I cant find in the documentation are: /permissive and /w<warning id>. Either way, I don't think it's ideal that both MSVC and clang-cl flags are being mixed on the command line in this case since they're redundant.

I'm sure theres some work around for this but perhaps cmkr could have better predefined compiler detection conditions?

@mrexodia
Copy link

mrexodia commented Feb 19, 2023

This is a rather esoteric part of CMake (https://stackoverflow.com/a/10055571/1806760). Likely the default condition for cmkr is not strict enough. You can fix this locally in your project by overwriting the clang condition:

[conditions]
clang = "CMAKE_CXX_COMPILER_ID MATCHES \"Clang\" AND NOT CMAKE_CXX_SIMULATE_ID"

@mrexodia
Copy link

Also as a general note you shouldn't set -Werror as a compiler flag. This means that if the compiler improves over time your compilation might fail, which is a pain in the ass for consumers of your library. Compiler flags in the CMake should be seen as requirements to build your target (which is why they are PUBLIC).

CMake's "solution" is to use presets for this and manually specify the CMAKE_CXX_FLAGS variables there, which isn't ideal...

@mrexodia
Copy link

Should be fixed in cmkr v0.2.19

@angelfor3v3r
Copy link
Contributor Author

angelfor3v3r commented Feb 20, 2023

Should be fixed in cmkr v0.2.19

You’re awesome, thank you so much for the information and cmkr changes! I’ll be giving it a try soon.

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 a pull request may close this issue.

2 participants