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

Extending the rock cmake macro "rock_library" to create and install a -config.cmake file #69

Open
bkocev opened this issue Apr 26, 2021 · 16 comments

Comments

@bkocev
Copy link

bkocev commented Apr 26, 2021

Hi everyone,

the motivation for this feature request is to allow porting existing Rock C++ libraries to ROS.

The initiative started together with the need to port https://github.com/rock-slam/slam-uwv_kalman_filters to a ROS catkin workspace. Despite the facts that slam-uwv_kalman_filters seems to be a plain C++ library and should be possible to port it to a ROS catkin workspace and that @saarnold removed the base types (including base-lib and base-logging) dependencies of this libary, I managed only, after adding a few package.xml files, to build it in a ROS catkin workspace successfully but it is not possible to include headers (e.g., uwv_kalman_filters/PoseUKF.hpp) of this library in a ROS node, for example.

@planthaber pointed out that Rock is using pgkconfig to find packages, while catkin uses cmake itself. In this regard, he suggested that the best option would be to extend the rock cmake macro "rock_library", defined in https://github.com/rock-core/base-cmake/blob/master/modules/Rock.cmake, to create and install a -config.cmake file. On a side note, he said that it might be required to make this optional by using a cmake flag (e.g., set(ROCK_CREATE_CMAKE_CONFIG ON)). Regarding possible conflicts between cmake configs and package configs, @2maz pointed out to just make sure that using CMake and using pkg-config provides the same information / flags.

In my understanding, if we extend the rock cmake macro "rock_library" then we would have a solution for porting any library (initially intended for Rock) to ROS right? I.e., for example "find_package(uwv_kalman_filters REQUIRED)" should work in in any ROS node CMakeLists.txt right? But then, I am confused why it was necessary @saarnold to remove the base types dependencies, i.e., aren't these base types part of another plain C++ library that would be automatically ported to ROS after extending the cmake macro "rock_library"?

