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

Install includes to include/ and misc CMake fixes #225

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

sloretz
Copy link

@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

@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
Contributor

@ros-pull-request-builder retest this please

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

None yet

3 participants