ros / console_bridge

A ROS-independent package for logging that seamlessly pipes into rosconsole/rosout for ROS-dependent packages.
BSD 3-Clause "New" or "Revised" License
22 stars 62 forks source link

cmake config: console_bridge_LIBRARIES should contain absolute paths. #52

Closed saladpanda closed 5 years ago

saladpanda commented 6 years ago

https://github.com/ros/console_bridge/blob/4a52e5dfe6b8301bc18e472ad585ea216f3762f7/console_bridge-config.cmake.in#L11

I'm trying to cross-compile console_bridge with catkin_make_isolated. Compiling it works fine, but when other packages try to link against console_bridge they can't find the library as console_bridge_LIBRARIES is only set to console_bridge. e.g. in rosbag_storage: https://github.com/ros/ros_comm/blob/kinetic-devel/tools/rosbag_storage/CMakeLists.txt#L41

I'm no cmake expert (so correct me if I'm wrong), but according to the documentation this variable should contain absolute paths to the library:

https://cmake.org/cmake/help/v3.10/command/link_directories.html:

Library locations returned by find_package() and find_library() are absolute paths. Pass these absolute library file paths directly to the target_link_libraries() command.

sloretz commented 6 years ago

Would you mind posting the text of the error you receive?

It looks like you're correct that console_bridge_LIBRARIES is supposed to include full paths (doc on find module standard variables). CMake is moving away from variables in favor of imported targets. The name of the library console_bridge appears to be the imported target name so I would expect target_link_libraries(rosbag_storage ... console_bridge) to work fine.

saladpanda commented 6 years ago

I don't get an error, but I have multiple versions of console_bridge available and target_link_libraries(rosbag_storage ... console_bridge) is picking the wrong one although find_package(console_bridge 0.4 REQUIRED) finds the right one.

sloretz commented 6 years ago

rosbag_storage has find_package(console_bridge REQUIRED) at the top. Does find_package(console_bridge REQUIRED) find the wrong version? What version does it find?

saladpanda commented 6 years ago

Oh yes, sure. I patched that line to include the version number 0.4. Otherwise it finds the system version, which is the one from the debian jessie repositories (0.2.* something). The version it is supposed to find is my self-compiled 0.4 version.

The setup is rather complicated and not fully working at the moment, thats why I left out most of the background story. I already have this issue fixed locally for me (ugly hack, so nothing worth a pull request).

sloretz commented 6 years ago

Understood, thanks for reporting the issue. If later you're willing, a pull request to fix the content of console_bridge_LIBRARIES would be appreciated.

saladpanda commented 6 years ago

My solution to this looks like this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d51e954..514c63e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -81,7 +81,7 @@ set(PKG_LIBRARIES ${PROJECT_NAME})
 set(cmake_conf_file "${PROJECT_NAME}-config.cmake")
 configure_package_config_file("${cmake_conf_file}.in" "${CMAKE_BINARY_DIR}/${cmake_conf_file}"
                               INSTALL_DESTINATION ${CMAKE_CONFIG_INSTALL_DIR}
-                              PATH_VARS CMAKE_INSTALL_FULL_INCLUDEDIR
+                              PATH_VARS CMAKE_INSTALL_FULL_INCLUDEDIR CMAKE_INSTALL_FULL_LIBDIR
                               NO_SET_AND_CHECK_MACRO
                               NO_CHECK_REQUIRED_COMPONENTS_MACRO)
 set(cmake_conf_version_file "${PROJECT_NAME}-config-version.cmake")
diff --git a/console_bridge-config.cmake.in b/console_bridge-config.cmake.in
index dfaa547..e15b9ee 100644
--- a/console_bridge-config.cmake.in
+++ b/console_bridge-config.cmake.in
@@ -6,6 +6,7 @@ set(@PKG_NAME@_CONFIG_INCLUDED TRUE)
 @PACKAGE_INIT@

 set(@PKG_NAME@_INCLUDE_DIRS "@PACKAGE_CMAKE_INSTALL_FULL_INCLUDEDIR@")
+set(@PKG_NAME@_LIBRARY_DIRS "@PACKAGE_CMAKE_INSTALL_FULL_LIBDIR@")

 include("${CMAKE_CURRENT_LIST_DIR}/@PKG_NAME@-targets.cmake")
-set(@PKG_NAME@_LIBRARIES @PKG_NAME@)
+set(@PKG_NAME@_LIBRARIES "@PACKAGE_CMAKE_INSTALL_FULL_LIBDIR@/lib@PKG_NAME@.so")

I'm pretty sure this is not a nice solution, but I don't know how to do this properly in cmake.

scpeters commented 5 years ago

Even though console_bridge does export a target, urdfdom is not using that yet; it's still using console_bridge_LIBRARIES, so this issue can crop up far downstream, as in https://github.com/dartsim/dart/issues/1200

scpeters commented 5 years ago

I believe this was resolved by #60