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

Export lz4 and zstd dependencies for mcap_vendor #56

Closed MichaelOrlov closed 1 year ago

MichaelOrlov commented 2 years ago

Exporting dependencies from header files from lz4 and zstd libraries. Note: It turns out that on my local machine include forzstd.h was taken from local usr/include/zstd.h instead of the downloaded version specified in mcap_vendor package. I have to export zstd header files the same way as lz4 since CI was failing. It also will solve some potential hidden compilation errors in future if our dependent zstd library will diverge from installed by default with Linux distro.

MichaelOrlov commented 2 years ago

@jhurliman @emersonknapp Can we come to some compromise solution? I removed definition for MCAP_IMPLEMENTATION from mcap_vendor main.cpp. Currently it's defined only in one place in mcap_storage.cpp. If someone else will need to use mcap_vendor package outside of the rosbag2 they will need to make the same define for MCAP_IMPLEMENTATION in their code. I recognize that it's a bit less straight forward solution rather then just include mcap_vendor as dependencies. I can add readme file with description about it. At the end it's no more than just like one more include. I mean those one who will be using mcap via mcap_vendor they will need to be aware what header file to include and it will be one more step to do to add those definition.

Currently no one using mcap_vendor package among us in Rosbag2 and it's not going to be a hassle.

As regards to a potential issue if someone will start using mcap_vendor in some another project and will need zstd or lz4 with different version we can get back to this issue and either bump versions for those libs in mcap or will try to find some another workaround. In worst case scenario we can revert this commit.

clalancette commented 2 years ago

Note: It turns out that on my local machine include forzstd.h was taken from local usr/include/zstd.h instead of the downloaded version specified in mcap_vendor package.

I don't have all of the context here, but I do think MCAP should use the system zstd (and lz4) when they are available. Not doing this can cause problems later on (mostly potentially nasty ABI issues). It's the pattern we follow almost everywhere else, with a few minor exceptions.

So with that said, I think the fact that this vendor package pulls a few files from zstd and lz4 and compiles them is not the right way to go. Instead, we should have proper vendor packages (and we don't even need one for zstd, we have that already).

jtbandes commented 2 years ago

Linking some prior discussion on vendor setup for reference:

MichaelOrlov commented 2 years ago

One more point towards my proposed solution is that if someone will need to use mcap format in ROS2 ecosystem they will likely be using rosbag2_cpp API or rosbag2_storage_mcap plugin rather then mcap_vendor package directly. Please note that mcap_vendor is internal sub package in rosbag2_storage_mcap package and was created to serve it.

@clalancette As regards that mcap should be using the system zstd and lz4 when they are available and from specific vendor packages. We've tried to consider this opportunity on today's WG meeting and figured out that it's not possible currently at least for zstd. Because mcap itself depends from zstd API available in version 1.5.2, but by default in Ubuntu 20.04 is 1.4.4 version. It means that users with Ubuntu 20.04 will not be able to compile mcap_vendor package if we will switch to the current zstd_vendor package and even if we will bump zstd version to the 1.5.2 in it.

MichaelOrlov commented 2 years ago

After some consideration I tend to incline to close this PR and relevant issue as will not done. The rationale for that is that it is not good to export lz4 and zstd dependencies since it could cause some ABI issues in future and in case if someone will start using mcap_vendor in some another project and will need zstd or lz4 with different version.

@clalancette I figured out how to compile mcap_vendor package with zstd_vendor package as dependencies without pulling zstd from git repo and compiling it from source. Perhaps we will be able to do the same with lz4. Long story short it will require fix inside mcap cpp implementation and multiple adjustments in mcap_vendor cmake file. There are one caveat that we will need to bump zstd version in zstd_vendor to the 1.5.2. This is the latest version of the zstd version and not available by default in Ubuntu 22.04. We still can have the rule to use locally installed version without mandatory downloading it. But the problem is that it's going to cause downloading it since this 1.5.2 version is not available yet by default. In my Ubuntu 22.04 installation zstd with version 1.4.8.

Is it ok to go with this approach and bump zstd version to 1.5.2?

amacneil commented 2 years ago

What's the reason that mcap depends on such a new version of zstd? Can it be updated to support older versions?

clalancette commented 2 years ago

What's the reason that mcap depends on such a new version of zstd? Can it be updated to support older versions?

We've been trying to reduce the number of vendored packages that we actually build[1] on our primary platform, Ubuntu 22.04. This reduces compile times and integrates much more nicely with the base system.

Thus, if there is any way we can make mcap use zstd 0.4.4 (the version current in Ubuntu 22.04), that would be ideal.

[1] In the interest of full disclosure, we aren't there yet. We unconditionally vendor at least OGRE and libyaml currently, but ideally we'd make both of those conditional as well. Most of our other vendored packages are conditional.

MichaelOrlov commented 1 year ago

@clalancette @amacneil I re-posted our discussion in newly created https://github.com/ros2/rosbag2/pull/1118#issuecomment-1273951249 Please continue discussion there.