Any hints on how to properly extend the rock cmake macro "rock_library" and whether that would be sufficient for porting any initially intended for Rock C++ library (e.g., https://github.com/rock-slam/slam-uwv_kalman_filters) to ROS would be very much welcome and appreciated.

Thank you in advance and kind regards,
Bojan Kocev.

@g-arjones
Copy link
Contributor

g-arjones commented Apr 26, 2021

Unless you are talking about orocos components, I don't see anything that would keep you from using any rock library anywhere (not only within ROS workspaces). Using pkg-config libraries should be possible with catkin:

find_package(PkgConfig REQUIRED)
pkg_check_modules(uwv_kalman_filters uwv_kalman_filters REQUIRED)

catkin_package(
   INCLUDE_DIRS include ${uwv_kalman_filters_INCLUDE_DIRS}
   LIBRARIES ${PROJECT_NAME} ${uwv_kalman_filters_LIBRARIES}
)

target_include_directories(${PROJECT_NAME} PUBLIC ${uwv_kalman_filters_INCLUDE_DIRS})
target_link_libraries(${PROJECT_NAME} PUBLIC ${uwv_kalman_filters_LIBRARIES})

That should be enough in most cases to export the dependency. The one limitation though is that since catkin's cmake macros do not provide an API to export LIBRARY_DIRS, you have to make sure that these libraries are installed somewhere all packages will be able to find.

@bkocev
Copy link
Author

bkocev commented Apr 26, 2021

Thank you very much for the prompt reply.

I included the following in my ROS node CMakeLists.txt:

find_package(PkgConfig REQUIRED)
pkg_check_modules(uwv_kalman_filters uwv_kalman_filters REQUIRED)

catkin_package(
INCLUDE_DIRS include ${uwv_kalman_filters_INCLUDE_DIRS}
LIBRARIES ${PROJECT_NAME}_node ${uwv_kalman_filters_LIBRARIES}
)

target_include_directories(${PROJECT_NAME}_node PUBLIC ${uwv_kalman_filters_INCLUDE_DIRS})

target_link_libraries(${PROJECT_NAME}_node PUBLIC
${catkin_LIBRARIES}
${uwv_kalman_filters_LIBRARIES}
)

But, now I am getting the following linking errors:

/usr/bin/ld: cannot find -luwv_kalman_filters
/usr/bin/ld: cannot find -lpose_estimation
/usr/bin/ld: cannot find -lslom
/usr/bin/ld: cannot find -luwv_dynamic_model
/usr/bin/ld: cannot find -lpose_estimation
/usr/bin/ld: cannot find -lslom
/usr/bin/ld: cannot find -luwv_dynamic_model
collect2: error: ld returned 1 exit status

@g-arjones
Copy link
Contributor

It seems like you are trying to build a node, in which case you would be fine with a link_directories(${uwv_kalman_filters_LIBRARY_DIRS}) but for libraries you would have to include the path where you installed uwv_dynamic_model in your workspace's env file.

Installing it somewhere that's already in your workspace's LD_LIBRARY_PATH should work as well.

@bkocev
Copy link
Author

bkocev commented Apr 27, 2021

That worked out. Thank you very much once again for the very helpful and prompt reply and advice.

Now, it is possible to include "#include <uwv_kalman_filters/PoseUKF.hpp>" in the ROS node cpp file.

Let's see how the rest works tomorrow.

P.S. Based on what you explained, to me it seems that it is not necessary to include Package.xml files if we use the "catkin_package" cmake macro right?

@g-arjones
Copy link
Contributor

g-arjones commented Apr 27, 2021

That file has a different purpose. You should probably look into ROS' documentation for a more in-depth explanation.

@bkocev
Copy link
Author

bkocev commented Apr 27, 2021

Thanks.

@g-arjones
Copy link
Contributor

g-arjones commented Apr 27, 2021

Another (and probably even better) option that would also work to export the dependencies of a library is to use cmake's find_library() on each one of the ${uwv_kalman_filters_LIBRARIES} with ${uwv_kalman_filters_LIBRARY_DIRS} as HINTS to convert the -l arguments to absolute paths to each lib and then use that on both catkin_package() and target_link_libraries() instead of ${uwv_kalman_filters_LIBRARIES}. With that, you wouldn't even need the link_directories() call.

@doudou
Copy link
Member

doudou commented Apr 27, 2021

One thing you could also do is use rock_find_pkgconfig

Importing the rock-config.cmake module does nothing in itself. Since the library uses it, you could import it as well in the node and use rock_find_pkgconfig to resolve the pkg-config libraries (it does the find_library resolution that @g-arjones mentions)

In fine, I have nothing against generating a -config.cmake file. Just that someone else will have to do the work :P

Also, there was no reason to remove base-types I can see.

@g-arjones
Copy link
Contributor

IMHO adding full support to "modern" cmake packages in base/cmake is the bigger issue right now.

I feel like it's only a matter of time until that becomes a real problem for rock users.

@planthaber
Copy link
Member

The idea is indeed to let the rock_library generate and export modern cmake configs. This way both can be used: rock_find_pkgconfig and find_package (up to the developer). For orogen packages, we can keep using pkg-config.
For others we have the option to use cmake.

The issue lies in catkins "devel" builds, where no real install happens. This means the .pc files are not installed to lib/pkgconfig and are not in the pkg-config path, to use them, you have to add each build folder to the PKG_CONFIG_PATH, which is not so nice.

Another solution is to provide libraries with custom CMakeLists.txt files (no rock macros), where a PACKAGE-config.cmake is generated and installed along with a PACKAGE.pc file. And this works fine with both build environments.

e.g.: https://github.com/dfki-ric/robot_remote_control and https://github.com/dfki-ric/slam3d

But it would be nicer when the rock_library macro of rock.cmake would do this directly, so those libs would be directly usable in a catkin/ros context.

@g-arjones
Copy link
Contributor

g-arjones commented Apr 27, 2021

The idea is indeed to let the rock_library generate and export modern cmake configs.

I meant using modern cmake packages, not becoming one.

The issue lies in catkins "devel" builds, where no real install happens

Supporting the "devel" space is not a good use of resources, IMHO. That was a hacky idea that even ROS have abandoned already (see here, here, and here).

Besides, catkin itself will have issues with modern cmake and its own devel space. Example:

add_library(foo INTERFACE IMPORTED)
target_include_directories(foo INTERFACE
  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include/foo>
  $<INSTALL_INTERFACE:include/foo>
)

This won't work in the devel space.

My suggestion if you want to use rock packages (or any non-catkin packages) in ROS workspaces is to not use catkin. Stick to autoproj (my personal choice back in the days when I was using ROS) or go with either ament or colcon.

@bkocev
Copy link
Author

bkocev commented Apr 27, 2021

Hi,

I continued with the tests using the initial solution proposed by @g-arjones, i.e., I will come back to the other suggestions a bit later. I am now getting the following error:

UKFPoseEstimator.cpp:(.text+0x1bb7): undefined reference to `uwv_kalman_filters::PoseUKF::PoseUKF(uwv_kalman_filters::PoseState const&, Eigen::Matrix<double, 53, 53, 0, 53, 53> const&, uwv_kalman_filters::LocationConfiguration const&, uwv_dynamic_model::UWVParameters const&, uwv_kalman_filters::PoseUKF::PoseUKFParameter const&)'

where UKFPoseEstimator.cpp is trying to replicate parts (currently the method PoseEstimator::initializeFilter) of this file https://github.com/rock-slam/slam-orogen-uwv_kalman_filters/blob/master/tasks/PoseEstimator.cpp and is configured to build using:

add_executable(${PROJECT_NAME}_node src/Main.cpp src/UKFPoseEstimator.cpp).

My cmake setup is as in my second comment above only with the difference that instead of:

target_link_libraries(${PROJECT_NAME}_node PUBLIC
${catkin_LIBRARIES}
${uwv_kalman_filters_LIBRARIES}
)

I have:

link_directories(${uwv_kalman_filters_LIBRARY_DIRS})

Any idea what the problem could be?

Thanks,
Bojan.

@bkocev
Copy link
Author

bkocev commented Apr 30, 2021

@g-arjones: Hi again, what do you mean by "The one limitation though is that since catkin's cmake macros do not provide an API to export LIBRARY_DIRS, you have to make sure that these libraries are installed somewhere all packages will be able to find."? For example, under devel/lib, I have, among others the following: libuwv_dynamic_model.so, and libuwv_kalman_filters.so. I think libuwv_kalman_filters.so is not found. Thanks, Bojan.

@bkocev
Copy link
Author

bkocev commented Apr 30, 2021

uwv_kalman_filters_LIBRARIES: uwv_kalman_filters;lapack;f77blas;atlas;pose_estimation;lapack;f77blas;atlas;gdal;slom;uwv_dynamic_model

uwv_kalman_filters_INCLUDE_DIRS: /opt/workspace/devel/include;/usr/include/gdal;/opt/workspace/devel/include;/usr/include/suitesparse;/opt/workspace/dev

There is something wrong with the setup of uwv_kalman_filters_LIBRARIES.

I tried adding:

Autoproj.env_add_path "PKG_CONFIG_PATH", Autoproj.workspace.root_dir() + "/../build/" + pkg.name.gsub("/","-") + "/src/"

in our .autobuild file, but did not help.

@bkocev
Copy link
Author

bkocev commented Apr 30, 2021

I guess catkin is missing what this Rock cmake macro is doing:

macro (rock_find_pkgconfig VARIABLE)
. I guess that is the reason why @planthaber at some point told me to add

Autoproj.env_add_path "PKG_CONFIG_PATH", Autoproj.workspace.root_dir() + "/../build/" + pkg.name.gsub("/","-") + "/src/"

in our .autobuild file

Not sure why it does not work again though. Maybe I need to run some autoproj command before "catkin build"?

@bkocev
Copy link
Author

bkocev commented May 1, 2021

If I declare the libraries explicitly, as follows

target_link_libraries(${PROJECT_NAME}_node PUBLIC
${catkin_LIBRARIES}
${uwv_kalman_filters_LIBRARIES}
${base-types_filters_LIBRARIES}
${uwv_dynamic_model_filters_LIBRARIES}
/opt/workspace/devel/lib/libuwv_kalman_filters.so
/opt/workspace/devel/lib/libbase-types.so
/opt/workspace/devel/lib/libbase-logging.so
)

then the linking errors are gone. So, it seems that the cmake variables such as uwv_kalman_filters_LIBRARIES are not set properly.

I tried copying lines

foreach(${VARIABLE}_lib ${${VARIABLE}_LIBRARIES})
to
include_directories(${${VARIABLE}_INCLUDE_DIRS})
to my ROS node CMakeLists.txt, by replacing ${VARIABLE} with uwv_kalman_filters, but that did not fix the linking errors for uwv_kalman_filters, so I gave up on that idea but maybe I did something wrong in my adaptation of the copied code?

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

No branches or pull requests

4 participants