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.

Sign up for GitHub

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

Jump to bottom

Improve ros2 component CLI #226

Merged
merged 5 commits into from Apr 30, 2019
Merged

Improve ros2 component CLI #226

merged 5 commits into from Apr 30, 2019

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Apr 24, 2019 •
edited

Fixes #225. This pull request adds a run verb to start a component on a standalone container, for instance:

ros2 component run composition composition::Talker -n my_talker

But it also:

  • Adds --quiet flag to existing verbs to ease shell scripting.
  • Adds missing dependencies to the package.xml
  • Mildly refactors the internal package API

@hidmic hidmic requested a review from mjcarroll April 24, 2019 16:18
@hidmic hidmic added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 24, 2019
@hidmic
Copy link
Contributor Author

hidmic commented Apr 24, 2019 •
edited

@mjcarroll incidentally, I think I found a bug in the composable nodes implementation. If you execute:

ros2 component run composition composition::Talker

The composed node is given the same name as that of the container, which is being passed upon container execution via __node:=standalone_container_<unique 12 chars long hex>. It seems that upon loading, container remapping rules like __node affect the components as well.

@hidmic hidmic requested a review from dirk-thomas April 24, 2019 16:23
@hidmic
Copy link
Contributor Author

hidmic commented Apr 24, 2019 •
edited

CI (build up to and test only ros2component):

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

dirk-thomas
dirk-thomas reviewed Apr 25, 2019
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

This pull request adds a run verb to start a component on a standalone container ...
But it also:

It would be much easier to read if these unrelated changes would be separate PRs (or at least separate commits). I don't see a reason why they should be bundled in a single commit?

To distinguish run more clearly from load how about naming it standalone instead?

ros2component/package.xml Outdated Show resolved Hide resolved
ros2component/ros2component/verb/list.py Outdated
@@ -59,7 +52,7 @@ def main(self, *, args):
node=node, remote_container_node_name=args.container_node_name
)
if any(components):
print(*['{} {}'.format(c.uid, c.name) for c in components], sep='\n')
print(*['{} {}'.format(c.uid, c.name) for c in components], sep='\n')
Copy link
Member

Choose a reason for hiding this comment

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

What motivates the change in indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get closer to a conventionally 4 spaces wide tabulation, but otherwise as arbitrary as the former 2 spaces.

My intent is to be consistent about indentation while at the same time making it easy to use for someone that's shell scripting. Any thoughts on usability in that regard are welcome

Copy link
Member

Choose a reason for hiding this comment

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

Since other verbs use 2 spaces for indentation I would rather keep it with 2 for now.

Copy link
Member

Choose a reason for hiding this comment

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

Still pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, fair enough.

ros2component/ros2component/verb/run.py Outdated Show resolved Hide resolved
@hidmic
Refactor ros2component internal API for reuse.
857c594 
In preparation for a standalone verb.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add ros2component standalone verb.
f866843 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Clean up ros2component dependencies.
c435cf1 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Improve ros2component CLI verbs usability.
9b300d8 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/ros2component-improvements branch from 07bb66c to 9b300d8 Compare April 25, 2019 16:28
@hidmic
Copy link
Contributor Author

hidmic commented Apr 25, 2019

It would be much easier to read if these unrelated changes would be separate PRs (or at least separate commits). I don't see a reason why they should be bundled in a single commit?

No technical reason, just to avoid having to git plumb it.

To distinguish run more clearly from load how about naming it standalone instead?

So I subjectively prefer ros2 component run over ros2 component standalone (thus why I didn't do it that way initially) but it's not a strong preference. Changed.

@hidmic
Copy link
Contributor Author

hidmic commented Apr 29, 2019

@mjcarroll @dirk-thomas mind to review?

dirk-thomas
dirk-thomas reviewed Apr 29, 2019
View reviewed changes
ros2component/ros2component/api/__init__.py Show resolved Hide resolved
ros2component/ros2component/api/__init__.py Outdated Show resolved Hide resolved
ros2component/ros2component/verb/list.py Outdated
@@ -59,7 +52,7 @@ def main(self, *, args):
node=node, remote_container_node_name=args.container_node_name
)
if any(components):
print(*['{} {}'.format(c.uid, c.name) for c in components], sep='\n')
print(*['{} {}'.format(c.uid, c.name) for c in components], sep='\n')
Copy link
Member

Choose a reason for hiding this comment

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

Still pending.

ros2component/ros2component/verb/list.py Outdated Show resolved Hide resolved
ros2component/ros2component/verb/load.py Outdated Show resolved Hide resolved
ros2component/ros2component/verb/load.py Outdated Show resolved Hide resolved
@hidmic
Address peer review comments.
d5d3e32 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit d98350c into master Apr 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/ros2component-improvements branch April 30, 2019 21:09
@hidmic hidmic removed the in review Waiting for review (Kanban column) label Apr 30, 2019
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
@clalancette
Add a package marker to test_launch_ros. (ros2#226)
ab52d30 
This quiets the warning from colcon about it not explicitly
installing a marker.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ros2component verb to start standalone component
2 participants
@hidmic @dirk-thomas