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

add support for COMPONENTS in ament_export_dependencies #456

Open
VRichardJP opened this issue May 30, 2023 · 3 comments
Open

add support for COMPONENTS in ament_export_dependencies #456

VRichardJP opened this issue May 30, 2023 · 3 comments
Labels

Comments

@VRichardJP
Copy link
Contributor

VRichardJP commented May 30, 2023

As a follow up of old closed issues:

Problem

Let's consider the following example:

  • Package A creates libA, which depends on PCL common and io components, and on Boost thread
  • Package B creates binB, which depends on libA

On Package A, we would have something like:

  • package.xml
<depend>libpcl-common</depend>
<depend>libpcl-io</depend>
<depend>libboost-thread</depend>
  • CMakeLists.txt
find_package(PCL REQUIRED COMPONENTS common io)
find_package(Boost REQUIRED COMPONENTS thread)

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:
<depend>packageA</depend>
  • CMakeLists.txt:
find_package(packageA REQUIRED)

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:
<depend>packageA</depend>
<depend>libpcl-common</depend>
<depend>libpcl-io</depend>
<depend>libboost-thread</depend>
  • CMakeLists.txt:
find_package(packageA REQUIRED)
find_package(PCL REQUIRED COMPONENTS common io)
find_package(Boost REQUIRED COMPONENTS thread)

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:

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
Copy link
Contributor

Ryanf55 commented Jun 14, 2023

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
Copy link
Contributor Author

VRichardJP commented Jun 15, 2023

@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
Copy link

GDTR12 commented Mar 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants