-
Notifications
You must be signed in to change notification settings - Fork 47
cmake: Add standalone build support for building and running unit tests #99
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
Conversation
Flamefire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a quick look and pointed out enhancement from a the CMake perspective alone.
I'd recommend waiting for the maintainer to decide if this use case should be supported. The current file is auto-generated
| message(STATUS "Using cmake version ${CMAKE_VERSION}") | ||
|
|
||
| project(boost_sort VERSION "${BOOST_SUPERPROJECT_VERSION}" LANGUAGES CXX) | ||
| cmake_minimum_required(VERSION 3.10...3.16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrote
it's find_package(Boost) that seems problematic prior to cmake-3.10.
In which way? What is the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ PATH=/opt/cmake-3.9.0-Linux-x86_64/bin:$PATH cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DBUILD_TESTING=Y -DCMAKE_CXX_COMPILER=clang++-18 && ninja check
-- Using cmake version 3.9.0
-- The CXX compiler identification is Clang 18.1.3
...
-- Found Boost 1.89.0 at /opt/boost-1.89.0/lib/cmake/Boost-1.89.0
-- Requested configuration: QUIET REQUIRED COMPONENTS core;range;included_test_exec_monitor
...
-- Found boost_numeric_conversion 1.89.0 at /opt/boost-1.89.0/lib/cmake/boost_numeric_conversion-1.89.0
Boost 1.85 found.
Found Boost components:
core;range;included_test_exec_monitor
CMake Error in /opt/cmake-3.9.0-Linux-x86_64/share/cmake-3.9/Modules/FindBoost.cmake:
cmake_policy PUSH without matching POP
Call Stack (most recent call first):
CMakeLists.txt:22 (find_package)
-- Configuring incomplete, errors occurred!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, a bug introduced in CMake 3.9 and fixed in 3.9.1
CMakeLists.txt
Outdated
|
|
||
| if(NOT BOOST_SUPERPROJECT_VERSION) | ||
| if(BUILD_TESTING) | ||
| set(Boost_DEBUG ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for users to set, not projects and produces log spam for everyone
| set(Boost_DEBUG ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to prefer this much information about boost.
cmake doesn't always do what's expected in terms of the version it found, or honoring BOOST_ROOT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you prefer this information. So you can use -DBoost_DEBUG=ON in the invocation of CMake
With this you are forcing this on every user. And you/one usually only want this when something fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than that, I recommend it here in this mode. These tests pull in a surprisingly large subset of boost, which I think that is relevant information in this context.
Besides that find_package(Boost) is hit and miss in practice (maybe more-so for older boost and older cmake), it will log that it succeeded but doesn't actually make clear what version it's using, which can be an ODR concern. In that sense failure is not always obvious via cmake failure.
Perhaps log spam to you, but I'd very much prefer to see it in a bug report, than not.
Boost is big, let's not pretend it doesn't have a bunch of moving parts.
-- Found boost_numeric_conversion 1.89.0 at /opt/boost-1.89.0/lib/cmake/boost_numeric_conversion-1.89.0
-- Boost 1.85 found.
(Yeah thanks cmake for the misleading logging!)
CMakeLists.txt
Outdated
| # Testing | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally dislike such comments. if(BUILD_TESTING already provides this information so I see no added value.
The CMakeLists.txt currently supports the "superproject" boost build,
but does not work stand alone.
This change adds support for a stand-alone build of boost::sort, using the cmake method of detecting an existing boost installation.
-DBUILD_TESTING=Ycan be used for opting into the test coverage.The default
alltarget will not build anything, since sort is a header-only library.The
teststarget can be used to build the unit tests.The
checktarget can be used to run the unit tests, building if necessary.