Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for rmw_connextdds #671

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

asorbini
Copy link
Contributor

@asorbini asorbini commented Mar 5, 2021

This PR removes all references to rmw_connext_cpp, so that it may be replaced by rmw_connextdds.

The PR re-enables a test which was previously disabled for Connext.

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.

Signed-off-by: Andrea Sorbini <asorbini@rti.com>
@asorbini asorbini requested a review from a team as a code owner March 5, 2021 22:10
@asorbini asorbini requested review from jaisontj and zmichaels11 and removed request for a team March 5, 2021 22:10
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming green CI - are there any dependencies for this PR? I assume if I just ran ci_launcher now, it would fail on the rmw_connext_cpp tests?

@asorbini
Copy link
Contributor Author

asorbini commented Mar 5, 2021

LGTM assuming green CI - are there any dependencies for this PR? I assume if I just ran ci_launcher now, it would fail on the rmw_connext_cpp tests?

I think you are correct. The merging of this PR will need to be coordinated with changes to the default ros2.repos, and updates to ros2/ci to fully replace rmw_connext_cpp.

I'm afraid there will be a short window of instability as all these changes are rolled out, but hopefully we will be able keep all failures to a minimum and make the transition as quick as possible.

Re: testing, you may want to try using this modified ros2.repos.

@emersonknapp
Copy link
Collaborator

I'm afraid there will be a short window of instability as all these changes are rolled out, but hopefully we will be able keep all failures to a minimum and make the transition as quick as possible.

I bet there is a way to do it without breaking anything :)

# disable the following tests for connext
# due to slower discovery of nodes
set(SKIP_TEST "")
if(${rmw_implementation} MATCHES "(.*)connext(.*)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could keep a (.*)connext(.*)_cpp filter here, which would skip the test for the old rmw_connext_cpp and rmw_connext_dynamic_cpp, but not for rmw_connextdds.

…unity/rmw_connextdds

Signed-off-by: Andrea Sorbini <asorbini@rti.com>
@asorbini asorbini changed the title Replace rmw_connext_cpp with rmw_connextdds Add support for rmw_connextdds Mar 11, 2021
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me with green CI.

@ivanpauno
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

CI looks good (the warnings in aarch64 doesn't matter here, as rmw_connextdds wasn't added to the official ros2.repos here).

@emersonknapp could you merge this PR? thanks!

@clalancette
Copy link
Contributor

CI looks good (the warnings in aarch64 doesn't matter here, as rmw_connextdds wasn't added to the official ros2.repos here).

It's not a problem for this PR (it won't make any difference), but it is probably going to be a problem for integrating the new RMW onto ci.ros2.org. But we can address that separately.

@ivanpauno
Copy link
Member

It's not a problem for this PR (it won't make any difference), but it is probably going to be a problem for integrating the new RMW onto ci.ros2.org. But we can address that separately.

The trick currently in use in ci is that when the CI_USE_CONNEXT_STATIC option is not ☑️ , the rmw_connext_cpp and rmw_connext_shared_cpp packages are ignored.
By default, that option is not selected in aarch64 jobs, and it actually goes red if you click it.

So, I would follow the same idea here, ignoring those packages in the ci scripts when CI_USE_CONNEXT_STATIC is not chosen, rather than downgrading the warning to an info message.

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@emersonknapp emersonknapp merged commit f5a5a48 into ros2:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants