search

ament / ament_cmake

Supporting CMake packages for working with ament
Apache License 2.0
99 stars 124 forks source link

add support for COMPONENTS in `ament_export_dependencies` #456

Open VRichardJP opened 1 year ago

VRichardJP commented 1 year ago

As a follow up of old closed issues:

Problem

Let's consider the following example:

On Package A, we would have something like:

add_library(libA

...

)

include/link found PCL and Boost components

ament_target_dependencies(libA PCL Boost)

ament_export_targets()

install(TARGETS libA...)

...

export components for downstream?

ament_export_dependencies(PCL Boost)

ament_package()


On package B, we would have:
- package.xml:
```package.xml
<depend>packageA</depend>

add_executable(binB

...

)

link libA target to binB

ament_target_dependencies(binB packageA)

ament_package()


As reported in other issues, this does not work. Essentially, what `ament_export_dependencies(PCL Boost)` does is to add a `find_package(PCL)` and `find_package(Boost)` in downstream packages. `find_package(PCL)` includes all PCL components, so "it will work", even though it will also bloat downstream packages with useless PCL components. On the other hand, `find_package(Boost)` does not include the `thread` component, and the downstream package will be broken.

The alternative is not to use `ament_export_dependencies()` in packageA and instead fetch PCL and Boost components manually downstream, in such case package B would look like this:
- package.xml:
```xml
<depend>packageA</depend>
<depend>libpcl-common</depend>
<depend>libpcl-io</depend>
<depend>libboost-thread</depend>

add_executable(binB

...

)

link libA target to binB

ament_target_dependencies(binB packageA PCL Boost )

ament_package()


Although this works and does exactly what is required, it makes no sense for downstream packages to have to fetch upstream dependency themselves.

# Solution

The solution is to let the user export components. Previous issues have suggested a new `ament_export_dependency_with_components()` macro, but I think the functionality can be added to the existing `ament_export_dependencies()` macro.

For example:
```cmake
ament_export_dependencies(PCL COMPONENTS common io)
ament_export_dependencies(Boost COMPONENTS thread)

Example implementation

Current implementation accepts syntax ament_export_dependencies(depA depB depC...). The macro call appends all these dependencies to _AMENT_CMAKE_EXPORT_DEPENDENCIES (e.g. "prevDepX;depA;depB;depC"). Later, when the package B calls find_package(PackageA), find_package() is called on each listed dependency.

There could be a new syntax ament_export_dependencies(depA COMPONENTS compA compB...). When COMPONENTS keyword is used, the dependency components would be packed and appended to _AMENT_CMAKE_EXPORT_DEPENDENCIES using a special separator character (e.g. :). For example: prevDepX;depA:compA:compB. Then, when someone calls find_package(PackageA), the list of components would be unpacked into a find_package(depA QUIET REQUIRED COMPONENTS compA compB).

Note that when the COMPONENTS keyword is used, only 1 dependency would be allowed per call. The 2 syntaxes would remain valid:

# legacy export
ament_export_dependencies(depA depB depC)
# new component export
ament_export_dependencies(PCL COMPONENTS common io)
ament_export_dependencies(Boost COMPONENTS thread)
Ryanf55 commented 1 year ago

Could we try something like this using the taret property LINK_LIBRARIES? It worked for me in a large package, but we didn't use components.

I have a massive library with 40+ dependencies, some are brought in with generator expressions depending on what the operating system compilation options specify. I can't blindly export all dependencies that are possible with ament_export_dependencies, and it's not feasible for me to manually maintain the exact ordering of them due to the complex chaining. 

find_package(rclcpp REQUIRED)
find_package(my_private_algs REQUIRED)
 # And other deps

add_library(${PROJECT_NAME} ...)
target_link_libraries(${PROJECT_NAME} PUBLIC rclcpp::rclcpp PRIVATE my_private_algs::my_private_alg1 my_private_algs::my_private_alg2)

install(
   TARGETS ${CMAKE_PROJECT_NAME}
   EXPORT export_${CMAKE_PROJECT_NAME}
)

ament_export_targets(export_${CMAKE_PROJECT_NAME} HAS_LIBRARY_TARGET)

get_target_property(${CMAKE_PROJECT_NAME}_link_libs ${CMAKE_PROJECT_NAME} LINK_LIBRARIES)
ament_export_dependencies(
   ${CMAKE_PROJECT_NAME}_link_libs
)

Have you considered how you handle dependencies relying on each other? IE: my_custom_pcl_algs target depends on certain PCL components. In the documentation, ament talks about how the order you pass these in to ament_export_dependencies is signficant.
ament_export_dependencies(my_custom_pcl_algs COMPENTS alg1) ament_export_dependencies(PCL COMPONENTS common io) ament_export_dependencies(Boost COMPONENTS thread)

VRichardJP commented 1 year ago

@Ryanf55 correct me if I am wrong, but I am not sure the LINK_LIBRARIES trick works. For instance, if your alg1 links on Boost::thread, you may have to find both libboost_thread.so and <boost/thread.hpp> in downstream package. There are maybe even some compile flags you would have to pass downstream. So only passing dependencies libraries may not be sufficient. Then, if you start to push libraries, include directories and flags downstream, you will blow up cmake processing time in downstream packages (e.g. because of duplicated items).

In the documentation, ament talks about how the order you pass these in to ament_export_dependencies is signficant.

I couldn't find this information in the documentation. Where did you find it? Since ament_export_dependencies(XXX) is just a shortcut to get a find_package(XXX) in downstream package, the whole question is whether find_package order matters. This is more of a findXXX.cmake issue.

In particular, it is normally not a problem to find the same package multiple times but with different components. For example, these two should behave the same:

# multiple 
find_package(PCL REQUIRED COMPONENTS common)
find_package(PCL REQUIRED COMPONETNS io)

# single
find_package(PCL REQUIRED COMPONENTS common io)

So there should be no problem if several dependencies are using different components of the same library. If there is an issue, it is up to the upstream library to fix it, not ament.

GDTR12 commented 6 months ago

I have meet the same issue, and I have read many sites to handle the issue, the funniest thing is that the ealiest issue I've found is proposed in 2019. in this years, developments could only handle this situation were used the hardest way: create a file.Please accept this PR as fast as you can.