ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
89 stars 44 forks source link

Enable topic "ros_discovery_info" for rmw_connextdds #253

Closed asorbini closed 3 years ago

asorbini commented 3 years ago

This PR replaces all references to rmw_connext_cpp with rmw_connextdds.

It also adds rmw_connextdds to the list of RMW implementations which support "built-in" topic ros_discovery_info.

See rticommunity/rmw_connextdds #9 for a list of related PRs, and an overview of all the changes required to replace ros2/rmw_connext (rmw_connext_cpp) with rticommunity/rmw_connextdds in the ROS2 source tree.

kyrofa commented 3 years ago

Does it make sense to actually remove support for rmw_connext_cpp before it's also removed from the master repos? Maybe it makes more sense to add support for the new one now, and remove support for the old one when it's actually gone.

codecov[bot] commented 3 years ago

Codecov Report

Merging #253 (87dd954) into master (2b520b9) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #253   +/-   ##
=======================================
  Coverage   77.25%   77.25%           
=======================================
  Files          25       25           
  Lines         664      664           
  Branches       55       55           
=======================================
  Hits          513      513           
  Misses        131      131           
  Partials       20       20           
Flag Coverage Δ
unittests 77.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s_ws/src/j68c7tcb0ir/sros2/sros2/sros2/__init__.py
.../j68c7tcb0ir/sros2/sros2/sros2/command/security.py
.../sros2/sros2/sros2/policy/defaults/dds/__init__.py
...0ir/sros2/sros2/sros2/policy/templates/__init__.py
...cb0ir/sros2/sros2/sros2/verb/generate_artifacts.py
...j68c7tcb0ir/sros2/sros2/sros2/keystore/__init__.py
...cb0ir/sros2/sros2/sros2/policy/schemas/__init__.py
...c/j68c7tcb0ir/sros2/sros2/sros2/policy/__init__.py
...c7tcb0ir/sros2/sros2/sros2/verb/create_keystore.py
...68c7tcb0ir/sros2/sros2/sros2/keystore/_keystore.py
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2b520b9...87dd954. Read the comment docs.

asorbini commented 3 years ago

I don't think the PR really precludes the use of rmw_connext_cpp, since all those changes are in documentation.

The code change will be needed to have tests pass once we start testing on ci.ros2.org (so far I've been testing with a modified ros2.repos).

I'm trying to coordinate all changes linked in #9 with the help of @ivanpauno and @clalancette, and I appreciate any input in making this transition as smooth as possible for everyone.

In general, I think this PR might be one of the lower priority ones from the list, so we can delay merging it until other changes have been put into place.

ivanpauno commented 3 years ago

Does it make sense to actually remove support for rmw_connext_cpp before it's also removed from the master repos? Maybe it makes more sense to add support for the new one now, and remove support for the old one when it's actually gone.

I agree with that, but this PR doesn't seem to be removing support of rmw_connext_cpp. It's just replacing in the tutorials rmw_connext_cpp with rmw_connextdds, and adding rmw_connextdds to the _RMW_WITH_ROS_GRAPH_INFO_TOPIC list.

We could also keep a reference to rmw_connext_cpp in the tutorials, but I don't think it's worth it.

mikaelarguedas commented 3 years ago

We could also keep a reference to rmw_connext_cpp in the tutorials, but I don't think it's worth it.

If the change to the test is necessary for the switch to rmw_connextdds to move forward I'm :+1:

However we should not break the tutorials by referencing not yet existing rmw implementations in the nightlies. The tutorials changes can be opened as a follow-up once rmw_connextdds is integrated and part of the artifacts produced by the buildfarm.

[ERROR] [1615413395.850699851] [rcl]: Error getting RMW implementation identifier / RMW implementation not installed (expected identifier of 'rmw_connextdds'), with error message 'failed to load shared library 'librmw_connextdds.so' due to dlopen error: librmw_connextdds.so: cannot open shared object file: No such file or directory, at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rcutils/src/shared_library.c:99, at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:74', exiting with 1., at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rcl/rcl/src/rcl/rmw_implementation_identifier_check.c:139
asorbini commented 3 years ago

I restored rmw_connext_cpp in the tutorials. I will open a new PR to update them again once rmw_connextdds makes it into the nightly builds.

SidFaber commented 3 years ago

No concerns here @clalancette, thanks