cmake: Avoid including CTest if not top level project

[ryanofsky]

Avoid including CTest in if this is not a top-level cmake project because as reported #145 (comment) this adds a BUILD_TESTING option could be confused for Bitcoin core's BUILD_TESTS option.

[ryanofsky]

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.

[hebasto]

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

[ryanofsky]

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?

[hebasto]

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.

[ryanofsky]

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?

[hebasto]

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:

$ cmake -B build -DBUILD_TESTS=OFF -DBUILD_BENCH=ON
$ cmake --build build -j $(nproc)
$ ctest --test-dir build
Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
Test project /home/hebasto/git/bitcoin/build
    Start 1: util_rpcauth_test
1/2 Test #1: util_rpcauth_test ..................   Passed    0.12 sec
    Start 2: bench_sanity_check_high_priority
2/2 Test #2: bench_sanity_check_high_priority ...   Passed    5.73 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   5.85 sec

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

if(BUILD_TESTS)
  enable_testing()
endif()

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.

[ryanofsky]

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 bitcoin/bitcoin#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.