Closed JohanMabille closed 5 years ago
Hello,
Thank you for your contribution, it could effectively be interesting to add the possibility to install the library.
As I'm not really familiar with this part of CMake it may take me a couple of days to find time before I review and merge the changes.
Concerning the release version, how are changes that don't belong to a release yet managed? Currently we have a 0.6 version, but some changes have been committed to the master since the last release. Are they just part of the 0.7.0 as you have done? Or an alpha version number?
Usually all the changes made after a release belong to the next release. I've set 0.7.0 here but it could be 0.6.1 if there is no backward incompatible change and if you follow semver.
A common practice is to have a dedicated commit for a release where you only change the release number.
Hello,
Sorry for the delay, I had a bit more time to look at it.
Couple of questions:
tsl_ordered_mapConfigVersion.cmake
need to be compatible 32 and 64 bits? It seems weird to temporarily modify CMAKE_SIZEOF_VOID_P
for that.tsl_ordered_mapConfig.cmake.in
, why the if(NOT TARGET @PROJECT_NAME@)
? When could it not be the case?write_basic_package_version_file
wouldn't SameMajorVersion
be better to use than AnyNewerVersion
in case I have to introduce an incompatibility?Personally I would simplify the code to this:
cmake_minimum_required(VERSION 3.1)
project(tsl_ordered_map VERSION 0.7.0)
add_library(tsl_ordered_map INTERFACE)
# Use tsl::ordered_map as target, more consistent with other libraries conventions (Boost, Qt, ...)
add_library(tsl::ordered_map ALIAS tsl_ordered_map)
target_include_directories(tsl_ordered_map INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
target_sources(tsl_ordered_map INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include/tsl/ordered_hash.h
${CMAKE_CURRENT_SOURCE_DIR}/include/tsl/ordered_map.h
${CMAKE_CURRENT_SOURCE_DIR}/include/tsl/ordered_set.h>)
if(MSVC)
target_sources(tsl_ordered_map INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/tsl_ordered_map.natvis>
$<INSTALL_INTERFACE:tsl_ordered_map.natvis>)
endif()
if(${CMAKE_VERSION} VERSION_GREATER "3.7")
# Only available since version 3.8
target_compile_features(tsl_ordered_map INTERFACE cxx_std_11)
endif()
# Installation
include(CMakePackageConfigHelpers)
include(GNUInstallDirs)
## Copy headers and eventual natvis file
install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/include/tsl
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
if(MSVC)
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/tsl_ordered_map.natvis
DESTINATION ".")
endif()
## Create and copy *Config.cmake and *ConfigVersion.cmake
file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake.in"
"\@PACKAGE_INIT\@\n"
"include(\"\${CMAKE_CURRENT_LIST_DIR}/\@PROJECT_NAME\@Targets.cmake\")\n")
configure_package_config_file(${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake.in
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})
write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
COMPATIBILITY SameMajorVersion)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})
## Create and export a target that can be used with find_package(...)
install(TARGETS tsl_ordered_map
EXPORT ${PROJECT_NAME}Targets)
install(EXPORT ${PROJECT_NAME}Targets
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})
Which would be used like:
find_package(tsl_ordered_map)
target_link_libraries(my_target PRIVATE tsl_ordered_map)
Note that to avoid having an extra file in the source tree, I create the *Config.cmake.in
on the fly. Would it be considered bad practice?
The main problem remaining is that I would like tsl::ordered_map
to be used in target_link_libraries
(like Boost, Qt, ...). I know I can use the NAMESPACE
option in install(EXPORT ...)
but I would need to rename tsl_ordered_map
target to ordered_map
. I have to check if I can find a better way.
Hi,
Thanks for your review. Regarding your questions:
tsl_ordered_map
is a pure header library, a single package can be used for both 32 and 64 bits architecture. This allows to reduce the number of different packages you build when packaging for Linux distribution for instance.tsl_ordered_map
as a subproject in another project, you don't want to load the target file because all the settings you need (include directories, compilation options and so on) are already defined in the CMakeLists.txt
and these are the ones you want to use, even if tsl_ordered_map
has been installed as a standalone package elsewhere on your system. SameMajorVersion
is more appropriate here.Regarding tsl_ordered_mapConfig.cmake.in
, I find it more convenient to have it in a dedicated file rather than generating it, this is more consistent with what is done in other projects, where the config.cmake.in file might be more complicated; also I think it's the common way of doing things so this file can be used easily by external projects.
Hello,
Thank you for your explanations.
I changed your branch a bit. Mainly to move the tsl_ordered_mapConfig.cmake.in
into a cmake
directory and also to install the tsl_ordered_map.natvis
file on Windows.
I still have to check if I can export the alias tsl::ordered_map
(to be similar to Boost::*
and Qt::*
) instead of tsl_ordered_map
before merging the changes. I could rename the main target to ordered_map
and then use the NAMESPACE
parameter in install(EXPORT ...)
, but I want to check if there is a better way.
Warn me if I made any error with my changes.
Hi,
Everything looks good to me.
I changed the main target from tsl_ordered_map
to ordered_map
and exported this target inside the tsl::
namespace so that target_link_libraries(my_target PRIVATE tsl::ordered_map)
must be used. It seems there is no other way.
I merged the changes.
Thank you for the contribution.
This PR enables installing
ordered_map
with cmake. This way packages managers can rely on a single method to install it instead of developing their own processes.EDIT: with that change, the
CMakeLists.txt
must be updated for each release so the number version matches the release tag. Another solution is to add aordered_map_version.h
file and retrieve the number from that file in theCMakeLists.txt
. That still requires updating a file for a release, but it might be easier to remember.