Skip to content

Conversation

purpleKarrot
Copy link
Contributor

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.

@DrahtBot
Copy link

DrahtBot commented Sep 3, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@ryanofsky
Copy link
Collaborator

Code review 47deaef.

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.

@hebasto
Copy link
Member

hebasto commented Sep 4, 2025

CI fails:

 make: *** No rule to make target 'tests'.

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.
@purpleKarrot purpleKarrot force-pushed the default-cmake-workflow branch from 47deaef to 1674fe0 Compare September 5, 2025 08:42
@purpleKarrot
Copy link
Contributor Author

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.

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

@ryanofsky
Copy link
Collaborator

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.

@ryanofsky
Copy link
Collaborator

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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants