[Foxy] rosbag2_storage_mcap: merge into rosbag2 repo (backport #1163)

[mergify]

This is an automatic backport of pull request #1189 done by Mergify.
Cherry-pick of 953c8ed has failed:

On branch mergify/bp/foxy/pr-1189
Your branch is up to date with 'origin/foxy'.

You are currently cherry-picking commit 953c8ed.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   .github/workflows/lint.yml
	new file:   mcap_vendor/CHANGELOG.rst
	new file:   mcap_vendor/CMakeLists.txt
	new file:   mcap_vendor/package.xml
	new file:   mcap_vendor/src/main.cpp
	new file:   rosbag2_storage_mcap/.clang-format
	new file:   rosbag2_storage_mcap/.vscode/settings.json
	new file:   rosbag2_storage_mcap/CHANGELOG.rst
	new file:   rosbag2_storage_mcap/CMakeLists.txt
	new file:   rosbag2_storage_mcap/CPPLINT.cfg
	new file:   rosbag2_storage_mcap/LICENSE
	new file:   rosbag2_storage_mcap/README.md
	new file:   rosbag2_storage_mcap/include/rosbag2_storage_mcap/message_definition_cache.hpp
	new file:   rosbag2_storage_mcap/include/rosbag2_storage_mcap/visibility_control.hpp
	new file:   rosbag2_storage_mcap/package.xml
	new file:   rosbag2_storage_mcap/plugin_description.xml
	new file:   rosbag2_storage_mcap/src/mcap_storage.cpp
	new file:   rosbag2_storage_mcap/src/message_definition_cache.cpp
	new file:   rosbag2_storage_mcap/test/rosbag2_storage_mcap/mcap_writer_options_zstd.yaml
	new file:   rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_mcap_storage.cpp
	new file:   rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp
	new file:   rosbag2_storage_mcap_testdata/CHANGELOG.rst
	new file:   rosbag2_storage_mcap_testdata/CMakeLists.txt
	new file:   rosbag2_storage_mcap_testdata/msg/BasicIdl.idl
	new file:   rosbag2_storage_mcap_testdata/msg/BasicMsg.msg
	new file:   rosbag2_storage_mcap_testdata/msg/ComplexIdl.idl
	new file:   rosbag2_storage_mcap_testdata/msg/ComplexMsg.msg
	new file:   rosbag2_storage_mcap_testdata/msg/ComplexMsgDependsOnIdl.msg
	new file:   rosbag2_storage_mcap_testdata/package.xml
	modified:   zstd_vendor/no_internal_headers.patch

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   .github/workflows/test.yml

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

• @Mergifyio refresh will re-evaluate the rules
• @Mergifyio rebase will rebase this PR on its base branch
• @Mergifyio update will merge the base branch into this PR
• @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

• look at your merge queues
• generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

[MichaelOrlov]

@james-rms API-ABI comatibility check fails with error message

In file included from /tmp/MswhE17sDo/dump1.h:39:
/tmp/tmpyspkvm0y/files/opt/ros/foxy/include/mcap_vendor/mcap/reader.inl:4:10: fatal error: lz4frame.h: No such file or directory
    4 | #include 
      |          ^~~~~~~~~~~~
compilation terminated.

[james-rms]

As I understand it the lz4frame.h being included here is a pinned dependency declared and fetched inside mcap_vendor/CMakeLists.txt - there should be no difference in lz4 version used between foxy and rolling.

This seems really odd to me that this is not an issue on rolling or humble - is this something about the version of the ABI checker being used here vs. anywhere else?

[MichaelOrlov]

As regards to the " is this something about the version of the ABI checker being used here vs. anywhere else?"
Heh, I am afraid that we have ABI checker only on Foxy branch.

[emersonknapp]

We could always turn if off for this branch. The unpleasant thing is that this didn't catch the incoming change being a problem. Only once the initial bad version was in, does it fail to compare it to a new version.

There's no REP-2004 quality declaration promising ABI compatibility for this package

[clalancette]

There's no REP-2004 quality declaration promising ABI compatibility for this package

But we do our very best to maintain ABI compatibility on all ROS 2 core packages (of which this is one). That was the whole reason for the foxy-future branch in this repository. I don't think we should go back on that promise now.

[emersonknapp]

@clalancette I'm mostly just lobbying for not letting difficult tooling get in the way. The way I understand it, no matter what happens now, we are going to have to override the ABI checker to get a new change in because it accepted a new thing that now it is not able to compare as "the old thing".

Do you have a pointer to a way to run the ABI checker locally? I remember going through this exercise once, but I can't find it, maybe it's in my old work email or some old github issue about zstd - I'm having a hard time finding it.

[clalancette]

These are the instructions I've always followed locally: https://lvc.github.io/abi-compliance-checker/

[james-rms]

I managed to get the ABI checker to not choke over here: #1207

My proposal is:

1. backport mcap_vendor: only install public headers #1207 to humble and cherry-pick it into this branch as well.
2. Disable the ABI checker in this branch, then merge it.
3. bloom-release and wait for sync in humble and foxy.
4. Once the sync is complete on both branches, enable the ABI checker for both foxy and humble.

[james-rms]

Thinking harder about this - Can we get more granular with the set of packages we plan to mark with REP-2004 QL 1 declarations? IMO it'd make sense to exclude mcap_vendor, zstd_vendor and others from ABI compatibility checking. It's not unreasonable for users to try to build against them directly, but I don't see a strong argument that those ABI/APIs should remain stable in the same sense that rosbag2_cpp APIs should.

[emersonknapp]

That plan seems fine to me - except I don't think we actually have to disable the checker. We can inspect the given build, say "everything is fine except for the issue with the vendor package in the ABI checker", then merge regardless of what that check says. Followup checks should then look good, which we can verify with a sample PR to the branch.

[clalancette]

I guess we can do that, though it makes things hard to explain to users.

That is, up until about 2 years ago, we just had a blanket "no ABI changes in the core to stable distributions". This is an easy thing to explain (though not always to achieve), and was something users could depend on. In the meantime, we've had various third-party components that want to carve out exceptions to this, so now the statement is "no ABI changes in the core to stable distributions, except for these certain parts of Fast-DDS, and these certain parts of CycloneDDS, and these certain parts of rosbag2, etc". I don't think this is a good situation to be in; it means that effectively users can't depend on that promise at all, because it is hard for them to detect ABI changes.

So I would really like to not expand that list, particularly in an old distribution like Foxy.

[MichaelOrlov]

@james-rms As regards to your plan suggestion.

• I hope that as soon as you will propagate and merge mcap_vendor: only install public headers #1207 on Humble and then rebase you current brunch on top of it, - ABI checker will pass. If not we still will be able to merge this PR with failing ABI checker if we assure that this is a false positive.
• However mcap_vendor: only install public headers #1207 on Humble should be first IMO.

[james-rms]

I guess we can do that, though it makes things hard to explain to users.

That is, up until about 2 years ago, we just had a blanket "no ABI changes in the core to stable distributions". This is an easy thing to explain (though not always to achieve), and was something users could depend on. In the meantime, we've had various third-party components that want to carve out exceptions to this, so now the statement is "no ABI changes in the core to stable distributions, except for these certain parts of Fast-DDS, and these certain parts of CycloneDDS, and these certain parts of rosbag2, etc". I don't think this is a good situation to be in; it means that effectively users can't depend on that promise at all, because it is hard for them to detect ABI changes.

So I would really like to not expand that list, particularly in an old distribution like Foxy.

OK, I have a few options we can choose from here:

1. Add a special caveat around mcap_vendor regarding ABI stability in the README, also in a quality level declaration if appropriate. This would matter for any users who are writing code that directly links to mcap_vendor (maybe 0 people). This would also matter to rosbag2_storage_mcap, which would be released in tandem with mcap_vendor.
2. Remove the mcap_vendor package, and include the MCAP library in the rosbag2_storage_mcap source as a header-only library. This removes any concern about ABI stability since the MCAP ABI is not presented as part of the rosbag2_storage_mcap plugin ABI.
3. release a 1.0 version of the MCAP C++ library, and commit to supporting that ABI for stable distributions in mcap_vendor.

I would choose 1) to make incremental builds slightly faster, or 2) if we want to prioritize having no caveats around ABI stability. 3) represents some significant work and also risk that we're committing to an ABI which doesn't work for future needs.

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/40bf0a50befa470e2b9c0f76c35f45c4/raw/ae1e612b7a6a463c0f379d0af9cb2692ed17e055/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage rosbag2_storage_sqlite3 rosbag2_storage_mcap rosbag2_tests rosbag2_cpp ros2bag rosbag2_transport
TEST args: --packages-above rosbag2_storage rosbag2_storage_sqlite3 rosbag2_storage_mcap rosbag2_tests rosbag2_cpp ros2bag rosbag2_transport
ROS Distro: foxy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12094

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Try to re-run CI with "focal"
ROS Distro: focal
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12095

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Try to re-run CI with "focal" replacing rosbag2_storage_sqlite3 with rosbag2_storage_default_plugins
ROS Distro: focal
BUILD args: --packages-above-and-dependencies rosbag2_storage rosbag2_storage_default_plugins rosbag2_storage_mcap rosbag2_tests rosbag2_cpp ros2bag rosbag2_transport
TEST args: --packages-above rosbag2_storage rosbag2_storage_default_plugins rosbag2_storage_mcap rosbag2_tests rosbag2_cpp ros2bag rosbag2_transport
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12099

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

@clalancette @emersonknapp If you don't mind I will merge this PR to Foxy branch since Foxy end of life is pretty soon and I would like to wrap up all activities and staled PRs related to it.

• Currently, Fpr job fails because auto-abi-checker can't pull in sources of the lz4 library in particularly lz4frame.h.
  lz4 dependency declared and fetched inside mcap_vendor/CMakeLists.txt.
  It seems auto-abi-checker is not able to get those dependencies referred in the CMake file as fetchcontent_declare(lz4, ..)
• However, CI tests are green for the Unix platforms. CI failure on Windows is unrelated and relates to some missing library from connext_dds. It seems Windows build is broken.
• As we discussed above in this thread error from auto-abi-checker is a false positive and there are no real API/ABI compatibility violations.
• As it was proposed, the mcap_vendor: only install public headers #1207 has been already backported to this branch in the latest commit b958fc6

I don't know how to disable auto-abi-checker or let it be aware that need to pull lz4 via Cmake script.

[emersonknapp]

This plan sounds fine to me. Since the checker issue is a false positive, I think it's OK to skip it

[clalancette]

• ``

• However, CI tests are green for the Unix platforms. CI failure on Windows is unrelated and relates to some missing library from connext_dds. It seems Windows build is broken.

To be clear, Windows build is not broken elsewhere on Foxy, so something must be going on here.

[clalancette]

Please don't merge things with red CI, particularly into stable distributions. If this really is a flake, then we should re-run the job. Otherwise, we may just be making things difficult for downstream users.

[clalancette]

I've kicked off another Windows build here to see if it is still red. If it is, then I think we should revert this:

• Windows

[clalancette]

(as for instance, ros2/message_filters#81 (comment) was run in the last week and built)

[MichaelOrlov]

@clalancette Sorry... I will revert it.

[MichaelOrlov]

@clalancette I created reverting PR #1347

[MichaelOrlov]

@james-rms @emersonknapp Need your help with RCA for failing https://ci.ros2.org/job/ci_windows/19528/testReport/junit/rosbag2_storage_mcap/test_message_definition_cache/can_resolve_msg_with_idl_deps/ test on Windows.

[MichaelOrlov]

Preliminary analysis for test failure.
The test fails because the difference in the expected output:

IDL: rosbag2_storage_mcap_testdata/msg/ComplexMsgDependsOnIdl
 // generated from rosidl_adapter/resource/msg.idl.em
-// with input from rosbag2_storage_mcap_testdata/msg\\ComplexMsgDependsOnIdl.msg
+// with input from rosbag2_storage_mcap_testdata/msg/ComplexMsgDependsOnIdl.msg
 // generated code does not contain a copyright notice

In particular, on the Windows platform message generator messed up with the input file name
/msg\\ComplexMsgDependsOnIdl.msg by inserting double backslashes \\ instead of one forward slash /.

It looks like this is an underlying issue in the message generator code on Foxy
I found relevant missing delta in :
On Foxy is
https://github.com/ros2/rosidl/blob/b62e8c4d156e73f155d3b8aaedaf96462baa94cf/rosidl_adapter/rosidl_adapter/msg/__init__.py#L33-L36

 data = {
        'pkg_name': package_name,
        'relative_input_file': input_file,
        'msg': msg,
    }

And on rolling is
https://github.com/ros2/rosidl/blob/e3b71ece1a5c351f4656e4ce6cbe421910936bc7/rosidl_adapter/rosidl_adapter/msg/__init__.py#L33-L37

 data = {
        'pkg_name': package_name,
        'relative_input_file': input_file.as_posix(),
        'msg': msg,
    }

On Foxy missing as_posix() method call for input_file property. Which is used in https://github.com/ros2/rosidl/blob/b62e8c4d156e73f155d3b8aaedaf96462baa94cf/rosidl_adapter/rosidl_adapter/resource/msg.idl.em#L2
as

// with input from @(pkg_name)/@(relative_input_file)

This missing as_posix() was introduced in ros2/rosidl#576

[MichaelOrlov]

Quick update:
I tried to make a fix on my local setup via adding .as_posix() to the input_file and it fixed the failing tests.

@clalancette How would you prefer to proceed? Revert this PR with rosbag2_storage_mcap bring up entirely or make a small fix in the https://github.com/ros2/rosidl/blob/b62e8c4d156e73f155d3b8aaedaf96462baa94cf/rosidl_adapter/rosidl_adapter/msg/__init__.py#L33-L36 ?

I honestly would prefer to try to make a small fix in rosidl_adapter rather than reverting this PR.

[emersonknapp]

I understand preferring the small patch (that is usually my instinct too) - but generally the practice is to revert any erroneously-merged PRs immediately, as the patch still may require review revisions.

[MichaelOrlov]

@emersonknapp Ok, then please review/approve the reverting PR #1347.

[emersonknapp]

I've kicked off another Windows build here to see if it is still red. If it is, then I think we should revert this:

Oh, the re-run wasn't red. It was yellow, though. I didn't read through that carefully enough. @clalancette thoughts - should we still revert, or just fix the failing tests and move on?

[MichaelOrlov]

@clalancette @emersonknapp I created PR ros2/rosidl#745 with fix anyway.
Please help with review.