Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test libraries and executables should be declared EXCLUDE_FROM_ALL #229

Closed
pdimov opened this issue May 14, 2024 · 6 comments · Fixed by #230
Closed

Test libraries and executables should be declared EXCLUDE_FROM_ALL #229

pdimov opened this issue May 14, 2024 · 6 comments · Fixed by #230

Comments

@pdimov
Copy link
Member

pdimov commented May 14, 2024

The Boost convention is that cmake --build . only builds the libraries. Test libraries and executables are built with cmake --build . --target tests.

@Flamefire
Copy link
Collaborator

Isn't this the case already through use of boost_test/boost_test_jamfile?

Or did I miss any?

boost_test_jamfile(FILE Jamfile.v2)
# Those require to be run in the test directory
foreach(name test_formatting test_message)
boost_test(SOURCES ${name}.cpp WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
endforeach()

@pdimov
Copy link
Member Author

pdimov commented May 14, 2024

Oh, that's examples, not tests.

[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_boundary.dir/boundary.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_boundary
[ 43%] Built target boost_locale-expl_boundary
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_calendar.dir/calendar.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_calendar
[ 43%] Built target boost_locale-expl_calendar
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_collate.dir/collate.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_collate
[ 43%] Built target boost_locale-expl_collate
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_conversions.dir/conversions.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_conversions
[ 43%] Built target boost_locale-expl_conversions
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_hello.dir/hello.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_hello
[ 43%] Built target boost_locale-expl_hello
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_wboundary.dir/wboundary.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_wboundary
[ 43%] Built target boost_locale-expl_wboundary
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_wconversions.dir/wconversions.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_wconversions
[ 43%] Built target boost_locale-expl_wconversions
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_whello.dir/whello.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_whello
[ 43%] Built target boost_locale-expl_whello
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_perf_collate.dir/performance/perf_collate.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_perf_collate
[ 43%] Built target boost_locale-expl_perf_collate
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_perf_convert.dir/performance/perf_convert.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_perf_convert
[ 43%] Built target boost_locale-expl_perf_convert
[ 43%] Building CXX object libs/locale/examples/CMakeFiles/boost_locale-expl_perf_format.dir/performance/perf_format.cpp.o
[ 43%] Linking CXX executable ../../../stage/bin/boost_locale-expl_perf_format
[ 43%] Built target boost_locale-expl_perf_format

Not sure what we're supposed to do with these. I usually treat them as compile tests.

@Flamefire
Copy link
Collaborator

In the context of what we have right now it makes sense then I guess
I'm still wondering WHY we do that.

The usual workflow for pretty much any software I built so far (and that's a lot given I'm co-maintaining an HPC cluster) is: configure && make && make test && make install (or the equivalent on other buildsystems, e.g. cmake instead of configure)

With EXCLUDE_FROM_ALL we break this "convention" by having make test (or ctest .) be a silent no-op unless make tests is executed before.

I try to go for the least element of surprise and deviating from semi-standard behavior, especially if it leads to silent unexpected results, is surprising.

So what was the motivation? IMO building ALL with make all serves as a smoke test if the library works on the current system/compiler/..., i.e. there are no unmet expectations on that system from the library dev.
And when adding/changing something myself I use the ALL target to quickly see if I missed anything obvious. I'll now need to remember to use the tests target for that.
Finally having make tests and make test might be confusing

@pdimov
Copy link
Member Author

pdimov commented May 15, 2024

No, ctest is not a silent no-op, all the tests that haven't been built fail, because the executables aren't there.

IMO building ALL with make all serves as a smoke test if the library works on the current system/compiler/...

But that's not what it does at all. It builds all of Boost, not "the library".

@Flamefire
Copy link
Collaborator

No, ctest is not a silent no-op, all the tests that haven't been built fail, because the executables aren't there.

You are right. I confused it with the case that no tests were added at all in which case ctest will succeed without having run anything. There is the flag --no-tests=error to detect that case, which can happen if ENABLE_TESTING was accidentally disable somewhere.

@pdimov
Copy link
Member Author

pdimov commented May 15, 2024

--output-on-failure and --no-tests=error should have been the default, really. I have a check target that runs ctest with these, but the problem with it is that there's no good way to pass the correct -j to it.

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 a pull request may close this issue.

2 participants