Skip to content

Install includes to include/ and misc CMake fixes#225

Merged
clalancette merged 2 commits intoros-drivers:ros2from
sloretz:sloretz__easy_idso_part1
Jan 28, 2022
Merged

Install includes to include/ and misc CMake fixes#225
clalancette merged 2 commits intoros-drivers:ros2from
sloretz:sloretz__easy_idso_part1

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Jan 5, 2022

Part of ros2/ros2#1150

This installs includes to include/${PROJECT_NAME} to mitigate include directory search order issues when overriding packages in desktop.

Part of ament/ament_cmake#292

This replaces ament_target_dependencies() calls with target_link_libraries().

I also made changes to use modern CMake targets, notably by creating IMPORTED targets wiimote::bluetooth and wiimote::cwid to make the wiimote::wiimote_lib target exportable.

Requires ros2/rclcpp#1855 for the rclcpp_components::component target.

Copy link
Copy Markdown

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@clalancette
Copy link
Copy Markdown
Contributor

@ros-pull-request-builder retest this please

@clalancette
Copy link
Copy Markdown
Contributor

We're actually going to have to split this up into several different branches, since we only want to land this change on ros2. I'm going to go ahead and make a separate foxy-devel branch here, then retarget Foxy and Galactic to that branch.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@clalancette clalancette force-pushed the sloretz__easy_idso_part1 branch from 5348737 to e4ff192 Compare January 28, 2022 18:04
@clalancette
Copy link
Copy Markdown
Contributor

I've rebased this onto the latest, which should 🤞 make CI pass here now.

Make sure to really declare and find all dependencies.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Copy Markdown
Contributor

@sloretz I just pushed 0d2b8e5 here, which adds in some additional infrastructure that the CMake and package.xml files were missing. This now looks good to me, and passes CI, so I'm going to approve it. However, since I did enough additional work here, I'd appreciate a comment from you on whether you agree with the changes I've done before I merge this.

@clalancette
Copy link
Copy Markdown
Contributor

I'm going to go ahead with this one so we can get a release out for Rolling and try to fix the spacenav regression. If anyone wants to do a review after merge, I'm happy to make follow-up changes.

@clalancette clalancette merged commit 6e67b78 into ros-drivers:ros2 Jan 28, 2022
@sloretz sloretz deleted the sloretz__easy_idso_part1 branch January 31, 2022 17:22
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.

3 participants