Install includes to include/${PROJECT_NAME} and use modern CMake

[sloretz]

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#365

This removes ament_export_libraries and ament_export_include_directories as they're redundant with the exported CMake targets in packages that I was already changing. I only updated packages that installed headers.

Part of ament/ament_cmake#292

This replaces ament_target_dependencies() calls with target_link_libraries() in packages that I was already changing. I only updated packages that installed headers.

This also removes the dependency on eigen3_cmake_module in packages that I was already changing. It's no longer necessary now that we can use modern CMake targets. I only updated packages that installed headers.

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

[clalancette]

I've left a number of questions inline. I basically have two overarching questions:

1. If we remove ament_export_include_directories and ament_export_libraries, what happens to downstream projects that are depending on the old-style CMake variables?
2. I'm fine with switching from ament_target_dependencies to target_link_libraries where we can, but shouldn't we be explicitly listing all dependencies in target_link_libraries? There seem to be a number of places where we are now implicitly depending on other targets.

[sloretz]

If we remove ament_export_include_directories and ament_export_libraries, what happens to downstream projects that are depending on the old-style CMake variables?

So far, they all still work. The reason is all the downstream projects I've come across are using ament_target_dependencies(), which reads either old-style CMake variables or CMake targets.

A common CMake pattern I've seen outside of ROS is to put exported targets into a "..._LIBRARIES" variable taking advantage of downstream targets are likely already doing target_link_libraries(...) with it. However, ament_cmake puts the targets into a separate ..._TARGETS variable, so that doesn't apply here.

I'm fine with switching from ament_target_dependencies to target_link_libraries where we can, but shouldn't we be explicitly listing all dependencies in target_link_libraries? There seem to be a number of places where we are now implicitly depending on other targets.

I've been intentionally leaving it off when a target is getting dependencies from another target in the same CMake project. My reasoning is it tests that the CMake target we're exporting properly made those targets as PUBLIC or INTERFACE. Say there was a package with a shared library and a test executable. Now say there's a bug where the shared library says it depends on rclcpp::rclcpp with the PRIVATE keyword, but it should have used the PUBLIC keyword. If the test also target_link_libraries(... rclcpp::rclcpp) then the project will build fine and the tests will pass, but when that package is released downstream projects will fail to build because the shared library target didn't properly export it's dependency on rclcpp::rclcpp. If the test instead got that dependency from the shared library target then the test would have failed to build and caught that bug before release.

[sloretz]

I rebased this and addressed all feedback.

CI

• build: --packages-above-and-dependencies examples_tf2_py geometry2 test_tf2 tf2 tf2_bullet tf2_eigen tf2_eigen_kdl tf2_geometry_msgs tf2_kdl tf2_msgs tf2_py tf2_ros tf2_ros_py tf2_sensor_msgs tf2_tools
• test: --packages-select examples_tf2_py geometry2 test_tf2 tf2 tf2_bullet tf2_eigen tf2_eigen_kdl tf2_geometry_msgs tf2_kdl tf2_msgs tf2_py tf2_ros tf2_ros_py tf2_sensor_msgs tf2_tools

Jobs

• Linux
• Linux-aarch64
• Windows

[sloretz]

https://github.com/ros2/choco-packages/blob/latest/package/eigen/share/cmake/Eigen3Config.cmake

Windows CI failed because we ship a custom Eigen3Config.cmake on Windows that doesn't provide targets instead of using upstreams. This is the same reason I upgraded bullet, but I don't have time to upgrade that one before the freeze. I'll undo all the target stuff for packages using Eigen to get Windows to pass.

[sloretz]

5cedb80 should resolve the windows issue. CI rerun:

• Linux
• Linux-aarch64
• Windows

[clalancette]

I've left a couple of minor things (and Windows CI needs to be fixed). With those things done, I'll be happy to approve.

[sloretz]

CI re-run:

• Linux
• Linux-aarch64
• Windows

[sloretz]

CI LGTM 🎉 @clalancette Mind taking another look?