Modern CMake adjustements

[XB32Z]

trafficstars

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

[mikepurvis]

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:

https://github.com/orocos/orocos_kinematics_dynamics/blob/5b37d7e62e1203de7a39cec333cdbe1ce19225e0/orocos_kdl/orocos_kdl-config.cmake.in#L8

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

[meyerj]

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:

https://github.com/orocos/orocos_kinematics_dynamics/blob/5b37d7e62e1203de7a39cec333cdbe1ce19225e0/orocos_kdl/orocos_kdl-config.cmake.in#L8

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 https://github.com/orocos/orocos_kinematics_dynamics/commit/beee9e5d4804ee57ace3a3c2cf565938347f3f52, which I slightly modified later in https://github.com/orocos/orocos_kinematics_dynamics/commit/228754b67123f9568cb21998ec1bebaae7e46ddb#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]

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. :joy:

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]

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]

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]

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: https://github.com/orocos/orocos_kinematics_dynamics/blob/5b37d7e62e1203de7a39cec333cdbe1ce19225e0/orocos_kdl/orocos_kdl-config.cmake.in#L8

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]

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):

https://github.com/orocos/orocos_kinematics_dynamics/blob/73721147ef7339df11a6a587f15db73410168873/orocos_kdl/CMakeLists.txt#L114-L128

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.

[XB32Z]

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.