Install msgs and fixtures for use by other packages by gerkey · Pull Request #190 · ros2/system_tests

gerkey · 2017-03-10T21:15:45Z

When implementing tests for rostopic_echo_py (https://github.com/ros2/cli_tools/pull/5), I had need of a bunch of messages to push through the YAML and CSV converters. This PR installs the messages and related fixtures so that they can be used in testing other packages.

http://ci.ros2.org/job/ci_linux/2314/
http://ci.ros2.org/job/ci_osx/1769/
http://ci.ros2.org/job/ci_windows/2343/

mikaelarguedas

LGTM 👍 .
As discussed offline , this is an first step toward creating a message package purely for testing purposes that depends only on the generators and that any package downstream will be able to use for testing.

dirk-thomas · 2017-03-10T21:30:45Z

These packages currently only contain tests. From a packaging point of view they don't even have to be released.

Therefore I don't think other packages should start depending on this package. Instead the messages should be extracted into a separate package which the cli tools as well as the tests can then depend on.

wjwwood

Other than a comment about the dependencies being changed, it lgtm.

I do agree with @dirk-thomas though that it would be better to have the messages in a separate package. @mikaelarguedas is there any reason not to just do that? I know in the past there were issues with the type support and message packages, but I think that might be resolved now?

wjwwood · 2017-03-10T21:43:55Z

test_communication/package.xml

+  <build_depend>builtin_interfaces</build_depend>
+  <build_depend>rclcpp</build_depend>
+  <build_depend>rclpy</build_depend>
+  <build_depend>rcl</build_depend>


(forgot to actually submit this comment with the review, sorry)

Do these (rcl*) packages need to be build depends? I think they could stay as test depends.

I think that they should be build depends, so that the call to ament_auto_find_build_dependencies() will have the side effect of causing the subsequent call to rosidl_generate_interfaces() to generate for all the languages.

But I could be very wrong; I've honestly never understood how to use the various _depend tags or how they interact with what happens in CMake :)

I think that @wjwwood is right. You need the generators (rosidl_default_generators) to build the messages but not the client libraries. The client libraries can be used to interact with messages but are not required to generate/build them.

The generated type support triggered by rosidl_generate_interfaces doesn't depend on the client libraries rcl*. The only build dependency necessary for that is rosidl_default_generators.

Also rosidl_default_runtime should be a run dependency.

a2e014c reshuffles the depend tags as suggested, and the desired code generation still works. I learn something new every day.

mikaelarguedas · 2017-03-10T21:58:29Z

I don't see any blocker for doing it.
There are 2 points we should agree on before moving on:

Where should this package land? I think that rosidl should work
Should this package be IDL only or also provide fixtures for both C, C++ and Python?

Once this package created we should update all packages generating messages and services for testing to use those instead.
I was seeing this work as a follow up refactor rather than holding https://github.com/ros2/cli_tools/pull/5 on it. But I'm fine either way

wjwwood · 2017-03-10T22:13:09Z

Ok, I'm fine with doing this for now and planning to split it out into a different package then update everything else to use it. However, it would be nice to see that task on the next sprint if possible. I'd even be willing to take on the task, but I fear if we don't schedule it, then it won't get done.

Where should this package land? I think that rosidl should work

If you're talking about repositories, then yeah rosidl can work since all of the other generators that are currently explicitly depended on live there too.

Should this package be IDL only or also provide fixtures for both C, C++ and Python?

Are you talking about some extra code that helps you run tests over all the messages? I don't see a problem with that since the package will probably be called test_interfaces or something like that, then it's clear it's for testing purposes and so having extra test utilities live there sounds fine to me too.

dirk-thomas · 2017-03-10T22:16:41Z

Where should this package land? I think that rosidl should work

If you're talking about repositories, then yeah rosidl can work since all of the other generators that are currently explicitly depended on live there too.

It can't be in rosidl. Every message package depends on rosidl_default_generators and rosidl_default_runtime which are in rosidl_typesupport. So putting them there would result in a circular dependency in repository level.

I would suggest either common_interfaces or a new repo.

wjwwood · 2017-03-10T22:19:28Z

That's true. common_interfaces sounds like a better idea. I'm also ok with a new repository.

mikaelarguedas · 2017-03-10T22:22:27Z

Where should this package land? I think that rosidl should work

If you're talking about repositories, then yeah rosidl can work since all of the other generators that are currently explicitly depended on live there too.

It can't be in rosidl. Every message package depends on rosidl_default_generators and rosidl_default_runtime which are in rosidl_typesupport. So putting them there would result in a circular dependency in repository level.
I would suggest either common_interfaces or a new repo.

Good point, I'm fine with either of those options. The former avoiding to have a new repo just for that purpose, the latter allowing to keep common_interfaces closer to its ROS1 counterpart common_msgs.

However, it would be nice to see that task on the next sprint if possible.

Agreed, we talked about it for a while and never got around doing it. I don't mind taking it on either, it shouldn't take long.

dirk-thomas · 2017-03-10T22:58:21Z

test_communication/package.xml

+  <build_depend>ament_cmake_auto</build_depend>
+  <build_depend>builtin_interfaces</build_depend>
+  <build_depend>rmw_implementation</build_depend>
+  <build_depend>rmw_implementation_cmake</build_depend>


The rmw_implementation packages are only used for the tests and should therefore still be test dependencies.

gerkey · 2017-03-10T23:03:34Z

I ticketed myself to create the separate repo to hold the reusable stuff: #191.

dirk-thomas · 2017-03-10T23:05:58Z

test_communication/package.xml

-  <test_depend>rcl</test_depend>
  <test_depend>rclcpp</test_depend>
  <test_depend>rclpy</test_depend>
+  <test_depend>rcl</test_depend>


This is now in wrong order. It was correct before being moved.

dirk-thomas · 2017-03-10T23:06:55Z

test_communication/package.xml

+  <build_depend>builtin_interfaces</build_depend>
+  <build_depend>rosidl_default_generators</build_depend>
+
  <buildtool_depend>ament_cmake</buildtool_depend>


This dependency as well as the find_package(ament_cmake) call is redundant now.

Good to know. But why?

Because ament_cmake_auto depends on ament_cmake and exports that dependency (https://github.com/ament/ament_cmake/blob/7b31d859f3d42b9eb6c84db807d95a5604a6881b/ament_cmake_auto/ament_cmake_auto-extras.cmake#L17).

Then should I declare ament_cmake_auto as a build_depend or a buildtool_depend and why?

The package should then also call ament_auto_package (https://github.com/ament/ament_cmake/blob/7b31d859f3d42b9eb6c84db807d95a5604a6881b/ament_cmake_auto/cmake/ament_auto_package.cmake#L31) instead of ament_package.

A buildtool_depend dependency because you need the dependency for the host system where you are performing the build on (in case of a cross compilation).

What difference in behavior should I expect by calling ament_auto_package()? I'm not doubting the advice, but trying to understand why each thing is used in a different context. That's especially difficult to determine for these kinds of changes to the package.xml and CMakeLists.txt because everything seems to work fine with a wide variety of different combinations and I don't have the context to identify potential problems by inspection.

ament_cmake_auto aims to automate several steps in your CMake and you need to call its function in order to actually perform all those: see https://github.com/ament/ament_cmake/blob/master/ament_cmake_auto/cmake/ament_auto_package.cmake You either don't need any of these (than you probably want to use ament_cmake instead) or you haven't noticed that they are not happening.

dirk-thomas · 2017-03-10T23:07:34Z

test_communication/CMakeLists.txt


-# TODO should not install anything
+install(FILES test/__init__.py test/message_fixtures.py
+  DESTINATION "${PYTHON_INSTALL_DIR}/${PROJECT_NAME}")


What about the C++ fixtures?

they were not needed by https://github.com/ros2/cli_tools/pull/5 that motivated this PR in the first place. Fixtures for all languages will be moved in (and installed by) the package that will be created in #191

gerkey · 2017-03-11T00:17:31Z

#192 fixes the minor dependency and cmake usage items that @dirk-thomas raised.

Install msgs and fixtures for use by other packages

87edb26

gerkey 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 Mar 10, 2017

mikaelarguedas approved these changes Mar 10, 2017

View reviewed changes

wjwwood approved these changes Mar 10, 2017

View reviewed changes

wjwwood reviewed Mar 10, 2017

View reviewed changes

reshuffle depends

a2e014c

dirk-thomas reviewed Mar 10, 2017

View reviewed changes

gerkey mentioned this pull request Mar 10, 2017

Move messages and fixtures from test_communication into another repo #191

Closed

reshuffle depends

da226ac

gerkey merged commit 54127c9 into master Mar 10, 2017

gerkey deleted the install_msgs_and_fixtures branch March 10, 2017 23:04

gerkey removed the in review Waiting for review (Kanban column) label Mar 10, 2017

dirk-thomas reviewed Mar 10, 2017

View reviewed changes

gerkey mentioned this pull request Mar 11, 2017

Fix deps #192

Merged

Conversation

gerkey commented Mar 10, 2017

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Mar 10, 2017

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerkey Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Mar 10, 2017

Uh oh!

wjwwood commented Mar 10, 2017

Uh oh!

dirk-thomas commented Mar 10, 2017

Uh oh!

wjwwood commented Mar 10, 2017

Uh oh!

mikaelarguedas commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerkey commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerkey Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerkey commented Mar 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gerkey Mar 10, 2017 •

edited

Loading

gerkey Mar 11, 2017 •

edited

Loading