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

Add Python API for RMW implementation lookups #73

Merged
merged 3 commits into from
Oct 15, 2019

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Oct 4, 2019

This pull request adds a basic Python API to lookup available RMW implementations in the system. Useful when parameterizing pytests by RMW implementation in pure Python packages i.e. no CMake templates available.

Opening PR as a draft for preliminary discussion on whether this functionality should live here or in a separate package of its own.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic changed the title Add Python API for RMW implementation lookups. Add Python API for RMW implementation lookups Oct 4, 2019
def get_available_rmw_implementations():
"""Return a list of all available RMW implementations as registered in the ament index."""
rmw_implementations = ament_index_python.get_resources('rmw_typesupport')
return [name for name, _ in rmw_implementations if name != 'rmw_implementation']
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried to run this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that's funny. I did, though not recently. Removed that extra , _ in 7a0309c.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
def get_available_rmw_implementations():
"""Return a list of all available RMW implementations as registered in the ament index."""
rmw_implementations = ament_index_python.get_resources('rmw_typesupport')
return [name for name in rmw_implementations if name != 'rmw_implementation']
Copy link
Member

Choose a reason for hiding this comment

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

How about returning a set here to not imply any order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds perfectly reasonable, see 5a7690a.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic marked this pull request as ready for review October 7, 2019 17:04
@mabelzhang mabelzhang added the enhancement New feature or request label Oct 10, 2019
@hidmic
Copy link
Contributor Author

hidmic commented Oct 15, 2019

Getting this one in to re-run CI on ros2/ros2cli#304.

@hidmic hidmic merged commit 07fcf4f into master Oct 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/rmw-implementation-python-api branch October 15, 2019 16:35
@wjwwood
Copy link
Member

wjwwood commented Dec 18, 2019

We should consider moving this to a different package. This change causes all of the client libraries to depend on Python indirectly, which probably isn't ideal... If it were a separate package, them packages like rcl or rclcpp could still depend on and use this python code, but they could do so via a build or buildtool depend (I'm pretty sure it's not being used by them at runtime). But since this is an exec depend and rmw_implementation is most often an exec depend (perhaps transitively) it causes a runtime dependency on ament_index_python and therefore Python itself.

@dirk-thomas
Copy link
Member

@wjwwood Can you please create a separate ticket for this. Otherwise the chance is high that your comment on a closed ticket will get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants