search

ros2 / rosbag2_bag_v2

rosbag2 plugin for replaying ros1 version2 bag files
Apache License 2.0
24 stars 4 forks source link

Remove blanket dependency on boost? #24

Open mikaelarguedas opened 4 years ago

mikaelarguedas commented 4 years ago

This package was flagged as one of the only ROS 2 packages using the blanket boost rosdep key.

This looks less sed-able than the other packages as this is used in some diffs that I don't know where they're applied.

Is it possible to audit if / how this dependency can be replaced with more specific ones ?

Some context and examples at: specific boost keys: https://github.com/ros/rosdistro/pull/23624 example of use: https://github.com/ros-drivers/transport_drivers/pull/10 DIscourse discussion about more precise dependency definition: https://discourse.ros.org/t/generating-dev-and-runtime-artefacts-from-ros-packages/12448

emersonknapp commented 4 years ago

Definitely agree that boost is an unecessarily large dependency. It's awkward in this case because this dependency is coming from building ros_comm's rosbag_storage. There seems to be a pretty heavy legacy dependency on many parts of boost in there - e.g. it's got boost::function and boost::shared_ptr going on at a quick glance, which are now available as stdlib concepts.

It might be possible to apply more patches to remove parts of boost in favor of stlib. Also might be possible to narrow down the dependency. Also maybe a branch of ros_comm for this build would be in order, once the noetic API is finalized, to make it easier to build here. Just wanted to add this context of what it's being used for.

mikaelarguedas commented 4 years ago

Maybe this PR will help https://github.com/ros/ros_comm/pull/1871/

Karsten1987 commented 4 years ago

I am closing this as we only implicitly depend on boost through ros_comm.

mikaelarguedas commented 4 years ago

@Karsten1987 does this mean that this explicit boost dependency https://github.com/ros2/rosbag2_bag_v2/blob/df3b04cdf6db4056e2c4a5d0bc957696479411f9/ros1_rosbag_storage_vendor/package.xml#L13 can be removed and that all the necessary dependencies will be exported by ros_comm ?

Karsten1987 commented 4 years ago

Hmm, I might have misunderstood your original intention for this ticket then.

What I meant is that I don't think we can easily get rid of this as we implicitly get the boost dependency from ros_comm. Note, we are currently using the melodic version of (ros1, ros_comm) rosbag here which brings boost with it.

I am personally not working on changing this to noetic, but I am happy to review any PRs for it.

EDIT: Maybe we can change the title of this ticket to make it more explicit that a change to rosbag noetic would help?

mikaelarguedas commented 4 years ago

I believe all the changes in rosbag have been made (https://github.com/ros2/rosbag2_bag_v2/issues/24#issuecomment-589939756)

As per a PR. I see master is v0.0.9 and foxy is 0.0.10. Should the PR target the foxy branch or master (which one is targetting noetic?) ?

Karsten1987 commented 4 years ago

I am not sure how I feel about backporting such a change to foxy. Please feel free to open a PR on master and we'll release it either into rolling or galactic at some point.

mikaelarguedas commented 4 years ago

IIUC Galactic wont have any overlap with ROS 1 distros. maybe its not worth spending the effort to fix this then.

dirk-thomas commented 4 years ago

Galactic wont have any overlap with ROS 1 distros.

Galactic will still target Ubuntu 20.04 and therefore overlap with ROS 1 Noetic.

mikaelarguedas commented 4 years ago

You're absolutely right, I was off by one for some reason