Skip to content

add more commands - #3

Merged
dirk-thomas merged 3 commits into
masterfrom
more_commands
Jun 16, 2017
Merged

dirk-thomas merged 3 commits into
masterfrom
more_commands

Conversation

@dirk-thomas

Copy link
Copy Markdown
Member

This PR adds some more commands:

  • Add the command ros2 msg with the verbs list, package, packages, and show
  • Add the command ros2 srv with the verbs list, package, packages, and show
  • Add the command ros2 run

The CI builds are again not really saying much (beside that the linter tests for the new code are passing):

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

Ready for review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Jun 14, 2017
@dirk-thomas dirk-thomas self-assigned this Jun 14, 2017
Comment thread ros2msg/package.xml Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is it on purpose that none of the new packages run the pep257 test ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, pep257 linting is provided by a flake8 plugin.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we ever installed this plugin because not available from debian packages (only via pip like import-order). Can't find any trace of it in the ci scripts either... Is there a PR I missed that was adding it ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should we install flake8-docstrings in the CI jobs then (as we do for flake8-import-order)? Or should we add pep257 tests explicitly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

flake8-docstrings is not available via apt so I'd lean towards using pep257 explicitely as we do everywhere else. But I don't feel very strongly about it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. I will add them for all packages in this repo.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I updated this PR and created #5 for the already merged packages.

@mikaelarguedas

Copy link
Copy Markdown
Member

That's pretty cool!

While testing a few things surprised me:

  • ros2 msg:
    • ros2 msg show expects "PackageName MessageName" but ros2 msg list gives me "PackageName/MessageName". So that means we cant copy paste the result of list to use show. It both differ from rosmsg in ros1 and is less convenient
  • ros2 run
    • run doesn't complete executable name so I need to know the package and the name to be able to run a node.
    • if I pick an executable it cannot find it:
$ros2 run examples_rclcpp_minimal_publisher examples_rclcpp_minimal_publisher_lambda
No executable found

Same terminal

$ examples_rclcpp_minimal_publisher_lambda
Publishing: [Hello, world! 0]
Publishing: [Hello, world! 1]

@dirk-thomas

Copy link
Copy Markdown
Member Author

ros2 run looks in the package specific libexec folder (<prefix>/share/lib/<pkgname>) for executables. Since non of our current packages installs any executables there you won't get any completion result.

Regarding ros2 msg: I usually prefer to pass information atomically. Otherwise e.g. your completion list for the combined argument would be extremely long. So I am not sure if we should change it to be the same as in ROS 1 or update the ros2 msg list output to print both parts (the package name and message name) separately.

@dirk-thomas

Copy link
Copy Markdown
Member Author

@mikaelarguedas mikaelarguedas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great! thanks foor iterating on this, lgtm

@mikaelarguedas

Copy link
Copy Markdown
Member

Note: I tried to install my executables in lib but couldn't get them to run No executable found. Also not sure how we should install ament_python packages now, but all this can be discussed/addressed after beta2

@dirk-thomas

dirk-thomas commented Jun 16, 2017

Copy link
Copy Markdown
Member Author

Did you install the executables in lib (which is the wrong location) or lib/<pkgname>? When I tried the code it worked for the later location.

@wjwwood

wjwwood commented Jun 16, 2017

Copy link
Copy Markdown
Member

I got it to work with this patch to demo_nodes_cpp:

diff --git a/demo_nodes_cpp/CMakeLists.txt b/demo_nodes_cpp/CMakeLists.txt
index 0e4f484..f6b0034 100644
--- a/demo_nodes_cpp/CMakeLists.txt
+++ b/demo_nodes_cpp/CMakeLists.txt
@@ -22,7 +22,7 @@ function(custom_executable subfolder target)
     "rcutils"
     "std_msgs")
   install(TARGETS ${target}
-    DESTINATION bin)
+    DESTINATION lib/${PROJECT_NAME})
 endfunction()

 # Tutorials of Publish/Subscribe with Topics

@wjwwood wjwwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, with one error case that could be handled better

Comment thread ros2run/ros2run/api/__init__.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if this is None?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I get this:

% ros2 run taco talker
Failed to load entry point 'service': No module named 'ros2service.command.service'
Traceback (most recent call last):
  File "/Users/william/ros2_ws/install/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli', 'console_scripts', 'ros2')()
  File "/Users/william/ros2_ws/build/ros2cli/ros2cli/cli.py", line 64, in main
    rc = extension.main(parser=parser, args=args)
  File "/Users/william/ros2_ws/build/ros2run/ros2run/command/run.py", line 50, in main
    executable_name=args.executable_name)
  File "/Users/william/ros2_ws/build/ros2run/ros2run/api/__init__.py", line 45, in get_executable_path
    paths = get_executable_paths(node=node, package_name=package_name)
  File "/Users/william/ros2_ws/build/ros2run/ros2run/api/__init__.py", line 30, in get_executable_paths
    base_path = os.path.join(prefix_path, 'lib', package_name)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/posixpath.py", line 78, in join
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed: 1232edf609ceaef883221a503ec35a34b6343731

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now it says No executable found, which to be more accurate, the package was not found. It would be better if it said that.

% ros2 run taco talker
No executable found

@dirk-thomas
dirk-thomas merged commit 2fc67d6 into master Jun 16, 2017
@dirk-thomas
dirk-thomas deleted the more_commands branch June 16, 2017 21:41
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 16, 2017
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.

3 participants