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.

Sign up for GitHub

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

Jump to bottom

Modern CMake adjustements #91

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Modern CMake adjustements #91

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6ac6c09
Update CMake design to export include directories
XB32Z Feb 9, 2017
4ec06e3
Update CMakeLits.txt
XB32Z Feb 9, 2017
845385d
Update CMakeLists.txt
XB32Z Feb 9, 2017
534605e
Update CMakeLists.txt
XB32Z Feb 9, 2017
9c44336
Update CMakeLists.txt
XB32Z Jun 30, 2017
54fd789
Update CMakeLists.txt
XB32Z Jun 30, 2017
3083a5b
Merge branch 'master' into master
MatthijsBurgh Sep 26, 2020
90e9f55
Fix merge
MatthijsBurgh Sep 30, 2020
135ee06
Add checks on eigen targets and use built-in cmake script for configu…
XB32Z Feb 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions orocos_kdl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#
# Test CMake version
#
CMAKE_MINIMUM_REQUIRED(VERSION 2.6)
CMAKE_MINIMUM_REQUIRED(VERSION 3.2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#MARK_AS_ADVANCED( FORCE CMAKE_BACKWARDS_COMPATIBILITY )


Expand Down Expand Up @@ -47,7 +47,7 @@ if(NOT Eigen_FOUND)
include(${PROJ_SOURCE_DIR}/config/FindEigen3.cmake)
set(Eigen_INCLUDE_DIR "${EIGEN3_INCLUDE_DIR}")
endif()
include_directories(${Eigen_INCLUDE_DIR})

SET(KDL_CFLAGS "${KDL_CFLAGS} -I${Eigen_INCLUDE_DIR}")

# Check the platform STL containers capabilities
Expand Down Expand Up @@ -97,12 +97,6 @@ ADD_SUBDIRECTORY( tests )
ADD_SUBDIRECTORY( models )
ADD_SUBDIRECTORY( examples )


export(TARGETS orocos-kdl
FILE "${PROJECT_BINARY_DIR}/OrocosKDLTargets.cmake")

export(PACKAGE orocos_kdl)

set(KDL_INCLUDE_DIRS ${KDL_INCLUDE_DIRS} ${Eigen_INCLUDE_DIR})

CONFIGURE_FILE(KDLConfig.cmake.in
Expand Down
5 changes: 3 additions & 2 deletions orocos_kdl/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,14 +126,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}> )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TARGET_LINK_LIBRARIES(orocos-kdl PUBLIC ${Boost_LIBRARIES} Eigen3::Eigen)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@PeterBowman PeterBowman Feb 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@PeterBowman PeterBowman Feb 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


INSTALL(TARGETS orocos-kdl
EXPORT OrocosKDLTargets
ARCHIVE DESTINATION lib${LIB_SUFFIX}
LIBRARY DESTINATION lib${LIB_SUFFIX}
PUBLIC_HEADER DESTINATION include/kdl
INCLUDES DESTINATION include
)

INSTALL(FILES ${UTIL_HPPS} DESTINATION include/kdl/utilities)
Expand Down