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

no way to ament_export a package with its components #199

Closed
rotu opened this issue Sep 8, 2019 · 11 comments
Closed

no way to ament_export a package with its components #199

rotu opened this issue Sep 8, 2019 · 11 comments
Labels
enhancement New feature or request

Comments

@rotu
Copy link
Contributor

rotu commented Sep 8, 2019

ament_export_dependencies is supposed to be used to re-export packages for downstream use. Unfortunately when using Boost or other large libraries, components don't tend to get re-exported. It would be nice if possible to have ament_export_dependencies and ament_target_dependencies respect the imported components of a package.

This may eventually be obsolete if we shift more to exporting targets instead of packages as per Discourse discussion "Ament best practice for sharing libraries"

Original answers.ros question with the example:

My build fails with:

/usr/bin/ld: CMakeFiles/slam_karto.dir/src/slam_karto.cpp.o: undefined reference to symbol '_ZN5boost6detail23get_current_thread_dataEv'
//usr/lib/x86_64-linux-gnu/libboost_thread.so.1.65.1: error adding symbols: DSO missing from command line
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [slam_karto] Error 1
make[1]: *** [CMakeFiles/slam_karto.dir/all] Error 2
make: *** [all] Error 2
---
Failed   <<< slam_karto [ Exited with code 2 ]

From my library CMake:

cmake_minimum_required(VERSION 3.5)
project(open_karto)
find_package(Boost REQUIRED COMPONENTS thread)
include_directories(include)
add_library(karto SHARED src/Karto.cpp src/Mapper.cpp)
ament_target_dependencies(karto Boost)
ament_export_include_directories(include)
ament_export_libraries(karto)
ament_export_dependencies(Boost)

From the consuming package CMakeLists.txt:

ament_target_dependencies(slam_karto
  open_karto
  )

I can make the error go away by changing the consuming package to have the following, but this is very bad practice:

find_package(Boost REQUIRED COMPONENTS thread)
ament_target_dependencies(slam_karto
  open_karto
  Boost
  )
@dirk-thomas dirk-thomas added enhancement New feature or request more-information-needed Further information is required labels Sep 9, 2019
@dirk-thomas
Copy link
Contributor

Afaik there is no way in CMake to determine which components of a package were found. If you happen to know a way to retrieve the information thread after the invocation of find_package(Boost REQUIRED COMPONENTS thread) in a generic fashion I would be eager to here about it.

Otherwise a new function like ament_export_dependency_with_components (doesn't have to be the actual name) could be implemented which can only export one dependency per invocation.

Exporting targets doesn't make this case go away - except if absolute paths of dependencies are exported at build time of a package which is undesired since it prevent relocating the dependencies.

@rotu
Copy link
Contributor Author

rotu commented Sep 9, 2019

EDIT: these might not be exposed after find_package returns
The documented way to list components searched for is:

<PackageName>_FIND_COMPONENTS
list of requested components

<PackageName>_FIND_REQUIRED_<c>
true if component <c> is required, false if component <c> is optional.

https://cmake.org/cmake/help/latest/command/find_package.html

I haven’t tried patching this yet, but I suspect it works as documented with Boost.

Exporting targets doesn't make this case go away - except if absolute paths of dependencies are exported at build time of a package which is undesired ...

Why not? These should already be installed via CMake and thus discoverable normally? Or is there some reason dependencies are not in the include and libraries search paths when the requiring packages are built?

@dirk-thomas
Copy link
Contributor

dirk-thomas commented Sep 9, 2019

Why not?

Because the underlying dependency might have been relocated (Boost is a bad example for that). ament_export_dependencies only "exports" the names and finds the again when the package is being used - so it is flexible if the dependency has been relocated as long as it can find it.

ament_export_interfaces on the other hand - at least atm (see #173) - exports absolute information for non-target dependencies and therefore fails when those are being relocated.

@rotu
Copy link
Contributor Author

rotu commented Sep 9, 2019

Got it. So ament_export_dependencies exports packages for find_package and does not re-export its COMPONENTS (how could it? it doesn't know about them?), and the solution will require adding more arguments or extending the interface of ament_export_dependencies to re-declare components.

I guess find_package was not designed for recursive resolution, or else it would have a PUBLIC keyword like target_link_libraries does. Hence the need to add ament_export_interfaces and ament_target_dependencies. This also explains #189. When using ament_target_dependencies you are specifying a dependency of a target on a package, and a package does not have a link interface.

ament_export_interfaces on the other hand - at least atm - exports absolute information for non-target dependencies and therefore fails when those are being relocated.

Thank you SO MUCH! This clears up a long-standing mystery to me why I sometimes see headers being used from the underlay when they exist in my overlay.

I think #173 will need to be fixed before I can switch over to target-level linking.

@dirk-thomas
Copy link
Contributor

Thank you SO MUCH! This clears up a long-standing mystery to me why I sometimes see headers being used from the underlay when they exist in my overlay.

That is probably due to the wrong order of include directories. If you need to specify multiple you can make sure they follow the overlay/underlay order using this function:

#
# Order include directories according to chain of prefixes.
#
# :param var: the output variable name
# :type var: string
# :param ARGN: a list of include directories.
# :type ARGN: list of strings
#
# @public
#
macro(ament_include_directories_order var)

@dirk-thomas dirk-thomas changed the title ament_export_dependencies ignores package components ament_export_dependency_with_component Sep 9, 2019
@dirk-thomas dirk-thomas removed the more-information-needed Further information is required label Sep 9, 2019
@rotu
Copy link
Contributor Author

rotu commented Sep 9, 2019

That is probably due to the wrong order of include directories.

Maybe. I noticed it in on someone else's package that used include_directories instead of target_include_directories. If it's user error, it's a widespread one.

A quick codesearch through my ROS2 master source:
ament_include_directories_order: 1
target_include_directories: 211
include_directories: 119

Could you please document correct usage in the Ament CMake Documentation

@dirk-thomas
Copy link
Contributor

Could you please document correct usage in the Ament CMake Documentation

Please consider contributing to the tutorial: https://github.com/ros2/ros2_documentation/blob/master/source/Tutorials/Ament-CMake-Documentation.rst I will likely not have the bandwidth to do so in the near future.

@rotu
Copy link
Contributor Author

rotu commented Sep 9, 2019

Please consider contributing to the tutorial

I would if I understood well enough to make recommendations. I’ll see what I can do

@rotu rotu changed the title ament_export_dependency_with_component ament_export_dependencies ignores find_package(... components) Sep 10, 2019
@rotu
Copy link
Contributor Author

rotu commented Sep 10, 2019

Changing the name of this ticket back. The ticket describes a behavioral quirk, which users of the library should be aware of. The previous suggested title might be a good name for a PR that resolves this issue but does not describe the functional gap itself.

@dirk-thomas dirk-thomas added question Further information is requested wontfix This will not be worked on and removed enhancement New feature or request labels Sep 10, 2019
@dirk-thomas
Copy link
Contributor

Changing the name of this ticket back. The ticket describes a behavioral quirk, which users of the library should be aware of. The previous suggested title might be a good name for a PR that resolves this issue but does not describe the functional gap itself.

Sure, then I will mark this as question and wontfix since there is no way for the existing function to determine the components and no good way to extend the signature of the function to support per package components.

@rotu rotu changed the title ament_export_dependencies ignores find_package(... components) no way to ament_export a package with its components Sep 10, 2019
@rotu
Copy link
Contributor Author

rotu commented Sep 10, 2019

Alright. Revised the title yet again to somewhere in the middle. I suppose my title was a bit to prescriptive too

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

No branches or pull requests

2 participants