search

ros2 / rmw_cyclonedds

ROS 2 RMW layer for Eclipse Cyclone DDS
Apache License 2.0
108 stars 89 forks source link

Update to work with Cyclone 0.9.0 and Iceoryx 2.0 #379

Closed sumanth-nirmal closed 2 years ago

sumanth-nirmal commented 2 years ago

Updates rmw_cyclonedds_cpp to work with Cyclone 0.9.0 and Iceoryx 2.0

Fixes #378

dkroenke commented 2 years ago

@sumanth-nirmal @MatthiasKillat The documentation for shared memory support needs to be adapted because the parameters SubQueueCapacity, SubHistoryRequest, and PubHistoryCapacity are now deprecated.

clalancette commented 2 years ago

Just as an FYI, the RMW freeze for Humble is on Monday, March 21 (one week from today). Assuming you want to get this into Humble, this PR, and any change to https://github.com/ros2/ros2/blob/master/ros2.repos to upgrade CycloneDDS and Iceoryx will need to pass full CI and be merged by then.

sumanth-nirmal commented 2 years ago

@eboasson @MatthiasKillat this should be ready for review, and I think we will also need a corresponding MR in ros2.repos updating the cyclone and iceoryx versions

eboasson commented 2 years ago

A still need to review it carefully, but a quick look says it's fine. Furthermore a local sanity check works fine for me with one patch applied, see https://github.com/eboasson/rmw_cyclonedds/tree/fix-localhost

Let's see what a ROS 2 CI run makes of it with Cyclone's current master branch and the extended version of this PR from this fix-localhost branch:

So far so good on Linux x64: it looks like it picked up Iceoryx, there's one "maybe uninitialised" warning that I'll look into and there are the expected deprecation warnings from OpenSSL 3.0 (I'll turn those off on the release branch, they'll get fixed properly on master after the release).

eboasson commented 2 years ago

Another go at full CI of https://github.com/ros2/ros2/pull/1248 + https://github.com/ros2/rmw_cyclonedds/pull/379

eboasson commented 2 years ago

CI for only cyclonedds to verify that the

projectroot.src.core.ddsc.tests.start_roudi

test failure in the full CI run above is now gone:

All the other failures appear to be unrelated to the proposed change.

eboasson commented 2 years ago

And another one of just Cyclone DDS — hopefully this eliminates the CMake warning on Windows:

eboasson commented 2 years ago

@dkroenke it looks like I screwed up my local repositories file a bit: I believe the runs did include the fixes I meant to include, but it would've been nice if the second run included the first run's fix ... 🤓

Both are in the Cyclone release branch now. Perhaps it would be a good idea to do one final run just to get all green, and I am thinking that perhaps it is good if it is an entirely independent effort to minimise the likelihood of screwing up branches in the same way!

dkroenke commented 2 years ago

@dkroenke it looks like I screwed up my local repositories file a bit: I believe the runs did include the fixes I meant to include, but it would've been nice if the second run included the first run's fix ... nerd_face

Both are in the Cyclone release branch now. Perhaps it would be a good idea to do one final run just to get all green, and I am thinking that perhaps it is good if it is an entirely independent effort to minimise the likelihood of screwing up branches in the same way!

@eboasson Yes right, I will not start anything on the CI and there is nothing running from my side. I guess the final run can be started once you are ready.

eboasson commented 2 years ago

Thanks to @dkroenke now it is the correct set of builds.

dkroenke commented 2 years ago

@eboasson referring to https://github.com/ros2/ros2/pull/1248#issuecomment-1072758151 we are good to go here. The CI seems to be ok and the failing rosbag2_transport test is already failing in the nightlies from time to time: https://ci.ros2.org/view/nightly/job/nightly_linux_release/2192/ https://ci.ros2.org/view/nightly/job/nightly_linux_release/2210/#showFailuresLink

CC @sumanth-nirmal

sumanth-nirmal commented 2 years ago

@clalancette and @eboasson Is there anything else needed for this PR to be merged?

clalancette commented 2 years ago

@clalancette and @eboasson Is there anything else needed for this PR to be merged?

I'm waiting for an answer to https://github.com/ros2/ros2/pull/1248#issuecomment-1072758151