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.

Sign up for GitHub

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

Jump to bottom

[tf2_ros, tf2_geometry_msgs] export targets in addition to include directories / libraries #271

Merged
merged 4 commits into from
Jun 4, 2020
Merged

[tf2_ros, tf2_geometry_msgs] export targets in addition to include directories / libraries #271

merged 4 commits into from
Jun 4, 2020

Conversation

dirk-thomas
Copy link
Member

Necessary to make downstream packages like rviz_common which already use modern CMake targets relocatable.

See ros2/rviz#554 and ros2/rviz#556.

@dirk-thomas
[tf2_ros] export targets in addition to include directories / libraries
d19162f 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas added the bug Something isn't working label Jun 2, 2020
@dirk-thomas dirk-thomas self-assigned this Jun 2, 2020
@dirk-thomas
add missing exported dependencies
0550d50 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
jacobperron
jacobperron approved these changes Jun 2, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@dirk-thomas
Copy link
Member Author

CI see ros2/message_filters#46.

@dirk-thomas
[tf2_geometry_msgs] export targets in addition to include directories…
89d38e3 
… / libraries

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas changed the title [tf2_ros] export targets in addition to include directories / libraries [tf2_ros, tf2_geometry_msgs] export targets in addition to include directories / libraries Jun 3, 2020
clalancette
clalancette reviewed Jun 3, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I'll note that this overlaps somewhat with #269. That's fine, I can rebase that one later. But I'd also be careful to run some of the same tests in CI that failed there; namely, make sure to run rviz_common and rviz_default_plugins tests.

tf2_geometry_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
@dirk-thomas
Copy link
Member Author

dirk-thomas commented Jun 3, 2020 •
edited
Loading

But I'd also be careful to run some of the same tests in CI that failed there; namely, make sure to run rviz_common and rviz_default_plugins tests.

Another full CI of this branch (which includes ros2/message_filters#46):

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

@dirk-thomas
remove install / export_include_dirs which is redundant with ament_au…
eb64bb5 
…to_package()

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas mentioned this pull request Jun 3, 2020
Closed
jacobperron
jacobperron approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This patch (with connected PRs) resolves ros2/rviz#554 and (partially) ros2/rviz#556 for me. CI also LGTM.

For ros2/rviz#556, I still see cmake errors related to rclcpp as noted in ros2/rviz#556 (comment).

I think we should go ahead and merge.

@dirk-thomas dirk-thomas merged commit 5880ce9 into ros2 Jun 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/export-targets branch June 4, 2020 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@clalancette clalancette Awaiting requested review from clalancette clalancette is a code owner

Assignees

@dirk-thomas dirk-thomas

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dirk-thomas @clalancette @jacobperron