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

Export libraries to trigger hooks. #358

Merged
merged 2 commits into from
Oct 11, 2018
Merged

Export libraries to trigger hooks. #358

merged 2 commits into from
Oct 11, 2018

Conversation

nuclearsandwich
Copy link
Member

Add ament_export_libraries calls to export libraries and set necessary environment hooks. The rviz packages are some of a handful which install libraries exporting them.

In conversation with @wjwwood we surmised that this was likely a small oversight rather than something that was done intentionally.

Connects to ros2/ros_workspace#10

@nuclearsandwich nuclearsandwich self-assigned this Oct 8, 2018
@nuclearsandwich nuclearsandwich added the in progress Actively being worked on (Kanban column) label Oct 8, 2018
@@ -241,6 +241,7 @@ ament_target_dependencies(rviz_default_plugins

ament_export_include_directories(include)
ament_export_interfaces(rviz_default_plugins)
ament_export_libraries(rviz_default_plugins)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is redundant with ament_export_interfaces, or rather whether or not that function should also extend the library paths.

@dirk-thomas what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While both do kind of the same thing they work differently:

  • ament_export_interfaces is used to export the targets via CMake install(EXPORT ...)
  • ament_export_libraries exports libraries using the variable <pkgname>_LIBRARIES

Both make sense depending on how downstream packages want to use them.

Since the target is a library ament_export_interfaces should pass the option HAS_LIBRARY_TARGET (see https://github.com/ament/ament_cmake/blob/88add8cab0ca579e89e95b3ab343af319c163195/ament_cmake_export_interfaces/cmake/ament_export_interfaces.cmake#L22-L24).

HAS_LIBRARY_TARGET

Replaces ament_export_libraries with the HAS_LIBRARY_TARGET option for
ament_export_interfaces.
@nuclearsandwich
Copy link
Member Author

I've updated this PR to use the HAS_LIBRARY_TARGET option rather than ament_export_libraries as all I needed was the environment hook to be set.

I swapped rather than did both since I think it's reasonable to make the minimal change unless maintainers prefer otherwise.

@nuclearsandwich
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Oct 11, 2018

@mjcarroll you might want to consider this pr in the migration of the yaml cpp stuff.

@wjwwood wjwwood merged commit 3fc5e3f into ros2 Oct 11, 2018
@wjwwood wjwwood deleted the environment-hooks-sweep branch October 11, 2018 16:59
@wjwwood wjwwood removed the in progress Actively being worked on (Kanban column) label Oct 11, 2018
@nuclearsandwich nuclearsandwich restored the environment-hooks-sweep branch December 11, 2018 19:17
@clalancette clalancette deleted the environment-hooks-sweep branch September 3, 2020 13:54
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

Successfully merging this pull request may close these issues.

None yet

3 participants