Update to the latest rosbag1.

ros2 / rosbag2_bag_v2

rosbag2 plugin for replaying ros1 version2 bag files

Apache License 2.0

24 stars 4 forks source link

Update to the latest rosbag1. #40

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

While looking through the packaging_linux jobs, I noticed that it always had a warning when building this package. It turns out that the particular warning that it was throwing was fixed long ago in ros_comm, but the vendored rosbag2 here never updated. This PR updates to the latest code on the noetic-devel branch (as of today), drops one patch which has already been applied upstream, and fixes the rest of the patches so that they apply cleanly.

With this in place, I was able to build locally without any warnings. However, there don't seem to be tests so I don't know how to test it. @emersonknapp @Karsten1987 any thoughts on what I should test here?

emersonknapp commented 3 years ago

However, there don't seem to be tests so I don't know how to test it

There are tests that happen if you run colcon test - I'm not super familiar with the contents, but I know they at least do bag playback of a ros1 bag.

clalancette commented 3 years ago

There are tests that happen if you run colcon test - I'm not super familiar with the contents, but I know they at least do bag playback of a ros1 bag.

Oh, you are right. I messed up my colcon test invocation completely. Thanks.

So I went ahead and ran the tests on the current master branch, and at least locally it fails the following tests:

test_rosbag_v2_storage
test_rosbag2_play_rosbag_v2_end_to_end
test_rosbag2_info_rosbag_v2_end_to_end

With this patch in place, the same tests fail. That isn't a great result, but I don't really have time right now to go and fix the already failing tests. I'm going to push another fix to this PR that I needed, and then kick off the ci_packaging job on it. I think that is the best I can do for now.

clalancette commented 3 years ago

Linux Packaging

emersonknapp commented 3 years ago

did you do

source /opt/ros/rolling/setup.bash
source /opt/ros/noetic/setup.bash
colcon build
colcon test

in that order? The tests are good and passing on the master branch right now under the correct setup

emersonknapp commented 3 years ago

the params were correct on the build but your ros2.repos gist was just missing rosbag2_bag_v2 repository

clalancette commented 3 years ago

did you do

source /opt/ros/rolling/setup.bash source /opt/ros/noetic/setup.bash colcon build colcon test

in that order? The tests are good and passing on the master branch right now under the correct setup

I believe so, but let me try it out again.

the params were correct on the build but your ros2.repos gist was just missing rosbag2_bag_v2 repository

Ah, thanks for kicking it off again with the right repos file.

emersonknapp commented 3 years ago

I'm looking to do a new bloom release into Rolling to get those builds succeeding - do you think we should hold off until we have this last compiler warning solved to do so?

clalancette commented 3 years ago

Here's a new CI:

clalancette commented 3 years ago

With CI now passing, @emersonknapp I'll leave it to you to re-review and merge when you are ready.

clalancette commented 3 years ago

Thanks!