Install includes to ${PROJECT_NAME} folder and use modern CMake #58
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Yeah, totally agreed on that. I'll go ahead and do that. |
|
ros/rosdistro#31688 is now merged in, so Rolling targets ros2. I'm thus going to retarget this PR to the 'ros2' branch. |
|
@ros-pull-request-builder retest this please |
Ah, this probably requires the |
I was just going to mention that :). |
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
CMakeLists.txt
Outdated
| target_link_libraries(DepthImageToLaserScanROS PUBLIC | ||
| rclcpp::rclcpp) | ||
| target_link_libraries(DepthImageToLaserScanROS PRIVATE | ||
| rcutils::rcutils |
There was a problem hiding this comment.
I know this isn't of your doing, but DepthImageToLaserScanROS.cpp doesn't actually need to include rcutils, and hence doesn't need to directly link against it. So I'd be in favor of removing that unnecessary include, and removing this.
(rcutils also isn't properly exposed as a dependency in package.xml or CMakeLists.txt)
There was a problem hiding this comment.
Removed traces of rcutils in 9926f09
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
There was a problem hiding this comment.
Looks good to me, thanks for iterating.
We should be able to get the Rpr job working in a couple of hours, assuming the current Rolling rebuild on https://build.ros2.org is successful 🤞
|
Looks like |
|
@ros-pull-request-builder retest this please |
…perception#58) * Install includes to ${PROJECT_NAME} folder and use modern CMake * Remove traces of rcutils Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
TargetingTargetingfoxy-develand notros2because the former seems to be marked as the upstream development branch, but I would strongly recommend releasing this change only to ROS Rolling. Maybe it's time to fast-forward ros2 to foxy-devel and change the ROS Rolling branch toros2?ros2now 🎉https://index.ros.org/p/depthimage_to_laserscan/#rolling
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#365This exports modern CMake targets and removesEdit: Added back to minimize disruptionament_export_librariesandament_export_include_directoriesas they're redundant with the exported CMake targets.Part of ament/ament_cmake#292
This replaces
ament_target_dependencies()calls withtarget_link_libraries().I also reduced the dependency on OpenCV from all of it, to just the core library as that's all that appears to be used.