CTest: Module must be included at the top level #145

pull purpleKarrot wants to merge 1 commits into bitcoin-core:master from purpleKarrot:fix-testing changing 2 files +1 −2
  1. purpleKarrot commented at 2:19 pm on February 1, 2025: contributor
    In order to be able to run ctest from the top level build directory, it is necessary that enable_testing() is invoked in the top level CMakeLists.txt file. The CTest module invokes enable_testing() based on the value of BUILD_TESTING.
  2. hebasto commented at 2:27 pm on February 1, 2025: member
    I don’t think this project should include(CTest) in the first place. Besides calling enable_testing(), it also performs some CDash-related actions, which are unnecessary.
  3. purpleKarrot commented at 2:37 pm on February 1, 2025: contributor

    The minimum replacement would be:

    0option(BUILD_TESTING "Build the testing tree." ON)
1
2include(CTestUseLaunchers)
3
4if(BUILD_TESTING)
5  enable_testing()
6endif()

    The CTestUseLaunchers module should not be omitted.

  4. hebasto commented at 2:38 pm on February 1, 2025: member

    The CTestUseLaunchers module should not be omitted.

    Why?

  5. purpleKarrot force-pushed on Feb 1, 2025
  6. purpleKarrot commented at 2:40 pm on February 1, 2025: contributor
    Because otherwise cmake produces an error when CTEST_USE_LAUNCHERS is on.
  7. ryanofsky commented at 5:50 pm on February 3, 2025: collaborator
    These changes look good to me and make sense to the extent I understand them (which is not very much), but it would be helpful to have some steps to reproduce, just to be able to see what was broken and compare different fixes that have been suggested here.
  8. purpleKarrot commented at 9:35 pm on February 3, 2025: contributor

    Try executing the following commands and see what error you get:

    0mkdir build
1cd build
2cmake ..
3make -j 10
4ctest -j 10

    The way you probably used to invoke testing on the command line is the following:

    0make -j 10 check

    This worked, because the check target is defined in a subdirectory where testing was enabled. However, there is a catch with this approach:

    What do you think how many tests are executed in parallel when the target check is built with -j 10? The answer is: one. The -j option is passed to the build tool, so the build tool runs 10 jobs in parallel. One of these jobs is to launch ctest without any options. So the tests are executed without any parellelization. (Which is not a big problem in this repository, because there is just one test anyway. But people will want to use the same approach (or user-specific aliases) to build and test any CMake project.)

    Therefore, the best recommendation is to run ctest explicitly, not via a build target. Make sure that the default target builds all tests when BUILD_TESTING is enabled, otherwise the tests will report failure because their executable is not found.

  9. ryanofsky commented at 12:50 pm on February 4, 2025: collaborator

    Thanks!

    The previous version of this PR 9d841a416c39a2ff31982f937ae5345bff9e5ee8 looks much more straightforward than current version 5f9162fbaa788db54bf9b1eda2dad0102b1be20c, and doesn’t add a new build option, so I would really prefer it. @hebasto you wrote https://github.com/chaincodelabs/libmultiprocess/pull/145#issuecomment-2628973879 “I don’t think this project should include(CTest) in the first place” because it is doing some unnecessary things. Why is this a problem? The build is written in cmake not rust so by definition is going to do a lot of unnecessary things, I would want to know what practical downsides to simple include(CTest) are if any

  10. hebasto commented at 1:04 pm on February 4, 2025: member

    @hebasto you wrote #145 (comment) “I don’t think this project should include(CTest) in the first place” because it is doing some unnecessary things. Why is this a problem? The build is written in cmake not rust so by definition is going to do a lot of unnecessary things, I would want to know what practical downsides to simple include(CTest) are if any

    For example, include(CTest) creates unused DartConfiguration.tcl in the build tree. I don’t think this is a user-facing drawback, though. However, this pertains to the intentions behind the code. If the intention is to enable test generation for the add_test() command, the CMake docs state:

    CMake only generates tests if the enable_testing() command has been invoked.

  11. in CMakeLists.txt:18 in 5f9162fbaa outdated 
     9@@ -10,6 +10,12 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
10   set(CMAKE_CXX_STANDARD_REQUIRED YES)
11 endif()
12 
13+option(BUILD_TESTING "Build the testing tree." ON)
14+if(BUILD_TESTING)
15+  enable_testing()
16+endif()
17+
18+include(CTestUseLaunchers)
    hebasto commented at 1:07 pm on February 4, 2025:
    What scenarios would this be useful for in this project?
  12. purpleKarrot commented at 1:28 pm on February 4, 2025: contributor

    Some background information about this DartConfiguration.tcl file: https://purplekarrot.net/blog/building-and-testing-with-cmake.html https://purplekarrot.net/blog/refactoring-ctest.html

    It is no longer up to date, because some of the refactoring has already be done.

  13. ryanofsky commented at 1:28 pm on February 4, 2025: collaborator

    I’d be fine with either calling include(CTest) or enable_testing() unconditionally, since the intention should be to always enable testing. I don’t think a separate build option should be required. If include(CTest) does unnecessary things, that’s an implementation detail of CMake, which should only matter if it has a noticeable impact on performance.

    So unless a bare enable_testing() call breaks something, I’d prefer using that. If it does break use cases, then include(CTest) would be fine.

  14. purpleKarrot commented at 2:22 pm on February 4, 2025: contributor

    Proprietary software is developed and tested by a single entity. Very often, the configuration for CI is located in the project’s repository and the CI configuration is intertwined with the project configuration (example: the project configuration has options for sanitizers and coverage).

    For software that is developed in public, a separation of project configuration and CI configuration brings advantages. Ideally, everyone is able to contribute CI resources, sometimes for platforms the project’s main developers do not even have access to. Project configuration should be written in a way that it supports the widest range of CI configurations and that just means: Make no assumption about what software is installed, what environment variables are set, and what cmake variables are set.

    This also includes the cmake variable CTEST_USE_LAUNCHERS: Don’t assume whether it is set or not, the project should be able to be configured in both cases. But when it is set and CTestUseLaunchers is not included, cmake will produce the following error:

    0CMake Error: CTEST_USE_LAUNCHERS is enabled, but the RULE_LAUNCH_COMPILE global property is not defined.
