Make sure that the default workflow of cmake ..; make; make test does not fail with an error message that no tests are found.
Remove any “convenience” targets that add maintenance burden and require tribal knowledge.
Make sure that the default workflow of cmake ..; make; make test does not fail with an error message that no tests are found.
Remove any “convenience” targets that add maintenance burden and require tribal knowledge.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
Code review 47deaef36c2b720cece2a471d23cca4c79dcf617.
Thanks for the PR. These changes seem worth considering, and it does seem pretty indefensible that “make test” doesn’t build the tests it depends on. This behavior seems to be a longstanding bug or quirk of cmake, and it’s especially surprising given how cmake’s “make test” target works differently than its “make install” target when it comes to building dependencies.
I think it could be good to implement the “fixtures” idea in the linked stackoverflow thread to address this, but not good to build targets by default that aren’t needed for installation. I think it’s best for the ‘make; make install` workflow to be fast and reliable and not do unnecessary work.
I’m also not sure about the other part of this change removing convenience targets. make check seems like a good thing to support given cmake’s make test limitations, but maybe it could go away if make test is improved, and maybe the other targets are overkill. The convenience targets are useful for CI as well as development, though, which is why several CI jobs are currently broken with this change.
Make sure that the default workflow of `cmake ..; make; make test`
does not fail with an error message that no tests are found.
Remove any "convenience" targets that add maintenance burden and
require tribal knowledge.
Thanks for the review, @ryanofsky.
longstanding bug or quirk (link to stackoverflow)
Stackoverflow is not a good source of information when it comes to CMake (neither is chatgpt, which seems to be trained on content from stackoverflow as well as projects that followed the advices found there). Too often, the accepted answers are outdated or plain wrong. In this particular case, what you are looking for is CMAKE_SKIP_TEST_ALL_DEPENDENCY.
The convenience targets are useful for CI
This is exactly my concern. The project configuration is intertwined with CI scripts. What I aim for, is that projects and CI scripts are developed independently against a defined interface so that CI scripts can be reused for multiple projects. The defined interface for CMake projects is that tests can be executed after the default target is built.
make test:The test target just invokes ctest without any options. When you execute make test -j20, make will try to spawn up to 20 processes in parallel, but it only has one process to spawn: ctest. And it does not pass any options to that command, so the tests are invoked without parallelization. Is that what you wanted? Maybe. But maybe not.
Developers really should learn what command line options are supported when ctest is invoked directly, like ctest -j20.
so that CI scripts can be reused for multiple projects
I’d be very interested in this. If there’s a generic cmake CI framework that could be plugged in here to simply the CI configuration here that would be great. Absent that though, I think make check is a pretty normal thing for cmake and noncmake projects to provide and a reasonable thing for cmake to call.
The test target just invokes ctest without any options.
Yeah that part makes sense. But it would also make sense if cmake provided a way to control what test target depends on, using something like add_dependency(test ...), or test properties, or something like that. The fact the make test doesn’t behave like make install by default, unless you specify CMAKE_SKIP_TEST_ALL_DEPENDENCY, and the fact that CMAKE_SKIP_TEST_ALL_DEPENDENCY is a blunt instrument that doesn’t add dependencies on the right targets, makes me think make test feature isn’t very well thought out and make check is a better alternative and reasonably standard itself.
I think my main objection to this is I don’t think the “configure, make, make install” should be doing unnecessary work and building things that don’t get installed or used.
I do think the “configure, make, make test” workflow should work, though and I appreciate you pointing out that it is broken. If there’s a way to fix this without using “make check”, which it seems like there might be, I do think we can drop “make check”