search

bemanproject / exemplar

Example Beman project
Other
11 stars 16 forks source link

Simplify CMakeLists.txt #42

Closed camio closed 1 month ago

camio commented 1 month ago

The block() isn't needed since we can set CMake options via. CMAKE_ARGS.

camio commented 1 month ago

This, surprisingly, didn't work as gtest files are being installed again with this change.

DeveloperPaul123 commented 1 month ago

@camio Are you using the necessary version of CMake (3.30+)? It could be that we also need to change cmake_minimum_required(VERSION 3.25) to cmake_minimum_required(VERSION 3.25...3.30.4) to properly set all the policies.

wusatosi commented 1 month ago

I don't see this being supported?

Use of CMAKE_ARGS is not mentioned in the fetch content documentation.

Yes it's not supported, see: https://discourse.cmake.org/t/fetchcontent-declare-with-cmake-args-does-not-work/11227/7

DeveloperPaul123 commented 1 month ago

Nice catch @wusatosi !

camio commented 1 month ago

Okay, I think I figured this out. Thanks @wusatosi and @DeveloperPaul123 for your help.

  1. CMAKE_ARGS is ignored by FetchContent_Declare. The documentation states "The configure, build, install, and test steps are explicitly disabled, so options related to those steps will be ignored." and CMAKE_ARGS is listed as a "configure" option.
  2. The add_subdirectory call is made in FetchContent_MakeAvailable so it makes sense to have the FetchContent_Declare call outside of the block.
  3. The use of CMAKE_ARGS to disable BUILD_TESTING appeared to work only because GoogleTest disables tests by default and uses a different variable to control testing.
  4. Although BUILD_TESTING doesn't do anything in this case, I think we should keep it there as other dependency projects will use the standard variable for this.

@wusatosi , @DeveloperPaul123 can you give this PR another look now? I've verified in the CI output that this works as expected.