search

ros2 / rosbag2_bag_v2

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

Fix compiler error about virtual storage filter interface #28

Closed mabelzhang closed 4 years ago

mabelzhang commented 4 years ago

Also fixes compiler warnings for TopicMetadata initialization.

Signed-off-by: Mabel Zhang mabel@openrobotics.org

mabelzhang commented 4 years ago

I am not familiar with how to run CI on this repo, specifically, how to specify the ROS 1 dependencies. I see that the checks below already fail with roscpp not found.

piraka9011 commented 4 years ago

I am not familiar with how to run CI on this repo, specifically, how to specify the ROS 1 dependencies. I see that the checks below already fail with roscpp not found.

I don't think the build farm is setup to support building ROS1 packages and we've been trying to find a way to build ROS1/2 packages in the same workflow with GitHub actions but no luck.

mabelzhang commented 4 years ago

I see. What is the condition for merging in this repo?

emersonknapp commented 4 years ago

The build that I know of that includes ROS 1 are the "packaging" builds - e.g. https://ci.ros2.org/view/packaging/job/packaging_linux/1830/ - that's the only place ros1_bridge tests are run. maybe it makes sense to run one of these jobs to verify?

Karsten1987 commented 4 years ago

That's right, the packaging job linux is the ones which allows to specify an overlay (CI_MIXED_ROS_OVERLAY_PKGS). All the packages in there are built after the regular ROS2 packages are built and a ROS melodic setup is sourced.

Karsten1987 commented 4 years ago

The description should hopefully be helpful: A list of packages - separated by spaces - which explicitly have to be built after sourcing the ROS 1 environment. All packages listed here have to be available from either the primary or supplemental repos file.

Karsten1987 commented 4 years ago

EDIT: Nevermind, local issue only.

~I am getting the following error message:~

--- stderr: rosbag2_bag_v2_plugins
/home/karsten/ros2_ws/install/lib/libros1_rosbag_storage.so: undefined reference to `class_loader::systemLibrarySuffix[abi:cxx11]()'
mabelzhang commented 4 years ago

I finally figured out how to get a CI build that isn't red, but there are a lot of warnings outside of rosbag2_bag_v2_plugins? That is the package I changed.

Build Status

Karsten1987 commented 4 years ago

it looks to me as if the tests don't run. when trying them out locally I see that both end to end tests fail as well as uncrustify

[0.932s] 3: [ RUN      ] PlayEndToEndTestFixture.play_end_to_end_test_does_not_try_to_publish_ros1_topics
[1.517s] 3: Segmentation fault (core dumped)

That segfault is something I would like you to look into.

mabelzhang commented 4 years ago

The segmentation fault seems a deeper issue. It comes from a test that runs this line: $ ros2 bag play src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/resources/test_bag_end_to_end.bag -s rosbag_v2

I can reproduce the seg fault on the command line.

If I comment out all the lines in the new functions in this PR, i.e. leave the filter functions empty, and revert has_next() to the original line, the seg fault still happens for me. That seems to suggest that the problem was there before this PR - but since the master branch does not compile, I could not double-check that.

valgrind traceback:

==179== Thread 9:
==179== Invalid read of size 8
==179==    at 0x8D1DCAD: rosbag2_cpp::allocate_introspection_message(rosidl_message_type_support_t const*, rcutils_allocator_t const*) (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_cpp.so)
==179==    by 0x8CBD521: rosbag2_cpp::Converter::convert(std::shared_ptr<rosbag2_storage::SerializedBagMessage const>) (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_cpp.so)
==179==    by 0x8CC3E2A: rosbag2_cpp::readers::SequentialReader::read_next() (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_cpp.so)
==179==    by 0x8CC24EA: rosbag2_cpp::Reader::read_next() (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_cpp.so)
==179==    by 0x819A871: rosbag2_transport::Player::load_storage_content(rosbag2_transport::PlayOptions const&) (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_transport.so)
==179==    by 0x819A570: rosbag2_transport::Player::play(rosbag2_transport::PlayOptions const&)::{lambda()#1}::operator()() const (in /home/developer/colcon_ws_ros12/install/lib/librosbag2_transport.so)

For the segfault to happen outside of this package, it must be something returned from this package to the reader. But since I revert all the functions and leave the new ones stub, the behavior should be the same as before the PR.

I am wondering if there are any internal changes to rosbag2 in the last two months (Feb 18 was the last PR to this package) that might require other changes in this package? Or do the bag files in this package need updating - since the bag files in rosbag2_tests needed updating?

mabelzhang commented 4 years ago

As for the uncrustify errors, I saw those and wasn't sure if this package follows different style rules from other ROS 2 packages? Because there are quite a few lines with uncrustify errors in different files - lines I haven't touched, so I wasn't sure whether I should apply the reformatting?

Karsten1987 commented 4 years ago

With the referenced rosbag2 PR merged, this should be good to go: Build Status