turanszkij / WickedEngine

3D engine with modern graphics
https://wickedengine.net
Other
5.85k stars 619 forks source link

cmake: replace explicit list of files with globs #887

Closed brakhane closed 4 months ago

brakhane commented 4 months ago

This PR is due to a recent discussion in Discord. I'm in two minds about this one. It prevents the problem of missing *.cpp files in the CMakeLists, and makes it a bit clearer which files are used for what (especially the Utility folder had a nice gotcha that wasn't obvious from the old CMakeLists), but it is also recommended against by the CMake team, however, I think the problem they mention is currently more theoretical.

Note that keeping CMakeLists.txt in the Template folder as-is is intentional: there's a edge case when a *.c file is listed in the root directory, so to prevent people from ever falling into that trap, and to follow CMake recommendation to not use globs, I've left it as it. If people want, they can always modify the cmakefile in their program based on the template.


CMake recommends against doing this because some build systems might not support CONFIGURE_DEPENDS. But currently, the ones we care about (Make, Ninja) do, and if there's ever a problem, we can just start listing them again.

Using globs has the advantage that the linux build will not break when a file is added in VS and somebody forgets to add the file to cmakelists as well. I've also found some instances were some *.h files were not listed, which means they weren't copied when installing.

brakhane commented 4 months ago

forgot something, brb

portaloffreedom commented 4 months ago

We need to set cmake_minimum_required(VERSION 3.12) for this feature. Let's set it in both the root cmakelists and in the wickedengine subfolder cmakelists.

brakhane commented 4 months ago

ok, found the problem. volk.c is not compiled in the old version.

brakhane commented 4 months ago

With all those special cases made explicit, I now think this is preferable to the old way of doing things; thinks like volk.c being included by volk.h was pretty subtle before

turanszkij commented 4 months ago

It looks ok to me, but from the conversation I'm not sure if it should be merged now, or is there anything else remaining?