search

swri-robotics / marti_common

Common utility functions for MARTI
BSD 3-Clause "New" or "Revised" License
54 stars 63 forks source link

WIP: Fix dependencies #704

Closed danthony06 closed 1 year ago

danthony06 commented 1 year ago

swri_roscpp is not exporting its dependencies correctly, as reported in https://github.com/swri-robotics/marti_common/issues/703. This appears to be caused by ament() macros not understanding what an INTERFACE library is. This PR replaces a lot of the ament() calls with direct CMake calls so that dependencies are correctly exported and linked.

danthony06 commented 1 year ago

@rjb0026 and @jonselling can you review this? It's definitely still a WIP at this point.

jonselling commented 1 year ago

I can give it a look / test tomorrow

danthony06 commented 1 year ago

I can give it a look / test tomorrow

Thanks. I just tested it on my system by install swri_roscpp to my system-wide ROS install, and then building a fresh copy of swri_transform_util, which I think should pull in the header file that was previously causing you problems.

danthony06 commented 1 year ago

Good catch, I'll change it.

On Fri, Mar 17, 2023, 9:17 AM Jonathan Selling @.***> wrote:

@.**** commented on this pull request.

In swri_roscpp/CMakeLists.txt https://github.com/swri-robotics/marti_common/pull/704#discussion_r1140295493 :

add_library(${PROJECT_NAME} INTERFACE) +target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_17)

You set the CMAKE_CXX_STANDARD above for 14. Should that be left as is or changed?

— Reply to this email directly, view it on GitHub https://github.com/swri-robotics/marti_common/pull/704#pullrequestreview-1346039271, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBZWHHUTGHFTQBA4YQC2QTW4RW6PANCNFSM6AAAAAAV5MBU6A . You are receiving this because you authored the thread.Message ID: @.***>

jonselling commented 1 year ago

FWIW, it looks like the dependencies get caught now.

However, I don't see the benefit of the Interface Library. I edited the CMakeLists.txt you had to use the correct ament calls, and I removed the whole library interface.

Instead of exporting the library target, I export the include directory. However, this seems to not work when trying to pickup the headers downstream. I looked in my install directory, and the files were actually in the correct spot.

Confused, I looked at the INCLUDE_DIRS set for swri_roscpp in my package that depends on it, and it looks like two swri_roscpp directories are set:

  1. install/swri_roscpp/include/swri_roscpp
  2. install/swri_roscpp/include

However, when I look at the "compile_commands.json" for the files, I see only -isystem <absolute_path>/install/swri_roscpp/include/swri_roscpp directory, so when trying to include anything like <swri_roscpp/swri_roscpp.hpp> it will not get picked up. I believe this is because of the rosidl_generate_interfaces used.

When I remove all of the code in the if (BUILD_TESTING ...) statement in swri_roscpp, this gets added to the compile command instead: -I<path>/install/swri_roscpp/include

danthony06 commented 1 year ago

I happened to look at the ROS2 sensor_msgs package, and I'm thinking this may be a cleaner approach to what I was trying: https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/CMakeLists.txt. It definitely looks like there's some weirdness around interface only libraries and ament, but that approach looks a lot cleaner.

jonselling commented 1 year ago

After doing some digging, I believe that creating the interface library is probably the way to go, mostly because a lot of things in the ament_cmake structure seem to be target based.