Prevent CMake from attempting to configure test executables without Catch2 #1380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…atch2 Previously, the CMake would check whether STDEXEC_BUILD_TESTS was set when determining whether CPM should add Catch2, but it would then check BUILD_TESTING when determining whether to add the CMake for the test subdirectory. If STDEXEC_BUILD_TESTS was enabled, this would work, because the CMake would enable BUILD_TESTING whenever STDEXEC_BUILD_TESTS was enabled. However, if BUILD_TESTING was enabled but STDEXEC_BUILD_TESTS was disabled, then the CMake would attempt to configure the tests without having added Catch2, causing the build configuration to fail. This was a particular problem when adding stdexec as a submodule of another project, since if that project had an include(CTest) command invocation before it added stdexec, and it did not explicitly enable STDEXEC_BUILD_TESTS, the build configuration would fail, since STDEXEC_BUILD_TESTS is disabled by default when stdexec is a submodule, and include(CTest) automatically sets BUILD_TESTING. This commit addresses the issue by setting BUILD_TESTING to whatever STDEXEC_BUILD_TESTS was set to, and then relying on BUILD_TESTING to decide both whether to add Catch2 and whether to configure the test executables. It also adds a check for BUILD_TESTING when determining how to set the default for STDEXEC_BUILD_TESTS when the user doesn't configure it. The outcome is that, if the user explicitly enables or disables STDEXEC_BUILD_TESTS, then the CMake will use it to override the preexisting BUILD_TESTING setting; but otherwise, the CMake will enable tests only if BUILD_TESTING is set or if stdexec is not a submodule.
ednolan
force-pushed
the
enolan_submoduletestcmake1
branch
from
f626596 to
7aec9ab
Compare
|
This addresses issue 1371: #1371 (comment) |
|
/ok to test |
|
Thanks! |
ednolan
added a commit
to bemanproject/utf_view
that referenced
this pull request
Nov 13, 2024
This commit adds a UTF_VIEW_BUILD_TESTS CMake option. The logic around
its use is:
- If the user has explicitly set UTF_VIEW_BUILD_TESTS, BUILD_TESTING
is set to that value
- Otherwise, if BUILD_TESTING was already set to ON, then it continues
to be set to ON
- Otherwise, if this is the top-level CMake project and not a
dependency of another project, BUILD_TESTING is set to ON
- Otherwise, BUILD_TESTING is set to OFF
Using BUILD_TESTING to control whether tests are enabled is
recommended by CMake in its documentation here:
https://cmake.org/cmake/help/latest/module/CTest.html
This logic is based on a similar option implemented in NVIDIA/stdexec.
My commit implementing this logic can be found here:
NVIDIA/stdexec@ee0a962
Additional reasoning can be found here:
NVIDIA/stdexec#1380 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the CMake would check whether STDEXEC_BUILD_TESTS was set when determining whether CPM should add Catch2, but it would then check BUILD_TESTING when determining whether to add the CMake for the test subdirectory. If STDEXEC_BUILD_TESTS was enabled, this would work, because the CMake would enable BUILD_TESTING whenever STDEXEC_BUILD_TESTS was enabled. However, if BUILD_TESTING was enabled but STDEXEC_BUILD_TESTS was disabled, then the CMake would attempt to configure the tests without having added Catch2, causing the build configuration to fail.
This was a particular problem when adding stdexec as a submodule of another project, since if that project had an include(CTest) command invocation before it added stdexec, and it did not explicitly enable STDEXEC_BUILD_TESTS, the build configuration would fail, since STDEXEC_BUILD_TESTS is disabled by default when stdexec is a submodule, and include(CTest) automatically sets BUILD_TESTING.
This commit addresses the issue by setting BUILD_TESTING to whatever STDEXEC_BUILD_TESTS was set to, and then relying on BUILD_TESTING to decide both whether to add Catch2 and whether to configure the test executables. It also adds a check for BUILD_TESTING when determining how to set the default for STDEXEC_BUILD_TESTS when the user doesn't configure it. The outcome is that, if the user explicitly enables or disables STDEXEC_BUILD_TESTS, then the CMake will use it to override the preexisting BUILD_TESTING setting; but otherwise, the CMake will enable tests only if BUILD_TESTING is set or if stdexec is not a submodule.