Skip to content

Simplify CMakeLists.txt#42

Merged
camio merged 2 commits intobemanproject:mainfrom
camio:simplify-cmakelists
Oct 8, 2024
Merged

Simplify CMakeLists.txt#42
camio merged 2 commits intobemanproject:mainfrom
camio:simplify-cmakelists

Conversation

@camio
Copy link
Copy Markdown
Member

@camio camio commented Oct 2, 2024

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

@camio
Simplify CMakeLists.txt
1fb6b07 
The `block()` isn't needed since we can set CMake options
via. CMAKE_ARGS.
@camio
Copy link
Copy Markdown
Member Author

camio commented Oct 2, 2024

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

@ptsouchlos
Copy link
Copy Markdown
Member

ptsouchlos commented Oct 2, 2024 •
edited
Loading

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

wusatosi commented Oct 2, 2024 •
edited
Loading

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

@ptsouchlos
Copy link
Copy Markdown
Member

ptsouchlos commented Oct 3, 2024

Nice catch @wusatosi !

@camio
Remove CMAKE_ARGS parameter that wasn't working.
d5b1e63 
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
Copy Markdown
Member Author

camio commented Oct 8, 2024 •
edited
Loading

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.

wusatosi
wusatosi approved these changes Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
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!.

ptsouchlos
ptsouchlos approved these changes Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ptsouchlos ptsouchlos 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
@camio camio deleted the simplify-cmakelists branch October 8, 2024 22:18
@camio camio mentioned this pull request Dec 5, 2024
Closed
@camio camio mentioned this pull request Feb 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ptsouchlos ptsouchlos ptsouchlos approved these changes

@neatudarius neatudarius neatudarius approved these changes

@wusatosi wusatosi wusatosi approved these changes

@bretbrownjr bretbrownjr Awaiting requested review from bretbrownjr bretbrownjr is a code owner

@dietmarkuehl dietmarkuehl Awaiting requested review from dietmarkuehl dietmarkuehl is a code owner

@steve-downey steve-downey Awaiting requested review from steve-downey steve-downey is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@camio @ptsouchlos @wusatosi @neatudarius