Support and prefer exported targets from rmw implementations #201

sloretz · 2021-12-15T23:24:09Z

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

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.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-12-15T23:28:48Z

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)

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-12-16T00:24:48Z

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)

Linux
Linux-aarch64
macOS
Windows

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

sloretz · 2021-12-16T00:59:03Z

CI again Single RMW implementation case witn rmw_cyclonedds

Repos: https://gist.githubusercontent.com/sloretz/26349bc77eb0931f8dbff9d3b4539d2d/raw/353b596d04cee3d2511ce7734c77a39fb87ddee2/ros2.repos
build: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.*
test: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.*

Jobs

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-12-16T13:25:40Z

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 · 2021-12-16T17:25:14Z

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 ros2/launch_ros#278 .

Possibly related to ros2/launch_ros#288 ?

sloretz · 2021-12-20T18:14:58Z

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 · 2021-12-20T18:17:40Z

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

build: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.*
test: --packages-ignore fastcdr foonathan_memory_vendor rosbag2_converter_default_plugins --packages-ignore-regex .*connext.* .*fastrtps.* --packages-select launch_xml
`

ivanpauno

LGTM!

I only have a minor comment.

ivanpauno · 2021-12-20T18:59:52Z

rmw_implementation/CMakeLists.txt

+    ament_index_cpp::ament_index_cpp
+    rcpputils::rcpputils
+    rcutils::rcutils
+    Threads::Threads)


why is Threads needed?
This library doesn't use threads.

If some dependency does need it, they should be exporting the link library correctly.
In my local testing I haven't found any issue with not adding Threads here, but I might have tested something incorrectly.

I think this was added because fastrtps wasn't exporting the dependency correctly at some point, but AFAIS it's not needed anymore

Removed unnecessary dependency on Threads:Threads in 8f00520

sloretz · 2021-12-21T23:02:20Z

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 · 2021-12-22T00:24:42Z

Another Windows run with ros2/launch#572 included

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-12-22T18:23:38Z

Yet another run after fixing ros2/launch#572

ivanpauno · 2021-12-23T12:56:06Z

Yet another run after fixing ros2/launch#572

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

sloretz · 2021-12-23T17:22:35Z

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

Cyclone DDS only

Linux - Checked parameters ok ✔️
Linux-aarch64 - Checked parameters ok ✔️
macOS
Windows

Fast-DDS static only

Linux - Checked parameters ok ✔️
Linux-aarch64 - Checked parameters ok ✔️
macOS
Windows

DDS Connext only

Linux - Checked parameters ok ✔️
Linux-aarch64 - Failure expected ✔️ - did not get connextdds flag set because CI job for Connext doesn't support arm64
macOS
Windows

Connext/Cyclone/FastDDS

Linux - Checked parameters ok ✔️
Linux-aarch64 - Checked parameters ok ✔️
macOS
Windows

sloretz · 2021-12-23T17:34:14Z

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.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-12-23T17:45:44Z

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

sloretz · 2021-12-23T19:14:23Z

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:

Linux
Linux-aarch64
macOS
Windows

Support and prefer exported targets from rmw implementations

00d565b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz self-assigned this Dec 15, 2021

Not all dependencies need to be exported

be7eee7

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

This was referenced Dec 15, 2021

Export only rmw::rmw to downstream targets ros2/rmw_cyclonedds#360

Merged

Mark dependencies as PRIVATE ros2/realtime_support#112

Merged

sloretz mentioned this pull request Dec 20, 2021

Fix build when only one rmw implementation is available #202

Closed

ivanpauno approved these changes Dec 20, 2021

View reviewed changes

sloretz mentioned this pull request Dec 22, 2021

Warn when loading launch extensions fails ros2/launch#572

Merged

Remove unnecessary dependency on FindThreads

8f00520

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

ivanpauno approved these changes Dec 23, 2021

View reviewed changes

Tests have more dependencies

c03b44f

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

ivanpauno approved these changes Dec 23, 2021

View reviewed changes

Blast545 mentioned this pull request Dec 23, 2021

👨‍🌾 rosbag2_transport Test regressions with cyclonedds + flaky with fastrtps ros2/rosbag2#933

Closed

sloretz merged commit afecc5d into master Dec 23, 2021

delete-merged-branch bot deleted the prefer_rmw_implementation_exported_targets branch December 23, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support and prefer exported targets from rmw implementations #201

Support and prefer exported targets from rmw implementations #201

sloretz commented Dec 15, 2021 •

edited

Loading

sloretz commented Dec 15, 2021

sloretz commented Dec 16, 2021 •

edited

Loading

sloretz commented Dec 16, 2021

clalancette commented Dec 16, 2021

sloretz commented Dec 16, 2021

sloretz commented Dec 20, 2021

sloretz commented Dec 20, 2021 •

edited

Loading

ivanpauno left a comment

ivanpauno Dec 20, 2021

ivanpauno Dec 20, 2021

sloretz Dec 22, 2021

sloretz commented Dec 21, 2021

sloretz commented Dec 22, 2021

sloretz commented Dec 22, 2021

ivanpauno commented Dec 23, 2021

sloretz commented Dec 23, 2021 •

edited

Loading

sloretz commented Dec 23, 2021

sloretz commented Dec 23, 2021

sloretz commented Dec 23, 2021

Support and prefer exported targets from rmw implementations #201

Support and prefer exported targets from rmw implementations #201

Conversation

sloretz commented Dec 15, 2021 • edited Loading

sloretz commented Dec 15, 2021

sloretz commented Dec 16, 2021 • edited Loading

sloretz commented Dec 16, 2021

clalancette commented Dec 16, 2021

sloretz commented Dec 16, 2021

sloretz commented Dec 20, 2021

sloretz commented Dec 20, 2021 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno Dec 20, 2021

Choose a reason for hiding this comment

ivanpauno Dec 20, 2021

Choose a reason for hiding this comment

sloretz Dec 22, 2021

Choose a reason for hiding this comment

sloretz commented Dec 21, 2021

sloretz commented Dec 22, 2021

sloretz commented Dec 22, 2021

ivanpauno commented Dec 23, 2021

sloretz commented Dec 23, 2021 • edited Loading

sloretz commented Dec 23, 2021

sloretz commented Dec 23, 2021

sloretz commented Dec 23, 2021

sloretz commented Dec 15, 2021 •

edited

Loading

sloretz commented Dec 16, 2021 •

edited

Loading

sloretz commented Dec 20, 2021 •

edited

Loading

sloretz commented Dec 23, 2021 •

edited

Loading