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

installation with cmake #22

Merged
merged 10 commits into from
Oct 10, 2018
Merged

installation with cmake #22

merged 10 commits into from
Oct 10, 2018

Conversation

JohanMabille
Copy link
Contributor

@JohanMabille JohanMabille commented Oct 2, 2018

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 a ordered_map_version.h file and retrieve the number from that file in the CMakeLists.txt. That still requires updating a file for a release, but it might be easier to remember.

@Tessil
Copy link
Owner

Tessil commented Oct 2, 2018

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?

@JohanMabille
Copy link
Contributor Author

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.

@Tessil
Copy link
Owner

Tessil commented Oct 7, 2018

Hello,

Sorry for the delay, I had a bit more time to look at it.

Couple of questions:

  • Why does a tsl_ordered_mapConfigVersion.cmake need to be compatible 32 and 64 bits? It seems weird to temporarily modify CMAKE_SIZEOF_VOID_P for that.
  • In tsl_ordered_mapConfig.cmake.in, why the if(NOT TARGET @PROJECT_NAME@)? When could it not be the case?
  • In 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.

@JohanMabille
Copy link
Contributor Author

Hi,

Thanks for your review. Regarding your questions:

  • since 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.
  • if you include 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.
  • I agree that 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.

@Tessil
Copy link
Owner

Tessil commented Oct 9, 2018

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.

@JohanMabille
Copy link
Contributor Author

Hi,

Everything looks good to me.

…arget inside the namespace 'tsl::'. Users will have to use 'target_link_libraries(my_target PRIVATE tsl::ordered_map)' to use the library with 'find_package(...)'.
@Tessil Tessil merged commit 8a3be75 into Tessil:master Oct 10, 2018
@Tessil
Copy link
Owner

Tessil commented Oct 10, 2018

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.

@JohanMabille JohanMabille mentioned this pull request Oct 12, 2018
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.

None yet

2 participants