jrouwe / JoltPhysics

A multi core friendly rigid body physics and collision detection library. Written in C++. Suitable for games and VR applications. Used by Horizon Forbidden West.
MIT License
6.77k stars 452 forks source link

Error when generating build files for Ninja and clang-cl #1211

Closed mihe closed 3 months ago

mihe commented 3 months ago

I'm still trying to figure out exactly what's going on, and it's not impossible that this actually is a bug in CMake, but it seems that #1206 broke something.

If you open up Visual Studio's "x64 Native Tools Command Prompt" and run this inside the Build folder:

cmake -S . -B Ninja_ClangCL -G "Ninja Multi-Config" -DCMAKE_MAKE_PROGRAM=C:/Path/To/ninja.exe -DCMAKE_CXX_COMPILER=clang-cl

You'll be met with this:

-- The CXX compiler identification is Clang 16.0.4 with MSVC-like command-line
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Path/To/clang-cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done (1.2s)
CMake Error: No output files for WriteBuild! called with comment: Additional output files.
CMake Error: No output files for WriteBuild! called with comment: Additional output files.
-- Generating done (0.5s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

Which seems to be caused by this:

https://github.com/jrouwe/JoltPhysics/blob/308dd5779af4da355f9ba9eff8fe8f583b799cc4/Build/CMakeLists.txt#L317

... and this:

https://github.com/jrouwe/JoltPhysics/blob/308dd5779af4da355f9ba9eff8fe8f583b799cc4/Jolt/Jolt.cmake#L504

More specifically the fact that these effectively evaluate to target_precompile_headers(... PRIVATE "") for the ReleaseCoverage configuration, and CMake doesn't seem to like that empty string there for some reason.

This works just fine with Ninja+MSVC though, so I feel like this is probably a CMake bug, but whatever is to blame it's proving to be a showstopper for me updating Jolt.

EDIT: I just realized that ReleaseCoverage configuration isn't present when using MSVC, so I guess that might explain the discrepancy, but things do seem to work fine on Linux with Ninja+Clang, which should have that configuration, so something odd is going on anyway.

jrouwe commented 3 months ago

Hmmm, the cmake documentation even suggests to use generator expressions here:

Projects should generally avoid using PUBLIC or INTERFACE for targets that will be exported, or they should at least use the $ generator expression to prevent precompile headers from appearing in an installed exported target.

Do you know if there is a generator expression detecting ninja, e.g.:

target_precompile_headers(Jolt PRIVATE "$<$<OR:$<NOT:$<CONFIG:ReleaseCoverage>>,$<BOOL:NINJA>>:${JOLT_PHYSICS_ROOT}/Jolt.h>") 

?

mihe commented 3 months ago

Do you know if there is a generator expression detecting ninja

I don't believe there is. The only variables I can find that hints towards the environment being Ninja is CMAKE_GENERATOR and possibly CMAKE_MAKE_PROGRAM, but I can't see any generator expression that lets you query those.

I've managed to reproduce this in a "Hello World"-style minimal repro, so I'll be sending this to CMake's issue tracker, as it smells a lot like a bug on their end.

Either way, assuming that everyone has the latest and greatest CMake (if/when this gets fixed on their end) somewhat defeats the purpose of cmake_minimum_required, so we'll need to figure out some kind of workaround I guess.

Maybe make the additional Clang-specific configurations (ReleaseASAN, ReleaseUBSAN, ReleaseCoverage) opt-in through a CMake option? I don't really need those on my end at least.

mihe commented 3 months ago

I filed a CMake issue here.

mihe commented 3 months ago

Do you know if there is a generator expression detecting ninja

I just realized you obviously wouldn't ever have different generators for different configurations anyway, so you could technically just do this I guess:

if(CMAKE_GENERATOR STREQUAL "Ninja Multi-Config" AND MSVC)
    target_precompile_headers(...)
else()
    target_precompile_headers(...)
endif()

EDIT: The single-configuration Ninja generator doesn't seem to have any issues, so it would only be necessary to check for Ninja Multi-Config.

jrouwe commented 3 months ago

I've implemented your suggestion. This means that coverage reports when compiling with Ninja will report all math code as 'not covered', but that seems like a minor thing compared to not being able to compile.

mihe commented 3 months ago

that seems like a minor thing compared to not being able to compile

For sure. :)

I can confirm that #1212 resolves this issue.