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

Remove RMW implementations from vendor typesupport dependencies #587

Merged
merged 4 commits into from
May 4, 2020

Conversation

jacobperron
Copy link
Contributor

They are not needed from Foxy onwards. The requirement was removed in ros2/rosidl_typesupport#62.

I'm not sure of the best way to test this change, confirming that they are indeed not actually needed. I've bloomed rmw_dds_common with this change, which was blocked previously because of a circular dependency with rmw_fastrtps_cpp: ros/rosdistro#24704

Looking at the resultant deb, it looks like the expected typesupport libraries are installed:

librmw_dds_common.so
librmw_dds_common__python.so
librmw_dds_common__rosidl_generator_c.so
librmw_dds_common__rosidl_typesupport_c.so
librmw_dds_common__rosidl_typesupport_connext_c.so
librmw_dds_common__rosidl_typesupport_connext_cpp.so
librmw_dds_common__rosidl_typesupport_cpp.so
librmw_dds_common__rosidl_typesupport_introspection_c.so
librmw_dds_common__rosidl_typesupport_introspection_cpp.so

They are not needed from Foxy onwards. The requirement was removed in ros2/rosidl_typesupport#62.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@cottsay
Copy link
Member

cottsay commented May 4, 2020

I think this same change applies to the RPM generator:

ROS2_VENDOR_TYPESUPPORT_DEPENDENCIES = [
'rmw-fastrtps-cpp',
'rmw-implementation',
'rmw-opensplice-cpp',
'rosidl-typesupport-opensplice-c',
'rosidl-typesupport-opensplice-cpp',
]

@jacobperron
Copy link
Contributor Author

err, and I also noticed that there is no typesupport for fastrtps. I guess it should also be there, and was being brought in transitively by rmw_fastrtps_cpp before.

@dirk-thomas
Copy link
Member

The current patch doesn't seem to be conditional on the ROS 2 distro (Foxy+). The behavior for Eloquent and earlier distros shouldn't be changed I assume.

@jacobperron
Copy link
Contributor Author

The current patch doesn't seem to be conditional on the ROS 2 distro (Foxy+). The behavior for Eloquent and earlier distros shouldn't be changed I assume.

Good catch, I mistaken thought this block was Foxy only 🙃

@nuclearsandwich
Copy link
Contributor

I think this same change applies to the RPM generator

I made these out-of-sync in #586 by omission as well.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@nuclearsandwich
Copy link
Contributor

Good catch, I mistaken thought this block was Foxy only upside_down_face

If you add the rmws back in the list here they'll only be included for Bouncy-Eloquent.

# OpenSplice was dropped after Eloquent.
if self.rosdistro in ['bouncy', 'crystal', 'dashing', 'eloquent']:
ROS2_VENDOR_TYPESUPPORT_DEPENDENCIES.extend([
'rmw-opensplice-cpp',
'rosidl-typesupport-opensplice-c',
'rosidl-typesupport-opensplice-cpp',
])

@jacobperron
Copy link
Contributor Author

Should I make the changes to the RPM generator here or in a separate PR?

@nuclearsandwich
Copy link
Contributor

Should I make the changes to the RPM generator here or in a separate PR?

If you'd make them match the rosdebian behavior here that would be preferred. I'm trying to dredge up an old private chat I had with @cottsay about implementing some shared logic but let's not block on that. If I find it I'll follow up with a future PR.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@nuclearsandwich nuclearsandwich merged commit f239970 into master May 4, 2020
@nuclearsandwich nuclearsandwich deleted the jacob/rm_rmw_inject branch May 4, 2020 19:39
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.

4 participants