search

ros-tooling / rosbag2_storage_mcap

rosbag2 storage implementation for MCAP file format
https://mcap.dev/
Apache License 2.0
32 stars 5 forks source link

Switch to using the vendored zstd library. #59

Closed clalancette closed 1 year ago

clalancette commented 1 year ago

Depends on:

We already have it vendored in the core as necessary.

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

Note that this is currently targeting the emersonknapp/set-read-order branch, because it won't build without that against the latest Rolling code. Once that one is merged I'll go ahead and rebase/retarget this to main.

Also note that I only tested this out on Rolling so far, but this is the kind of thing I think we should do. @MichaelOrlov FYI.

MichaelOrlov commented 1 year ago

@clalancette I rebased your branch on top of latest main and re-targeted it, since #54 has been already merged. It looks like indeed ZSTD_compress2() API mentioned by @jhurliman in https://github.com/ros2/rosbag2/pull/1118#issuecomment-1278111552 present in old zstd 1.4.4 version and rosbag2_mcap_storage plugin correctly compile and working with it. To make sure that this is true I've added can_write_mcap_with_zstd_configured_from_yaml unit test which is using storage yaml config file to force using zstd compression. I even walked through in Debugger to make sure that ZSTD_compress2() has been correctly called.

@jhurliman and @james-rms It seems nothing blocks us from moving forward and merge this PR to be able to use zstd_vendor package even with older 1.4.4 version. Any concerns about it?

MichaelOrlov commented 1 year ago

Gist: https://gist.githubusercontent.com/MichaelOrlov/f5d47c37320c4c60a213a11c5b1035c1/raw/778a5ec3f21820aea7ec54eb81e65406d6fbeed1/ros2.repos BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap TEST args: --packages-above rosbag2_storage_mcap ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10997

clalancette commented 1 year ago

@jhurliman and @james-rms It seems nothing blocks us from moving forward and merge this PR to be able to use zstd_vendor package even with older 1.4.4 version.

I do think that it would be a good idea for us to update zstd_vendor to 1.4.8; that will make it match what is in Ubuntu 22.04. But I leave it to you whether you want to do that.

MichaelOrlov commented 1 year ago

@clalancette CI builds fails with error messages

-- Build files have been written to: /home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor
[ 16%] Building CXX object CMakeFiles/mcap.dir/src/main.cpp.o
In file included from /home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/reader.hpp:708,
                 from /home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/mcap.hpp:3,
                 from /home/jenkins-agent/workspace/ci_linux/ws/src/ros-tooling/rosbag2_storage_mcap/mcap_vendor/src/main.cpp:16:
/home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/reader.inl:8:10: fatal error: zstd_errors.h: No such file or directory
    8 | #include <zstd_errors.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.
