-
Notifications
You must be signed in to change notification settings - Fork 12
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
Domain bridge container #32
Domain bridge container #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
Instead of adding a new executable, what do you think about making the existing executable a "container"? Then we get the existing CLI options without having to maintain two different executables. I can't really think of any downsides.
I agree, this makes sense for reducing code duplication. |
The changes lgtm, pending the decision in ros2/rclcpp#1702. We should document this feature (maybe a new section in the README) and some simple regression tests would be good too. |
Following the discussion in ros2/rclcpp#1702, this PR now contains a component manager specific to the domain bridge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a comment about why not to create one Context
per Node
, that's the only blocker for me.
About adding a SetNodeOptions()
method as suggested in this comment, I think it's okay but it looks a bit strange.
I really think that it would be okay to add a "domain_id" option to the component manager in rclcpp_components
. If the implementation guarantees only one Context
per domain it isn't an issue.
But both solutions are okay to me, I don't mind strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please add a brief section to the README demonstrating how someone could load a node into the container with a given domain ID.
We'll have to wait until the connected PR is merged and released before the CI and RPr jobs will pass. I don't think getting the upstream component manager change into Galactic is likely since it changes ABI (I think adding a new virtual function is not guaranteed to be ABI compatible). So, we'll have to diverge from Galactic (e.g. create a new |
👍 |
I've created a |
@ros-pull-request-builder retest this please |
1 similar comment
@ros-pull-request-builder retest this please |
I manually retriggered the GitHub workflow too. We can ignore the Gpr job since we've already updated rosdistro to point to the |
I guess the workflow may not use our testing repo for fetching Debians (or it's caching an older version of the package). I think we can ignore it and rely on the RPr job. |
There's an API change in ros2/rclcpp#1716 that affects this PR, so I think we should wait until that lands before merging. |
6f92b0d
to
c3afff1
Compare
I rebased and CI looks good now. @jacobperron could you give this one more look before we merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
It would be good to have some tests for this.
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
caafb0a
to
4e97e8f
Compare
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
@ivanpauno I added a test for this, could you take a look again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
install(TARGETS ${PROJECT_NAME}_test_component_lib | ||
EXPORT export_${PROJECT_NAME} | ||
ARCHIVE DESTINATION lib | ||
LIBRARY DESTINATION lib | ||
RUNTIME DESTINATION lib/${PROJECT_NAME} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing the test library and using the same export name as the primary domain_bridge library causes issues with linking in downstream packages. For instance, I ran into a case where the dependency test_msgs
could not be found when linking against the domain_bridge.
I don't think we need to install the library (or at the very least we can avoid exporting it).
See #61 for a fix.
This PR is related to ros2/rclcpp#1702.
Signed-off-by: Rebecca Butler rebecca@openrobotics.org