Switch robot_localization to modern CMake idioms. #895
Conversation
|
Thanks for this! I owe this package some attenion and will get to it soon. |
Some of these have been best practice for a while, and some of them will become best practice in the near future. 1. Move the include directory down one-level. This is to better support overlays and underlays; see ros2/ros2#1150 for the entire saga. The upshot is that all includes should be one more directory level down, and CMake will handle the differences. This has been best practice since Humble. 2. Switch from ament_target_dependencies() to target_link_libraries(). ament_target_dependencies was developed in the days before target_link_libraries() was fully capable, and nowadays target_link_libraries() is a superset of the functionality. There is one oddity here, in that in order to deal with ROS message packages, we have to use ${<msg_pkg_name>_TARGETS}, rather than the traditional CMake target like msg_pkg_name::msg_pkg_name. This is a bug in ROS that will eventually be fixed. 3. Export a CMake target from robot_localization. This means that downstream packages will be able to use target_link_libraries(<target> robot_localization::robot_localization) 4. Export all dependencies with ament_export_dependencies. This ensures that downstream project which rely on this one will be able to find all of the includes. Signed-off-by: Chris Lalancette <[email protected]>
clalancette
force-pushed
the
clalancette/switch-to-modern-cmake
branch
from
5acbd41 to
6ec1732
Compare
There was a problem hiding this comment.
This also fails with
06:34:49 33: [test_ekf_node_interfaces-2] [ RUN ] InterfacesTest.ImuDifferentialIO
06:34:49 33: [test_ekf_node_interfaces-2] /tmp/ws/src/robot_localization/test/test_se_node_interfaces.cpp:830: Failure
06:35:01 33: [test_ekf_node_interfaces-2] Expected: (std::abs(rollFinal)) < (0.2), actual: 1.2032755481031931 vs 0.2
06:35:01 33: [test_ekf_node_interfaces-2]
06:35:01 33: [test_ekf_node_interfaces-2] /tmp/ws/src/robot_localization/test/test_se_node_interfaces.cpp:831: Failure
06:35:01 33: [test_ekf_node_interfaces-2] Expected: (std::abs(pitchFinal)) < (0.2), actual: 0.65010049931252023 vs 0.2
06:35:01 33: [test_ekf_node_interfaces-2]
06:35:01 33: [test_ekf_node_interfaces-2] /tmp/ws/src/robot_localization/test/test_se_node_interfaces.cpp:832: Failure
06:35:01 33: [test_ekf_node_interfaces-2] Expected: (std::abs(yawFinal)) < (0.2), actual: 2.2377817033514558 vs 0.2
06:35:01 33: [test_ekf_node_interfaces-2]
06:35:01 33: [ekf_node-1] [INFO] [1724420101.340558210] [test_se_node_interfaces]: Received a request to reset filter.
06:35:01 33: [test_ekf_node_interfaces-2] [ FAILED ] InterfacesTest.ImuDifferentialIO (12004 ms)
I take it that we should ignore it then for the purposes of these PRs?
For context @ayrton04, Chris is helping me with Nav2 by updating Nav2's CMake system which this is needed as an element of
|
I'm pretty sure the pitch one has given me trouble in the past, but all three Euler angles failing is a little strange. I also feel like this test fails more in ROS 2 than it did in ROS 1. These tests were stable at one point. 🤔 Are the values consistent when failing? That would be more concerning. Obviously there's nothing in this PR that could cause any issues with the actual state estimation, but it should be fixed. Maybe I'll look into this tonight. |
|
@SteveMacenski do we need this in any other branches? |
Not to speak for Steve, but I don't believe we do. In particular, this is to support ros-navigation/navigation2#4648 , which I believe will only be merged for Rolling right now. That said, it would help that PR immensely if we could have a new |
|
Will do! I'm behind on those anyway. Thanks for the PRs! |
|
Agree with Chris! |
|
Some of these have been best practice for a while, and some of them will become best practice in the near future.
Move the include directory down one-level. This is to better support overlays and underlays; see Overlaying packages using CMake export targets can fail with merge install underlay ros2/ros2#1150 for the entire saga. The upshot is that all includes should be one more directory level down, and CMake will handle the differences. This has been best practice since Humble.
Switch from ament_target_dependencies() to target_link_libraries(). ament_target_dependencies was developed in the days before target_link_libraries() was fully capable, and nowadays target_link_libraries() is a superset of the functionality. There is one oddity here, in that in order to deal with ROS message packages, we have to use ${<msg_pkg_name>_TARGETS}, rather than the traditional CMake target like msg_pkg_name::msg_pkg_name. This is a bug in ROS that will eventually be fixed.
Export a CMake target from robot_localization. This means that downstream packages will be able to use
target_link_libraries( robot_localization::robot_localization)
Export all dependencies with ament_export_dependencies. This ensures that downstream project which rely on this one will be able to find all of the includes.