Switch to using the vendored zstd library.

[clalancette]

Depends on:

• Bump zstd to 1.4.8 in zstd_vendor package ros2/rosbag2#1132
• Add set_read_order reader API #54

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]

@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 ros2/rosbag2#1118 (comment) 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]

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

• Linux
• Linux-aarch64
• Windows

[clalancette]

@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]

@clalancette CI builds fails with error messages

-- Build files have been written to: /home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor
[ 16%] �[32mBuilding CXX object CMakeFiles/mcap.dir/src/main.cpp.o�[0m
In file included from �[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/reader.hpp:708�[m�[K,
                 from �[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/mcap.hpp:3�[m�[K,
                 from �[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros-tooling/rosbag2_storage_mcap/mcap_vendor/src/main.cpp:16�[m�[K:
�[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/build/mcap_vendor/_deps/mcap-src/cpp/mcap/include/mcap/reader.inl:8:10:�[m�[K �[01;31m�[Kfatal error: �[m�[Kzstd_errors.h: No such file or directory
    8 | #include �[01;31m�[K<zstd_errors.h>�[m�[K
      |          �[01;31m�[K^~~~~~~~~~~~~~~�[m�[K
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]

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

[clalancette]

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]

@clalancette Thanks for the details and suggestion.

• I replaced target_link_libraries(mcap PRIVATE zstd::zstd) with ament_target_dependencies(mcap zstd).
• Also I have to add ament_export_dependencies(zstd_vendor zstd) otherwise rosbag2_storage_mcap will fail to compile.

Will see if CI will pass green with it

• Linux

[MichaelOrlov]

@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]

@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

• Bump zstd to 1.4.8 in zstd_vendor package ros2/rosbag2#1132

[jhurliman]

cc @emersonknapp RE zstd_errors.h

[MichaelOrlov]

Re-run CI Linux after updating zstd_vendor package

• Linux

[MichaelOrlov]

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

• Linux-aarch64
• Windows

[MichaelOrlov]

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]

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]

Re-run Windows build one more time.

• Windows

[clalancette]

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]

@clalancette This failure on Windows CI build actually a known issue #60.
Last time @emersonknapp came across on it. We've tried to fix it but it turns out to be non-trivial, #54 (comment).

I think rosbag2_storage_mcap_test_fixture_interfaces package should be renamed in something with shorter name in the scope of the #60