MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
291 stars 179 forks source link

Add new CMake target to build testing tools #2880

Closed daljit46 closed 4 months ago

daljit46 commented 5 months ago

Useful when you want to run tests for a single command. Currently, tests require the binaries built by testing/tools before they can run. However, if you build a specific target cmake --build build --target tckgen and then run the tests via ctest -R bin_tckgen, it won't work because testing tools are missing. This PR mitigates this by allowing you to run cmake --build build --target tckgen testing_tools and then run ctest -R bin_tckgen.

github-actions[bot] commented 5 months ago

clang-tidy review says "All clean, LGTM! :+1:"

daljit46 commented 5 months ago

@MRtrix3/mrtrix3-devs A question I have is whether we should always build the testing tools (and perhaps C++ unit tests too) if the user has configured the project with MRTRIX_BUILD_TESTS=ON. So for example, consider the following workflow:

cmake -B build -DMRTRIX_BUILD_TESTS=ON
cmake --build build --target tckgen
ctest -R bin_tckgen --test-dir build

This will not work because the testing tools are not built yet. The user must specify cmake --build build --target tckgen testing_tools, but we could allow for this if you think this is more ergonomic (by building the testing tools anytime a command is built).

Lestropie commented 5 months ago

I don't have familiarity with the cmake precedent for building only specific commands. The old build script could do it in its own way.

My initial intuition given other aspects of the cmake transition making various things more explicit would be that binaries shouldn't be getting built that the user didn't instruct to be built. However if going that route, then the naming of environment variable "MRTRIX_BUILD_TESTS" may not be optimal: it's not so much "you will build these testing tools" as "testing tools should be included in the full set of possible build targets".

Hypothetically, it's possible that there could be two separate environment variables: one that would always build testing tools regardless of user specification, and one that would add testing tools to the set of possible build targets but only build them if either within the requested set or if all targets are to be built. However I don't think such complexity is warranted.

daljit46 commented 5 months ago

the naming of environment variable "MRTRIX_BUILD_TESTS" may not be optimal

Yes, I tend to agree. The name was chosen because it's quite common to use "PROJECT_BUILD_TEST*" in CMake projects, but perhaps a better name would be MRTRIX_ENABLE_TESTS.

For now, I will leave the option explicit and add the instructions to build a single command and test it to the documentation.