gmake[2]: *** [CMakeFiles/mcap.dir/build.make:76: CMakeFiles/mcap.dir/src/main.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:137: CMakeFiles/mcap.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2

It seems some problem with finding header files from zstd library. In particular zstd_errors.h Although I can build it from scratch with no errors on my local machine.

MichaelOrlov commented 1 year ago

On my local machine zstd_errors.h was taken from /usr/include/zstd_errors.h

clalancette commented 1 year ago

It seems some problem with finding header files from zstd library. In particular zstd_errors.h

So, there are 2 different things going on here.

The first thing is that https://ci.ros2.org doesn't use rosdep to install dependencies (yet). So that means that we are building zstd from source during the build of zstd_vendor currently. We can and should fix that, but only after the second thing.

The second problem seems to be in finding the headers that a built-from-source zstd_vendor installed. That should work, so I'm not quite sure why this package can't find them. One thing to try would be to replace:

target_link_libraries(mcap PRIVATE zstd::zstd)

with

ament_target_dependencies(mcap zstd)

And see if that is any different.

MichaelOrlov commented 1 year ago

@clalancette Thanks for the details and suggestion.

Will see if CI will pass green with it

MichaelOrlov commented 1 year ago

@clalancette CI fails again with the same error. Which is fairly strange because it compiles with no errors on my local machine in ether way. I've even tried to change FORCE_BUILD_VENDOR_PKG to ON https://github.com/ros2/rosbag2/blob/5b8b658b1f70a9f6184e4f6f31a42d50267f0b47/zstd_vendor/CMakeLists.txt#L9-L11 in attempt to reproduce CI failure locally.

I have no idea why it can't find zstd header on CI. I need someone help with this issue.

MichaelOrlov commented 1 year ago

@clalancette I've figured out why it fails on CI. It fails due to the patch https://github.com/ros2/rosbag2/blob/5b8b658b1f70a9f6184e4f6f31a42d50267f0b47/zstd_vendor/no_internal_headers.patch#L22 where we explicitly removing zstd_errors.h from installation. We applying this patch in https://github.com/ros2/rosbag2/blob/5b8b658b1f70a9f6184e4f6f31a42d50267f0b47/zstd_vendor/CMakeLists.txt#L55-L56

The proper fix would be to remove this patch, or upgrade to the 1.4.8 and remove this patch.

I created PR for bumping zstd to the 1.4.8

jhurliman commented 1 year ago

cc @emersonknapp RE zstd_errors.h

MichaelOrlov commented 1 year ago

Re-run CI Linux after updating zstd_vendor package

MichaelOrlov commented 1 year ago

It seems CI failure has been fixed as I expected. Re-running Linux-aarch64 and Windows CI jobs:

MichaelOrlov commented 1 year ago

Failure on Windows build is unrelated to this PR and it seems exists on baseline.

Creating library C:/ci/ws/build/rmw_connextdds_common/Release/rmw_connextdds_common_pro.lib and object C:/ci/ws/build/rmw_connextdds_common/Release/rmw_connextdds_common_pro.exp
CVTRES : fatal error CVT1107: 'C:\Program Files\rti_connext_dds-6.0.1\lib\x64Win64VS2017\nddscored.lib' is corrupt [C:\ci\ws\build\rmw_connextdds_common\rmw_connextdds_common_pro.vcxproj]
LINK : fatal error LNK1123: failure during conversion to COFF: file invalid or corrupt [C:\ci\ws\build\rmw_connextdds_common\rmw_connextdds_common_pro.vcxproj]
---
Failed   <<< rmw_connextdds_common [47.2s, exited with code 1]

@jhurliman @james-rms @emersonknapp Wouldn't you mind to review and approve this PR if you don't have any concern?

clalancette commented 1 year ago

Failure on Windows build is unrelated to this PR and it seems exists on baseline.

Sigh. Our Windows workers are just unstable right now, and we haven't been able to figure out why. I'd suggest running a couple of more times, you should be able to get through it eventually. That said, this isn't (yet) a core package, so you can go ahead without it if you want.

MichaelOrlov commented 1 year ago

Re-run Windows build one more time.

clalancette commented 1 year ago

So that last Windows run showed a real failure:

00:31:16.156 c:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(382,5): error MSB3491: Could not write lines to file "rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_c.dir\Release\rosbag2_.CFB5D6E9.tlog\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_c.lastbuildstate". Path: rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_c.dir\Release\rosbag2_.CFB5D6E9.tlog\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_c.lastbuildstate exceeds the OS max path limit. The fully qualified file name must be less than 260 characters. [C:\ci\ws\build\rosbag2_storage_mcap_test_fixture_interfaces\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_c.vcxproj]

I'm not sure why that didn't fail before, but it is definitely a problem. I'll suggest renaming the rosbag2_storage_mcap_test_fixture_interfaces package to something shorter, maybe rosbag2_storage_mcap_test_interfaces to work around it.

MichaelOrlov commented 1 year ago

@clalancette This failure on Windows CI build actually a known issue https://github.com/ros-tooling/rosbag2_storage_mcap/issues/60. Last time @emersonknapp came across on it. We've tried to fix it but it turns out to be non-trivial, https://github.com/ros-tooling/rosbag2_storage_mcap/pull/54#issuecomment-1276821694.

I think rosbag2_storage_mcap_test_fixture_interfaces package should be renamed in something with shorter name in the scope of the https://github.com/ros-tooling/rosbag2_storage_mcap/issues/60