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

Simplify CMakeLists.txt #42

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Conversation

camio
Copy link
Contributor

@camio camio commented Oct 2, 2024

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

The `block()` isn't needed since we can set CMake options
via. CMAKE_ARGS.
@camio
Copy link
Contributor Author

camio commented Oct 2, 2024

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

@DeveloperPaul123
Copy link
Member

DeveloperPaul123 commented Oct 2, 2024

@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
Copy link
Member

wusatosi commented Oct 2, 2024

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
Copy link
Member

Nice catch @wusatosi !

CMAKE_ARGS isn't used when a directory is brought in via
`add_subdirectory`. Instead, variables need to be set in a block.
Additionally, the block is only needed for the
`FetchContent_MakeAvailable` call.
@camio
Copy link
Contributor Author

camio commented Oct 8, 2024

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.

Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Turns out we still need the block directive at the end :(
But its nice to know the BUILD_TESTING option is off correctly for google test now!.

Copy link
Member

@DeveloperPaul123 DeveloperPaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Nice work 👍

@camio camio merged commit 325a07f into bemanproject:main Oct 8, 2024
27 checks passed
@camio camio deleted the simplify-cmakelists branch October 8, 2024 22:18
@camio camio mentioned this pull request Dec 5, 2024
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 this pull request may close these issues.

4 participants