search

ros2 / rosidl_typesupport

Packages which provide the typesupport for ROS messages and services
Apache License 2.0
13 stars 34 forks source link

Make Poco optional again #54

Closed theseankelly closed 4 years ago

theseankelly commented 4 years ago

Poco library is only needed if multiple DDS implementations are to be supported. For 'static' builds with a single DDS in the sources, Poco can be omitted.

Closes #53

Signed-off-by: Sean Kelly sean@seankelly.dev

Please consider for backport to dashing

dirk-thomas commented 4 years ago

Thanks for the patch.

theseankelly commented 4 years ago

Thanks for the merge!

Karsten1987 commented 4 years ago

Could it be that this PR prohibits other RMW's to be loaded on top of a binary distribution? I've just noticed that I am unable to launch rmw_iceoryx_cpp on top of the latest eloquent binary distributions as I get fastrtps' typesupport passed in. I was assuming that additional RMW's can be added later on when using the introspection type support.

theseankelly commented 4 years ago

To the best of my understanding, this PR doesn't change any core functionality. If multiple typesupports are present at build time, the Poco library is used to dynamically load them. If a single typesupport is present, Poco is not used at all for rmw/typesupport loading.

This change allowed me to construct a customized ROS2 build containing a single RMW implementation and avoid including poco entirely. Without the change, poco would need to compile despite not being actually used.

So, while I'll certainly need @dirk-thomas or someone else to confirm the implications, my understanding is that this ought to not impact the ability to dynamically load RMW implementations.

theseankelly commented 4 years ago

Hey @dirk-thomas -- I'm working on porting my UWP project to Eloquent and found this commit didn't make it to Eloquent. Can it be backported?

Also note the comment @Karsten1987 which raises some concern this may have introduced a regression but I'm not sure whether that was ever chased down.

dirk-thomas commented 4 years ago

@mjcarroll This PR was merged before the Eloquent release but isn't part of the eloquent branch. I am not sure why that is the case? It should indeed be backported to Eloquent since it has even been backported to Dashing in Nov.

dirk-thomas commented 4 years ago

@Karsten1987 have you followed up on the problem you found and commented about above?

mjcarroll commented 4 years ago

@dirk-thomas I'm not sure how that happened, I'll make sure that it gets backported.