search

ros2 / rmw_implementation

CMake infrastructure and dependencies for rmw implementations
Apache License 2.0
21 stars 48 forks source link

Support and prefer exported targets from rmw implementations #201

Closed sloretz closed 2 years ago

sloretz commented 2 years ago

@Blast545 FYI this is an attempt to fix the CI issue from ros2/rmw_cyclonedds#357 ~Requires ros2/rmw_cyclonedds#360~ :heavy_check_mark: ~Requires ros2/realtime_support#112~ :heavy_check_mark:

The package rmw_implementation appears to be the CMake interface between downstream libraries like rcl and the actual rmw implementations. It looks like when there are multiple rmw_implementations it will export a shared library that can load and switch between them, but when there is only one implementation it sets old-style standard CMake variables to the underlying implementation. Unfortunately, the latter expects the rmw implementation to set old-style CMake variables instead of modern CMake targets.

This PR creates an IMPORTED target rmw_implementation::rmw_implementation and variable rmw_implementation_TARGETS that lets downstream packages like rcl depend on targets instead of old-style CMake variables. It also updates the logic to support, and prefer, modern CMake from the rmw implementations when available.

I also replaced an out-of-place CMAKE_THREAD_LIBS_INIT with a dependency on Threads::Threads on the shared library.

sloretz commented 2 years ago

The single rmw implementation case and multi rmw implementation cases both need to be tested, so I'll do CI in stages as long as it looks positive.

First: Does it work with the single RMW implementation rmw_cyclonedds? I would expect a build failure if not, so not running many tests to get quicker CI turnaround

CI (build: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.* test: --packages-select rmw_implementation rcl rclcpp_components)

sloretz commented 2 years ago

Retrying CI with a repos file with the two linked PRs: https://gist.githubusercontent.com/sloretz/26349bc77eb0931f8dbff9d3b4539d2d/raw/353b596d04cee3d2511ce7734c77a39fb87ddee2/ros2.repos

CI (build: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.* test: --packages-select rmw_implementation rcl rclcpp_components)

EDIT: Failing due to test arguments not ignoring unbuilt packages, still, building all the way is promising

sloretz commented 2 years ago

CI again Single RMW implementation case witn rmw_cyclonedds

Jobs

clalancette commented 2 years ago

Hm, Windows CI is cranky, failing to load certain DLLs. I'm not sure if that is related to the change, or maybe to the set of build/test flags? Either way it probably needs to be looked into.

sloretz commented 2 years ago

Hm, Windows CI is cranky, failing to load certain DLLs. I'm not sure if that is related to the change, or maybe to the set of build/test flags? Either way it probably needs to be looked into.

It's very interesting that launch_xml, a package that doesn't depend directly or indirectly on rclpy, is failing to import it via launch_ros :-/ . This reminds me of the bug where pytest hooks in a merged workspace were loading rclpy https://github.com/ros2/launch_ros/pull/278 .

Possibly related to https://github.com/ros2/launch_ros/pull/288 ?

sloretz commented 2 years ago

Hm, Windows CI is cranky, failing to load certain DLLs. I'm not sure if that is related to the change, or maybe to the set of build/test flags? Either way it probably needs to be looked into.

I setup a windows VM and ran the same arguments, and I can't reproduce it. I'll try another CI run to see if it's a flaky issue.

sloretz commented 2 years ago

CI Windows this branch instead of repos file since required PRs in other repos are in

sloretz commented 2 years ago

Try as I might, I can't reproduce the Windows CI test failure locally on a windows VM. I'm going to apply Ivan's feedback and open up a mitigation in launch.

sloretz commented 2 years ago

Another Windows run with ros2/launch#572 included Build Status

sloretz commented 2 years ago

Yet another run after fixing ros2/launch#572 Build Status

ivanpauno commented 2 years ago

Yet another run after fixing ros2/launch#572 Build Status

It's really strange that now there's no even a warning :open_mouth:

sloretz commented 2 years ago

CI (build: --packages-up-to rcl test: --packages-select rcl)

Cyclone DDS only

Fast-DDS static only

DDS Connext only

Connext/Cyclone/FastDDS

sloretz commented 2 years ago

Something odd going on with the above CI jobs - lots of them don't have the parameters I set on the launcher. https://ci.ros2.org/job/ci_linux-aarch64/10524/parameters/

There might be a ros2/ci bug here. Investigating.

sloretz commented 2 years ago

No CI bugs! Failing combined CI is due to tests having dependencies that they weren't target-linked against. Fixed in c03b44f

sloretz commented 2 years ago

Since c03b44f modifies code that's only enabled in the multi-RMW case, I think the green CI for the single RMW cases is still valid. I'll rerun the CI for the connext/cyclone/fastdds combinded jobs only: