CTest: Module must be included at the top level

[purpleKarrot]

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.

[hebasto]

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.

[purpleKarrot]

The minimum replacement would be:

option(BUILD_TESTING "Build the testing tree." ON)

include(CTestUseLaunchers)

if(BUILD_TESTING)
  enable_testing()
endif()

The CTestUseLaunchers module should not be omitted.

[hebasto]

The CTestUseLaunchers module should not be omitted.

Why?

[purpleKarrot]

Because otherwise cmake produces an error when CTEST_USE_LAUNCHERS is on.

[ryanofsky]

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.

[purpleKarrot]

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

mkdir build
cd build
cmake ..
make -j 10
ctest -j 10

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

make -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.

[ryanofsky]

Thanks!

The previous version of this PR 9d841a4 looks much more straightforward than current version 5f9162f, and doesn't add a new build option, so I would really prefer it.

@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

[hebasto]

@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.

[hebasto]

What scenarios would this be useful for in this project?

[purpleKarrot]

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.

[ryanofsky]

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.

[purpleKarrot]

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:

CMake Error: CTEST_USE_LAUNCHERS is enabled, but the RULE_LAUNCH_COMPILE global property is not defined.
Did 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.

[ryanofsky]

Code review ACK e7329ae.

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.

[hebasto]

ACK 63ac092, tested on Ubuntu 24.04:

$ cmake -B build
$ cmake --build build -t mptests
$ ctest --test-dir build

[hebasto]

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

[ryanofsky]

re: #145 (comment)

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.