-
Notifications
You must be signed in to change notification settings - Fork 59
Feat: modernize CMake files #35
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
base: develop
Are you sure you want to change the base?
Feat: modernize CMake files #35
Conversation
| // FIXME(CK): #include <boost/type_index.hpp> | ||
| import boost.type_index; |
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.
@codeowners any reason to not use import?
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.
#include <boost/type_index.hpp> automatically preprocesses to import boost.type_index; if the module is available
|
|
||
| cmake_minimum_required( VERSION 3.5...3.31 ) | ||
| project( boost_any VERSION "${BOOST_SUPERPROJECT_VERSION}" LANGUAGES CXX ) | ||
| cmake_minimum_required(VERSION 3.21...4.2) |
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.
@codeowners what is the minimum version that must be supported?
import std; is usable starting with cmake v3.30 AFAIK
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 don't think we want to bump the minimum version so much. These CMake files are invoked from the Boost superproject, so bumping a minimum version in one affects every other library. I think 3.8 is okay.
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.
on every build host cmake v4.2 may be installed with pipx install cmake?
I want to have PROJECT_IS_TOP_LEVEL available with cmake version 3.21 anything older is not state of the art!
|
|
||
| cmake_minimum_required( VERSION 3.5...3.31 ) | ||
| project( boost_any VERSION "${BOOST_SUPERPROJECT_VERSION}" LANGUAGES CXX ) | ||
| cmake_minimum_required(VERSION 3.21...4.2) |
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 don't think we want to bump the minimum version so much. These CMake files are invoked from the Boost superproject, so bumping a minimum version in one affects every other library. I think 3.8 is okay.
CMakeLists.txt
Outdated
| add_executable(module_sample modules/usage_sample.cpp) | ||
| target_link_libraries(module_sample PRIVATE boost_any) | ||
| else() | ||
| set(CMAKE_VERIFY_INTERFACE_HEADER_SETS 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.
CMake docs suggest to not set this directly in code (https://cmake.org/cmake/help/latest/variable/CMAKE_VERIFY_INTERFACE_HEADER_SETS.html)
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 changed it so it is only usable for standalone builds
CMakeLists.txt
Outdated
| endif() | ||
| set(__scope PUBLIC) | ||
|
|
||
| add_executable(module_sample modules/usage_sample.cpp) |
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.
Please don't build examples from top-level cmakes
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.
done
|
Fails with error: |
use CMAKE_VERIFY_INTERFACE_HEADER_SETS if possible
This fix #34 too