cmake: Avoid including CTest if not top level project #161

1. 
   ryanofsky commented at 5:23 am on February 10, 2025: collaborator

   Avoid including CTest in if this is not a top-level cmake project because as reported https://github.com/chaincodelabs/libmultiprocess/pull/145#issuecomment-2644563297 this adds a BUILD_TESTING option could be confused for Bitcoin core’s BUILD_TESTS option.
2. cmake: Avoid including CTest if not top level project

   Reported
   https://github.com/chaincodelabs/libmultiprocess/pull/145#issuecomment-2644563297
   this creates confusion in the bitoin core project because it adds a
   BUILD_TESTING variable which is confusing because bitcoin core uses a different
   BUILD_TESTS variable.

   a7f0669320
3. 
   ryanofsky commented at 5:24 am on February 10, 2025: collaborator

   Marking as a draft for now because this change seems to result in mptest not being found by ctest in Bitcoin core project. I assume some other change is needed there to be compatible with this, but I’m not sure what that would be.
4. 
   hebasto commented at 9:13 am on February 10, 2025: member

   Marking as a draft for now because this change seems to result in mptest not being found by ctest in Bitcoin core project. I assume some other change is needed there to be compatible with this, but I’m not sure what that would be.

   Maybe set(BUILD_TESTING ON) in bitcoin/src/ipc/CMakeLists.txt before calling add_subdirectory(libmultiprocess)?

   This will satisfy the following condition: https://github.com/chaincodelabs/libmultiprocess/blob/26b9f3dda42110a8ffa5e81b0ea78ba1e30ae659/test/CMakeLists.txt#L22

5. 
   ryanofsky commented at 1:31 pm on February 10, 2025: collaborator

   Thanks will experiment, but do you think set(BUILD_TESTING ${BUILD_TESTS}) make sense? Or should BUILD_TESTS not be followed?

   In general I’m pretty confused about this situation. I don’t understand why bitcoin is using a different variable name and not including ctest when both bitcoin and libmultiprocess cmake projects appear to be using ctest. Is bitcoin using ctest in a different way than libmultiprocess is using ctest?

6. 
   hebasto commented at 1:45 pm on February 10, 2025: member

   Thanks will experiment, but do you think set(BUILD_TESTING ${BUILD_TESTS}) make sense?

   Yes, it does. We do the same for the secp256k1 subtree.

   In general I’m pretty confused about this situation. I don’t understand why bitcoin is using a different variable name and not including ctest when both bitcoin and libmultiprocess cmake projects appear to be using ctest. Is bitcoin using ctest in a different way than libmultiprocess is using ctest?

   For Bitcoin Core’s test suite, different executables and commands are used, including test_bitcoin, test_bitcoin-qt, bench_bitcoin and a few of Python scripts. Using CTest’s BUILD_TESTING variable to control the configuration of these targets seems confusing. Additionally, the BUILD_TESTS and BUILD_BENCH variables inherit the corresponding Autotools’ options—namely, --disable-tests and --disable-bench.

7. 
   ryanofsky commented at 1:49 pm on February 10, 2025: collaborator

   Using CTest’s BUILD_TESTING variable to control the configuration of these targets seems confusing.

   This is the part I don’t understand. We are using ctest so why would it be confusing to use the ctest variable and less confusing to have our own similarly named variable?

8. 
   hebasto commented at 2:17 pm on February 10, 2025: member

   Using CTest’s BUILD_TESTING variable to control the configuration of these targets seems confusing.

   This is the part I don’t understand. We are using ctest so why would it be confusing to use the ctest variable and less confusing to have our own similarly named variable?

   Hmm, I was going to respond with the following example:

    0$ cmake -B build -DBUILD_TESTS=OFF -DBUILD_BENCH=ON
    1$ cmake --build build -j $(nproc)
    2$ ctest --test-dir build
    3Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
    4Test project /home/hebasto/git/bitcoin/build
    5    Start 1: util_rpcauth_test
    61/2 Test [#1](/bitcoin-core-multiprocess/1/): util_rpcauth_test ..................   Passed    0.12 sec
    7    Start 2: bench_sanity_check_high_priority
    82/2 Test [#2](/bitcoin-core-multiprocess/2/): bench_sanity_check_high_priority ...   Passed    5.73 sec
    9
   10100% tests passed, 0 tests failed out of 2
   11
   12Total Test time (real) =   5.85 sec
   

   However, due to the following (buggy?) code:

   0if(BUILD_TESTS)
   1  enable_testing()
   2endif()
   

   my example is broken, and the semantics of BUILD_TESTS are indistinguishable from those of BUILD_TESTING.

   UPD. Bitcoin Core’s code should have enable_testing() unconditionally.

9. 
   ryanofsky commented at 2:39 pm on February 10, 2025: collaborator

   I see, so the idea that you want BUILD_TESTS to specifically control whether or not test_bitcoin executable is built, and want BUILD_TESTING as an option for whether to require ctest to simply not exist and always be forced on? That seems reasonable and consistent.

   I will say this is not the approach I would take if I were designing the cmake build. IMO, it makes more sense to have options to control whether dependencies are used, not whether targets are built. So having a BUILD_TESTING option to control whether ctest is used seems useful, and having a BUILD_TEST option to control whether test_bitcoin is built seems less useful. But your approach is consistent and I think it should work.

   I think I will merge this PR, and then in https://github.com/bitcoin/bitcoin/pull/31741 I can decide what to do to support it. I could do set(BUILD_TESTING ON) and add a comment about how bitcoin build always requires ctest so this option is forced on. I could set(BUILD_TESTING ${BUILD_TESTS}) and let BUILD_TESTS control whether unit tests are built in general. Or I could add a separate option to control whether libmultiprocess unit tests are built.

10. ryanofsky merged this on Feb 10, 2025
11. ryanofsky closed this on Feb 10, 2025

    ---

12. Sjors referenced this in commit 6aabfcb615 on Feb 10, 2025
13. Sjors referenced this in commit 1746618e08 on Feb 13, 2025
14. ryanofsky referenced this in commit 83e40d3b52 on Feb 24, 2025
15. ryanofsky referenced this in commit 8619f03ec2 on Feb 24, 2025
16. ryanofsky referenced this in commit cbb7b41c20 on Feb 24, 2025
17. fanquake referenced this in commit 01f7715766 on Feb 25, 2025
18. fanquake referenced this in commit ba0a4391ff on Feb 25, 2025
19. janus referenced this in commit 86cb86b050 on Sep 1, 2025

Contributors
ryanofsky hebasto