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

extract message / service / action files from test_msgs package #58

Closed
dirk-thomas opened this issue Jan 14, 2019 · 11 comments · Fixed by ros2/rosidl_python#89
Closed
Assignees
Labels
enhancement New feature or request in progress Actively being worked on (Kanban column)

Comments

@dirk-thomas
Copy link
Member

Currently many packages (mostly message generators) have their own test interface to check if the generator works as expected. But each package has an arbitrary subset. And with the amount of corner cases and variations it would be helpful if all these interface files exist in a single location. The message generator packages can't depend on test_msgs since that package itself depends on all message generators.

To avoid the duplication and get rid of the differences of these interface files I would like to propose to extract the actual interface files from the test_msgs package into a new separate package (and repository). I would propose the package / repo name test_interface_files. The test_msgs package would then be updated to still invoke the message generation for all these files - just not contain the interface files anymore but get them from the new package.

I am mostly looking for feedback on this before actually spending the time to create PRs for it.

@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Jan 14, 2019
@jacobperron
Copy link
Member

+1 for having a single place for test interface files.

It's unfortunate that we can't keep them alongside test_msgs though, since they are closely coupled. Maybe the two packages could be in the same repo for convenience? E.g. Move test_msgs out of this repo into another with test_interface_files.

@dirk-thomas
Copy link
Member Author

Maybe the two packages could be in the same repo for convenience? E.g. Move test_msgs out of this repo into another with test_interface_files.

That would introduce a repository level dependency cycle which I don't want to do.

@clalancette
Copy link
Contributor

Note that the individual rosidl generators also have their own .msg files to ensure generation happens properly (see https://github.com/ros2/rosidl/tree/master/rosidl_generator_cpp/msg and https://github.com/ros2/rosidl_python/tree/master/rosidl_generator_py/msg). It may make sense to consolidate these into test_interface_files as well.

@dirk-thomas
Copy link
Member Author

dirk-thomas commented Jan 15, 2019

Note that the individual rosidl generators also have their own .msg files to ensure generation happens properly

These are the duplicates I was referring to.

@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jan 31, 2019
@dirk-thomas dirk-thomas added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 9, 2019
@dirk-thomas
Copy link
Member Author

Not working on this until after the IDL changes have landed.

@jacobperron
Copy link
Member

One issue with this approach is that it won't be able to capture tests involving nested types from different packages (e.g. NestedMessage.action).

@jacobperron
Copy link
Member

@dirk-thomas I've built on top of your changes and opened a PR for the new package test_interface_files ros2/test_interface_files#1.

@jacobperron jacobperron added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed ready Work is about to start (Kanban column) in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) labels Apr 19, 2019
@jacobperron
Copy link
Member

jacobperron commented Apr 24, 2019

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 24, 2019
@dirk-thomas
Copy link
Member Author

This work will conflict with the ongoing work on WString support (ros2/rosidl#352). I just wanted to mentioned it early that we don't run into problems last minute before the API freeze date and try to land both at the same time.

@jacobperron
Copy link
Member

Thanks for the heads up. This ticket is lower priority and can be done piecemeal (ie, we can update affected packages independently), so I can give the WString support right of way.

@jacobperron
Copy link
Member

Another potential conflict: #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress Actively being worked on (Kanban column)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants