ros2 / message_filters

BSD 3-Clause "New" or "Revised" License
76 stars 66 forks source link

Correct package.xml and CMakeLists.txt #58

Closed hidmic closed 3 years ago

hidmic commented 3 years ago

This patch revisits and corrects this package's metadata and build system. Namely,

hidmic commented 3 years ago

@clalancette It looks like build_export_depend tags are not picked by the build and install step in CI. Compare https://build.ros2.org/job/Rpr__message_filters__ubuntu_focal_amd64/18/console#console-section-3 (failing job) with https://build.ros2.org/job/Rpr__message_filters__ubuntu_focal_amd64/17/consoleFull#console-section-10 (passing job).

Is it intentional or is it a bug?

hidmic commented 3 years ago

Assuming it is intentional, I think the problem is that we shouldn't be doing the find_package(rclcpp) during the build phase, but only in the test phase. That is, it isn't required for rclcpp to be there during build, but it is required during tests, and for any downstream packages that depend on it (hence the test_depend and the build_export_depend). Does that make sense?

~The thing is, we still have to invoke ament_export_dependencies() for rclcpp to be exported for downstream CMake package consumption. And that has to happen regardless of whether it was built for testing or not.~

Scratch that, exportation does not require we find_package() it. It just puts together a list. ~I'll fix it~

Hmm, wait. We can export rclcpp without finding it but we can't target it as a dependency. Which, if I'm not mistaken, will break downstream packages that use modern CMake targets instead of ament_target_dependencies(). Actually, it'll break them all because the latter prioritizes modern CMake targets.

hidmic commented 3 years ago

CI up to message_filters and tf2_ros (a dependent package):

hidmic commented 3 years ago

I'd say let's do a full --packages-above-and-dependencies CI build, just to make sure we get as much coverage as possible.

Sounds reasonable:

hidmic commented 3 years ago

Alright, all green. Going in. Thanks @clalancette !