Modern CMake adjustements #91

XB32Z · 2017-02-09T13:36:16Z

Hi all,

First of all, thx for your amazing work! I just commit this minor change to your CMake. It's really good but I ran into two issues:
The cmake currently exports the target which means that when find_package(ing) for orocos, cmake looks directly in the build directory. Although it's really convinient to use, the interface is different than the installed interface eg. : in the build the file chain.hpp is in src/chain.hpp and in the install dir it's kdl/chain.hpp so that's really not easy.

Second issue, in the installed exported targets, the include where saved in a config file while they could just be added to the targets themselves, which removes the issue of using include_directories on the user-side. I just added a line INCLUDE DESTINATION include/kdl in the src/CMakeLists.txt

If you are concerned about the dependency with Eigen, know that it is possible to use the modern cmake on Eigen too and use it as target_link_library(orocos-kdl PUBLIC Eigen) which will create a cleaner dependency in the config file.

All the best,
XB32Z

The current cmake implementation provides target modern cmake but those targets does not hold their include. This aims to correct it.

Same problem as ./CMakeLists.txt

Wrong install path

Added minimum version 3.2 to support new technologies

smits · 2017-02-20T12:47:41Z

orocos_kdl/CMakeLists.txt

@@ -1,7 +1,7 @@
 #
 # Test CMake version
 #
-CMAKE_MINIMUM_REQUIRED(VERSION 2.6)
+CMAKE_MINIMUM_REQUIRED(VERSION 3.2)


I'm hesitant to require newer cmake version higher then 2.8 because: "2.8.12 is the version shipped starting from Ubuntu Trusty (April 2014) or Debian Jessie (April 2015). Other Linux distributions released at approximately the same time have typically newer versions. So that's where we would cut off. Ubuntu Precise (cmake 2.8.7) goes EOL in April 2017, Debian Wheezy (cmake 2.8.9) in May 2018, so probably its a good idea to stay compatible with 2.8.9 until at least next year."

CMake 3.5 was backported to Trusty a few months ago: https://packages.ubuntu.com/trusty/cmake3.

Updated to new Eigen exportefd target

PeterBowman · 2018-06-12T19:07:54Z

orocos_kdl/src/CMakeLists.txt

-INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR})
-TARGET_LINK_LIBRARIES(orocos-kdl ${Boost_LIBRARIES})
+TARGET_INCLUDE_DIRECTORIES(orocos-kdl PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}> )
+TARGET_LINK_LIBRARIES(orocos-kdl PUBLIC ${Boost_LIBRARIES} Eigen3::Eigen)


This target was introduced in Eigen 3.3.1, you'll need to add some fallback behavior (if(TARGET Eigen3::Eigen)...else()...endif()).

Sorry for the (very) long absence, is this still relevant?

Since KDL doesn't enforce a minimum version, anyone trying to use Eigen<3.3.1 can experience errors, so I think this is still an issue. However, you could just tweak FindEigen3.cmake so that modern CMake targets are made available for use both by KDL itself and projects consuming KDL (make sure this gives precedence to the local file or just add OR NOT TARGET Eigen3::Eigen in the if clause). See upstream find module, especially the last few lines:

if(EIGEN3_FOUND AND NOT TARGET Eigen3::Eigen) add_library(Eigen3::Eigen INTERFACE IMPORTED) set_target_properties(Eigen3::Eigen PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${EIGEN3_INCLUDE_DIR}") endif()

currently if Eigen is not found there is no error? Adding eigen with the TARGET_LINK_LIBRARIES will print an error when generating.

If Eigen is a required dependency, there should be a check somewhere else (and before those lines) to issue an error. I wouldn't delegate such responsibility to the target_link_libraries call, typically you would use find_package(<pkg> REQUIRED) or a simple if(NOT <pkg>_FOUND)-then-message(FATAL_ERROR ...) condition instead.

mikepurvis · 2020-09-26T02:59:04Z

Hey, I'm running into a similar issue, where the find module generated from orocos_kdl-config.cmake.in ends up trying to look for KDL stuff relative to the directory of the package including it, due to the ${CMAKE_CURRENT_LIST_DIR} here not being resolved at configure time:

orocos_kinematics_dynamics/orocos_kdl/orocos_kdl-config.cmake.in

Line 8 in 5b37d7e

    
           get_filename_component(orocos_kdl_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)

In any case, the oldest Ubuntu LTS at this point is Xenial, and it has CMake 3.5.1. :)