1Did you forget to include(CTest) in the toplevel CMakeLists.txt ?

    Hence, having include(CTest) in the toplevel CMakeLists.txt probably really is the most convenient approach. I agree that the added .tcl file and targets like NightlyCoverage are a bit annoying. But that is something that should be fixed in upstream cmake.

  15. purpleKarrot force-pushed on Feb 4, 2025
  16. in CMakeLists.txt:13 in e7329aed0f outdated 
     9@@ -10,6 +10,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
10   set(CMAKE_CXX_STANDARD_REQUIRED YES)
11 endif()
12 
13+include(CTest)
    ryanofsky commented at 3:58 pm on February 4, 2025:

    In commit “CTest: Module must be included at the top level” (e7329aed0f711400d7a14ce8d5edbede03b1e75b)

    Would it work to move this include down by other includes like include(GNUInstallDirs) below around line 45? This compat_find include on line 13 is meant to be a special case, not primary place where includes are located.

    Also, in case we do decide to make this include conditional based on whether a build option is set, or whether this is a top-level project it would be better if this happened below where more conditions should be known.

  17. ryanofsky approved
  18. ryanofsky commented at 3:59 pm on February 4, 2025: collaborator

    Code review ACK e7329aed0f711400d7a14ce8d5edbede03b1e75b.

    I like idea of project/ci separation and just in general, minimizing any assumptions we make about how this build will be used and keeping it generic. If there are practical problems with this include we can address them but at a high level this seems like a good approach.

  19. purpleKarrot force-pushed on Feb 4, 2025
  20. CTest: Module must be included at the top level 
    In order to be able to run `ctest` from the top level build directory,
it is necessary that `enable_testing()` is invoked in the top level
CMakeLists.txt file. The `CTest` module invokes `enable_testing()`
based on the value of `BUILD_TESTING`.
    63ac092ddd
  21. hebasto approved
  22. hebasto commented at 7:25 pm on February 4, 2025: member

    ACK 63ac092ddd8156b5eb56f2302b3d03ba739cf9ce, tested on Ubuntu 24.04:

    0$ cmake -B build
1$ cmake --build build -t mptests
2$ ctest --test-dir build
  23. ryanofsky merged this on Feb 5, 2025
  24. ryanofsky closed this on Feb 5, 2025
  25. ryanofsky referenced this in commit 1d75538a32 on Feb 5, 2025
  26. ryanofsky referenced this in commit 9437e6846f on Feb 7, 2025
  27. ryanofsky referenced this in commit a4a8f7a7ba on Feb 7, 2025
  28. hebasto commented at 7:31 am on February 8, 2025: member

    I don’t think this project should include(CTest) in the first place. Besides calling enable_testing(), it also performs some CDash-related actions, which are unnecessary.

    I forgot to mention another drawback of using include(CTest).

    This command introduces the BUILD_TESTING cache variable, which can sneak into downstream projects.

    For example, in https://github.com/bitcoin/bitcoin/pull/31741, after reconfiguring with -DENABLE_IPC=ON, BUILD_TESTING appears in the CMake cache along with the Bitcoin Core project’s BUILD_TESTS, which might be confusing at the very least.

  29. ryanofsky referenced this in commit 3a95817ece on Feb 10, 2025
  30. ryanofsky referenced this in commit a7f0669320 on Feb 10, 2025
  31. ryanofsky commented at 5:26 am on February 10, 2025: collaborator

    re: https://github.com/chaincodelabs/libmultiprocess/pull/145#issuecomment-2644563297

    This command introduces the BUILD_TESTING cache variable, which can sneak into downstream projects.

    Opened #161 to try to fix this, which gets rid of BUILD_TESTING but has other side effects. If you have ideas on how to improve it, would be helpful to have feedback there.

  32. ryanofsky referenced this in commit 011fc53aea on Feb 10, 2025
  33. Sjors referenced this in commit 6aabfcb615 on Feb 10, 2025
  34. Sjors referenced this in commit 1746618e08 on Feb 13, 2025
  35. ryanofsky referenced this in commit 83e40d3b52 on Feb 24, 2025
  36. ryanofsky referenced this in commit 8619f03ec2 on Feb 24, 2025
  37. ryanofsky referenced this in commit cbb7b41c20 on Feb 24, 2025
  38. fanquake referenced this in commit 01f7715766 on Feb 25, 2025
  39. fanquake referenced this in commit ba0a4391ff on Feb 25, 2025
  40. janus referenced this in commit 86cb86b050 on Sep 1, 2025


purpleKarrot hebasto ryanofsky

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/libmultiprocess. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-12-04 19:30 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me