meyerj · 2020-09-26T18:38:42Z

Hey, I'm running into a similar issue, where the find module generated from orocos_kdl-config.cmake.in ends up trying to look for KDL stuff relative to the directory of the package including it, due to the ${CMAKE_CURRENT_LIST_DIR} here not being resolved at configure time:

orocos_kinematics_dynamics/orocos_kdl/orocos_kdl-config.cmake.in

Line 8 in 5b37d7e

get_filename_component(orocos_kdl_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)

Hmm, that's strange. Normally ${CMAKE_CURRENT_LIST_FILE} and ${CMAKE_CURRENT_LIST_DIR} in an installed CMake package config file should refer to where the config file was actually found, and not to the directory of the package including it. This approach is also described in the CMake wiki here, and the OrocosKDLTarget.cmake file generated by CMake itself does something similar:

# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()

The reason why it is not hard-coded at configure-time (of orocos_kdl) is to make the installation relocatable. Actually you proposed that patch originally in beee9e5, which I slightly modified later in 228754b#diff-1dafaf63f004fa12ab4d31e2cc5da717 to set orocos_kdl_PREFIX first.

Are you using colcon or catkin with a symlinked install-space or something like that? I could imagine that my version with ../../.. fails to find the correct prefix if orocos_kdl-config.cmake in the install-space (or one of its parent directories) is a symlink to the file generated in the orocos_kdl build directory.

mikepurvis · 2020-09-28T01:32:10Z

The reason why it is not hard-coded at configure-time (of orocos_kdl) is to make the installation relocatable. Actually you proposed that patch originally in beee9e5, which I slightly modified later in 228754b#diff-1dafaf63f004fa12ab4d31e2cc5da717 to set orocos_kdl_PREFIX first.

Well, how about that. 😂

I am indeed using colcon, though not with the symlink-install space. And yes, I think the problem is indeed with the ../../... The problem currently manifests for me with an internal package, and I haven't been able to get together a shareable MWE, but this is the error— you can see that it's a twice-removed issue where we have our package failing to build, due to tf2_geometry_msgs having embedded in its find module a bad include path for orocos_kdl.

--- stderr: my_fancy_package
CMake Error at /home/administrator/ws/install/tf2_geometry_msgs/share/tf2_geometry_msgs/cmake/tf2_geometry_msgsConfig.cmake:113 (message):
  Project 'tf2_geometry_msgs' specifies
  '/home/administrator/ws/build/orocos_kdl/../../../include' as an include
  dir, which is not found.  It does neither exist as an absolute directory
  nor in
  '${prefix}//home/administrator/ws/build/orocos_kdl/../../../include'.
  Check the website 'http://www.ros.org/wiki/tf2_ros' for information and
  consider reporting the problem.
Call Stack (most recent call first):
  /home/administrator/ws/install/catkin/share/catkin/cmake/catkinConfig.cmake:76 (find_package)
  CMakeLists.txt:6 (find_package)

And of course in /home/administrator/ws/install/tf2_geometry_msgs/share/tf2_geometry_msgs/cmake/tf2_geometry_msgsConfig.cmake we find the bad embedded path:

set(_include_dirs "include;/home/administrator/ws/build/orocos_kdl/../../../include;/usr/include/eigen3")

But when I build that package on its own separately (attempting to produce a MWE), I end up with the reasonable, expected path instead:

set(_include_dirs "include;/home/administrator/ws/install/orocos_kdl/share/orocos_kdl/cmake/../../../include;/usr/include/eigen3")

So yeah, it's not at all clear to me how it ends up referencing the build/orocos_kdl path; each individual package should only ever be aware of one build space— its own. Dependencies should be known only by their install spaces, so it's perfectly possible that this is in fact some kind of colcon-specific cross-talk issue, particularly since this chain of packages builds fine for us with catkin_tools. So yeah, FYI @dirk-thomas as a point of curiosity, but obviously I need to dig further here (and sorry for rezzing a super old ticket with something which is probably going to turn out to be unrelated).

dirk-thomas · 2020-09-28T19:13:11Z

orocos_kdl/src/CMakeLists.txt

@@ -80,14 +80,15 @@ ENDIF()
 #####end RPATH

 # Needed so that the generated config.h can be used
-INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR})
-TARGET_LINK_LIBRARIES(orocos-kdl ${Boost_LIBRARIES})
+TARGET_INCLUDE_DIRECTORIES(orocos-kdl PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}> )


This additionally needs a $<INSTALL_INTERFACE:include> to export an include directory in the install case.

Isn't that what

install(TARGETS ... INCLUDES DESTINATION include )

is supposed to do here?

From the documentation:

INCLUDES DESTINATION
This option specifies a list of directories which will be added to the INTERFACE_INCLUDE_DIRECTORIES target property of the <targets> when exported by the install(EXPORT) command. If a relative path is specified, it is treated as relative to the $<INSTALL_PREFIX>.

So probably both are valid approaches to achieve the same at the end. But having both might be redundant.

dirk-thomas · 2020-09-28T19:27:38Z

Normally ${CMAKE_CURRENT_LIST_FILE} and ${CMAKE_CURRENT_LIST_DIR} in an installed CMake package config file should refer to where the config file was actually found, and not to the directory of the package including it.

I would agree to that.

The problem currently manifests for me with an internal package, and I haven't been able to get together a shareable MWE

What exact version of orocos-kdl are you using? Have you build orocos-kdl and/or tf2_geometry_msgs from source? Can you print orocos_kdl_PREFIX when you run into the problem?

Also feel free to mention me again when you can reproduce it.

mikepurvis · 2020-09-29T21:34:16Z

Okay, so I think the issue here has to do with how tf2_geometry_msgs expresses its dependency on orocos_kdl, using the debianized name:

https://github.com/ros/geometry2/blob/dae26a225e6c14a5aabd25a8b08c3031e6563535/tf2_geometry_msgs/package.xml#L16

If I populate and build a workspace, but switch that dependency over:

rosinstall_generator tf2_geometry_msgs --rosdistro noetic --deps --tar | vcs import
git clone https://github.com/orocos/orocos_kinematics_dynamics
sed -i 's/liborocos-kdl-dev/orocos_kdl/' geometry2/tf2_geometry_msgs/package.xml
colcon build --packages-up-to tf2_geometry_msgs

Then I end up with the correct result:

$ grep _include_dirs install/tf2_geometry_msgs/share/tf2_geometry_msgs/cmake/tf2_geometry_msgsConfig.cmake
  set(_include_dirs "include;/home/administrator/orocos_ws/install/orocos_kdl/include;/usr/include/eigen3")
  foreach(idir ${_include_dirs})

So I'm not sure how to really make this better— tf2_geometry_msgs should probably use rosdep indirection for that dependency, but I can't really imagine how colcon could reasonably detect/warn the user that there's going to be this kind of a crosstalk issue, since it's almost entirely an internal thing to CMake. Either way, at least this kind of thing is easier to patch now in the brave new world of colcon out-of-tree metas.

XB32Z · 2021-02-14T18:48:01Z

Hey, I'm running into a similar issue, where the find module generated from orocos_kdl-config.cmake.in ends up trying to look for KDL stuff relative to the directory of the package including it, due to the ${CMAKE_CURRENT_LIST_DIR} here not being resolved at configure time:

orocos_kinematics_dynamics/orocos_kdl/orocos_kdl-config.cmake.in

Line 8 in 5b37d7e

get_filename_component(orocos_kdl_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)

Hmm, that's strange. Normally ${CMAKE_CURRENT_LIST_FILE} and ${CMAKE_CURRENT_LIST_DIR} in an installed CMake package config file should refer to where the config file was actually found, and not to the directory of the package including it. This approach is also described in the CMake wiki here, and the OrocosKDLTarget.cmake file generated by CMake itself does something similar:
# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()
The reason why it is not hard-coded at configure-time (of orocos_kdl) is to make the installation relocatable. Actually you proposed that patch originally in beee9e5, which I slightly modified later in 228754b#diff-1dafaf63f004fa12ab4d31e2cc5da717 to set orocos_kdl_PREFIX first.

Are you using colcon or catkin with a symlinked install-space or something like that? I could imagine that my version with ../../.. fails to find the correct prefix if orocos_kdl-config.cmake in the install-space (or one of its parent directories) is a symlink to the file generated in the orocos_kdl build directory.

About that, we could actually use the cmake export structure to create orocos_kdl-config.cmake instead of the custom in file. https://cmake.org/cmake/help/git-stage/guide/importing-exporting/index.html#id10 let me know if that would be of interest.

PeterBowman · 2021-02-14T19:38:29Z

About that, we could actually use the cmake export structure to create orocos_kdl-config.cmake instead of the custom in file. https://cmake.org/cmake/help/git-stage/guide/importing-exporting/index.html#id10 let me know if that would be of interest.

I think this is already implemented, see export(TARGETS) and INSTALL(EXPORT):

orocos_kinematics_dynamics/orocos_kdl/CMakeLists.txt

Lines 114 to 128 in 7372114

    
           export(TARGETS orocos-kdl 
        
             FILE "${PROJECT_BINARY_DIR}/OrocosKDLTargets.cmake") 
        
           export(PACKAGE orocos_kdl) 
        
           # Generate CMake package configuration 
        
           CONFIGURE_FILE(orocos_kdl-config.cmake.in 
        
             ${PROJECT_BINARY_DIR}/orocos_kdl-config.cmake @ONLY) 
        
           CONFIGURE_FILE(orocos_kdl-config-version.cmake.in 
        
             ${PROJECT_BINARY_DIR}/orocos_kdl-config-version.cmake @ONLY) 
        
           INSTALL(FILES cmake/FindEigen3.cmake DESTINATION share/orocos_kdl/cmake) 
        
           INSTALL(FILES ${PROJECT_BINARY_DIR}/orocos_kdl-config.cmake DESTINATION share/orocos_kdl/cmake) 
        
           INSTALL(FILES ${PROJECT_BINARY_DIR}/orocos_kdl-config-version.cmake DESTINATION share/orocos_kdl/cmake) 
        
           INSTALL(EXPORT OrocosKDLTargets DESTINATION share/orocos_kdl/cmake)

As far as I can tell, CMake can take care for you of creating the OrocosKDLTargets.cmake file for both build and install trees, but even in the simplest case you still need at least a tiny OrocosKDLConfig.cmake(.in) for including that file in downstream code:

include("${CMAKE_CURRENT_LIST_DIR}/OrocosKDLTargets.cmake")

Since KDL must take care of its dependencies (i.e. Eigen+Boost, here one could use find_dependency by the way) and also defines additional CMake variables evaluated on config time, you do need to provide a custom .in template anyway.

Well, actually the version .in could be replaced by some sort of CMake automation, see CMakePackageConfigHelpers.

…ration Eigen3 is offering CMake target from 3.3.1, we add in the local find module a target mechanism and fall back to it when FindEigen3.cmake did not create a target. Use cmake CMakePackageConfigHelpers to generate the .config file.

XB32Z · 2021-02-20T14:25:26Z

I added the CMakePackageConfigHelpers as suggested to create the config and version files.

I also forced the usage of the local FindEigen3.cmake when the default does not create a target. The local FindEigen3.cmake now also creates a target, as proposed. I also added a check if the target is defined in order to not fail at generate or build time. This was not there before as finding eigen was flagged as QUIET. I hope this does not break anything.

XB32Z added 4 commits February 9, 2017 14:26

Update CMake design to export include directories

6ac6c09

The current cmake implementation provides target modern cmake but those targets does not hold their include. This aims to correct it.

Update CMakeLits.txt

4ec06e3

Same problem as ./CMakeLists.txt

Update CMakeLists.txt

845385d

Wrong install path

Update CMakeLists.txt

534605e

Added minimum version 3.2 to support new technologies

smits reviewed Feb 20, 2017

View reviewed changes

XB32Z added 2 commits June 30, 2017 10:16

Update CMakeLists.txt

9c44336

Update CMakeLists.txt

54fd789

Updated to new Eigen exportefd target

PeterBowman suggested changes Jun 12, 2018

View reviewed changes

meyerj added the requires-cmake3 requires CMake 3.0 or higher label Feb 21, 2020

meyerj force-pushed the master branch from 8af3593 to b35c424 Compare February 22, 2020 02:22

Merge branch 'master' into master

3083a5b

dirk-thomas reviewed Sep 28, 2020

View reviewed changes

Fix merge

90e9f55

XB32Z changed the title ~~Modern CMake Adjustements~~ Modern CMake adjustements Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modern CMake adjustements #91

Modern CMake adjustements #91

XB32Z commented Feb 9, 2017

smits Feb 20, 2017

PeterBowman Jun 12, 2018

PeterBowman Jun 12, 2018

XB32Z Feb 14, 2021

PeterBowman Feb 14, 2021 •

edited

Loading

XB32Z Feb 20, 2021

PeterBowman Feb 20, 2021 •

edited

Loading

mikepurvis commented Sep 26, 2020

meyerj commented Sep 26, 2020

mikepurvis commented Sep 28, 2020 •

edited

Loading

dirk-thomas Sep 28, 2020

meyerj Sep 28, 2020

dirk-thomas commented Sep 28, 2020

mikepurvis commented Sep 29, 2020 •

edited

Loading

XB32Z commented Feb 14, 2021

PeterBowman commented Feb 14, 2021

XB32Z commented Feb 20, 2021

Modern CMake adjustements #91

Are you sure you want to change the base?

Modern CMake adjustements #91

Conversation

XB32Z commented Feb 9, 2017

smits Feb 20, 2017

Choose a reason for hiding this comment

PeterBowman Jun 12, 2018

Choose a reason for hiding this comment

PeterBowman Jun 12, 2018

Choose a reason for hiding this comment

XB32Z Feb 14, 2021

Choose a reason for hiding this comment

PeterBowman Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

XB32Z Feb 20, 2021

Choose a reason for hiding this comment

PeterBowman Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

mikepurvis commented Sep 26, 2020

meyerj commented Sep 26, 2020

mikepurvis commented Sep 28, 2020 • edited Loading

dirk-thomas Sep 28, 2020

Choose a reason for hiding this comment

meyerj Sep 28, 2020

Choose a reason for hiding this comment

dirk-thomas commented Sep 28, 2020

mikepurvis commented Sep 29, 2020 • edited Loading

XB32Z commented Feb 14, 2021

PeterBowman commented Feb 14, 2021

XB32Z commented Feb 20, 2021

PeterBowman Feb 14, 2021 •

edited

Loading

PeterBowman Feb 20, 2021 •

edited

Loading

mikepurvis commented Sep 28, 2020 •

edited

Loading

mikepurvis commented Sep 29, 2020 •

edited

Loading