Switch to using ament_vendor_package for lz4. #1583

clalancette · 2024-03-12T14:20:30Z

This is more explicit, and more like how we vendor the rest of the packages in our ecosystem.

Note that this is the first of two PRs to switch mcap_vendor over to using ament_vendor_package. We want to do that to make it more like the other vendor packages in the core, and to fix building of binary mcap_vendor packages on Noble (we currently have a hack that allows us to do this, but we would like to get rid of it).

clalancette · 2024-03-12T14:21:00Z

CI:

Linux
Linux-aarch64
Windows
RHEL

lz4_vendor/CMakeLists.txt

lz4_vendor/README.md

lz4_vendor/CMakeLists.txt

lz4_vendor/cmake/Modules/Findlz4.cmake

clalancette · 2024-03-12T22:03:11Z

CI:

Linux
Linux-aarch64
Windows
RHEL

MichaelOrlov · 2024-03-14T07:15:17Z

@clalancette Any idea why ros2bag tests fails with error message:

 ==================================== ERRORS ====================================
2024-03-12T22:32:52.5615828Z ____________________________ ERROR collecting test _____________________________
2024-03-12T22:32:52.5620514Z /usr/local/lib/python3.10/dist-packages/pluggy/_hooks.py:501: in __call__
2024-03-12T22:32:52.5623335Z     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
2024-03-12T22:32:52.5625134Z /usr/local/lib/python3.10/dist-packages/pluggy/_manager.py:119: in _hookexec
2024-03-12T22:32:52.5626189Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-03-12T22:32:52.5627625Z /usr/local/lib/python3.10/dist-packages/_pytest/python.py:225: in pytest_collect_file
2024-03-12T22:32:52.5628637Z     module: Module = ihook.pytest_pycollect_makemodule(
2024-03-12T22:32:52.5630579Z /usr/local/lib/python3.10/dist-packages/_pytest/config/compat.py:79: in fixed_hook
2024-03-12T22:32:52.5631429Z     return hook(**kw)
2024-03-12T22:32:52.5646994Z /usr/local/lib/python3.10/dist-packages/pluggy/_hooks.py:501: in __call__
2024-03-12T22:32:52.5648220Z     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
2024-03-12T22:32:52.5649653Z /usr/local/lib/python3.10/dist-packages/pluggy/_manager.py:119: in _hookexec
2024-03-12T22:32:52.5650835Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-03-12T22:32:52.5652056Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:188: in pytest_pycollect_makemodule
2024-03-12T22:32:52.5653140Z     entrypoint = find_launch_test_entrypoint(path)
2024-03-12T22:32:52.5654253Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:178: in find_launch_test_entrypoint
2024-03-12T22:32:52.5655327Z     module = import_path(path, root=None)
2024-03-12T22:32:52.5656607Z E   TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'
2024-03-12T22:32:52.5658146Z --- generated xml file: /__w/rosbag2/rosbag2/ros_ws/build/ros2bag/pytest.xml ---
2024-03-12T22:32:52.5659198Z =========================== short test summary info ============================
2024-03-12T22:32:52.5660652Z ERROR test - TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'
2024-03-12T22:32:52.5661641Z !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
2024-03-12T22:32:52.5662128Z =============================== 1 error in 0.16s ===============================
2024-03-12T22:32:52.5948706Z Failed   <<< ros2bag [1.54s, exited with code 2]

I am pretty sure it is unrelated to the changes from this PR, but still unclear for me what is wrong with it.

MichaelOrlov

@clalancette @cottsay If I am not mistaken and recall correctly, the reason why we didn't "vendorize" lz4 at the very beginning and use it directly during the compilation of the mcap library from sources is because:

lz4 is released under the dual license. From the https://github.com/lz4/lz4?tab=License-1-ov-file
```
  This repository uses 2 different licenses :
   - all files in the `lib` directory use a BSD 2-Clause license
   - all other files use a GPLv2 license, unless explicitly stated otherwise
```
It turns out, if do it the way how it is done in the current PR. i.e. mix and match by compiling everything from lz4 sources into the vendor package library and then link against it in the mcap library. The mcap library will become poisoned by the GPLv2 license and everything else downstream linking mcap library will become poisoned by the GPLv2 license!!!
It might be "ok" for open source projects but certainly NO-NO for any commercial products. The consequences will be that all closed-source "poisoned" by the GPLv2 code or library will automatically become under the GPLv2 license and could be enforced to be open sourced.
Originally we workaround this problem by using explicitly only those part of the lz4 which is under the BSD license via

  file(GLOB _lz4_srcs
    ${lz4_SOURCE_DIR}/lib/*.c)

  add_library(mcap SHARED
    src/main.cpp
    ${_lz4_srcs}
  )

No one else uses lz4 anywhere else in the other ROS 2 core packages.

lz4_vendor/package.xml

MichaelOrlov · 2024-03-14T08:36:49Z

@clalancette I recall one more argument as to why we didn't "vendorize" lz4 from the very beginning.
3. The lz4 is a high-performant compression library and compiling mcap library with lz4 raw sources gives better results in performance rather than wrapping lz4 into the library and linking against it.
Folks from Foxglove even did some measurements before as far as I recall.

cc: @james-rms @defunctzombie

clalancette · 2024-03-14T12:47:27Z

lz4 is released under the dual license. From the https://github.com/lz4/lz4?tab=License-1-ov-file

I'll have to look at this one in more detail, though clearly I am not a lawyer so there's only so much I can do. All I'll say is that it seems very dicey to rely on that from a legal point-of-view.

2. No one else uses `lz4` anywhere else in the other ROS 2 core packages.

This part doesn't really matter; that's the case for e.g. zstd_vendor as well.

The lz4 is a high-performant compression library and compiling mcap library with lz4 raw sources gives better results in performance rather than wrapping lz4 into the library and linking against it.

This would be extremely surprising. The only way I can think that would happen would be if the compiler inlined everything.

MichaelOrlov · 2024-03-14T18:05:35Z

@clalancette My major concern is a viral "infection" of other libraries and executables by the GPLv2 license from the lz4 library.
We at Apex.AI have a strict requirement not to link against GPL-licensed libraries and code, and this strict requirement comes to us from big customers with well-known names.
Please note that at the same time it is ok to use standalone GPL-licensed tools or libraries which is not part of the product.

Curious, if we can include in the lz4_vendor package only BSD licensed part i.e. ${lz4_SOURCE_DIR}/lib/*.c?
It will solve the licensing issue.

clalancette · 2024-03-14T18:33:01Z

@clalancette My major concern is a viral "infection" of other libraries and executables by the GPLv2 license from the lz4 library.

I honestly don't think this is going to be much of a difference. In particular, we are still only building those same BSD-licensed files; the only difference is that we are now using a bit more of the build infrastructure. But I am not a lawyer and I cannot give legal advice.

In all honesty, if this is a huge concern we should not be using lz4 at all, and should switch to something where the license is clearer.

lz4_vendor/CMakeLists.txt

clalancette · 2024-03-19T17:23:01Z

All right. As we discussed, my latest push here renames the package to liblz4_vendor to emphasize we are only vendoring the library itself, none of the surrounding bits. I also added some explanation to the package.xml and the README.md about this.

Note that this now requires ros/rosdistro#40263 to go in first, since that makes an explicit distinction between liblz4 (the runtime library) and liblz4-devel (the development libraries).

@cottsay @MichaelOrlov Please take another look when you get a chance.

MichaelOrlov

@clalancette 👍 LGTM.

clalancette · 2024-03-19T19:47:05Z

Here is CI for it. Note that we can't merge this until we merge ros/rosdistro#40263, but at least we can be ready to go:

Linux
Linux-aarch64
Windows

mcap_vendor/CMakeLists.txt

defunctzombie · 2024-03-20T02:32:37Z

@clalancette I know you wrote this as the justification - but I'm wondering about the justification that led to this being the approach?

This is more explicit, and more like how we vendor the rest of the packages in our ecosystem.

Does this open the possibility that other packages can depend on this new vendor package? What if you don't want that to be the case and prefer to have this as an internal dependency?

What you have here seems fine but I was curious at the reasoning for making something that was previously internal now external.

clalancette · 2024-03-20T11:52:44Z

Does this open the possibility that other packages can depend on this new vendor package? What if you don't want that to be the case and prefer to have this as an internal dependency?

It does open this possibility, yes.

However, I will point out a few things:

On Linux (both Ubuntu and RHEL), this will actually not compile lz4 at all, and instead use the system version. That's our typical policy, as it lets ROS 2 use the same system libraries as everything else.
On Windows, you are right that this will make lz4 available for other packages to depend upon.
It is possible to make a library more "private"; we do that with rviz_ogre_vendor, for instance. I don't think it is necessary in this case, particularly because we are going to be using the system version whenever possible here.

defunctzombie · 2024-03-20T14:12:36Z

On Linux (both Ubuntu and RHEL), this will actually not compile lz4 at all, and instead use the system version. That's our typical policy, as it lets ROS 2 use the same system libraries as everything else

FWIW I've found this a hinderance in the past. System versions can sometimes be out of date and also drift which makes reasoning about which actual version of the library you have built against difficult. At a previous place of work we had a large effort to never depend on any system library for the robotics stack. Might be something to consider for the future.

clalancette · 2024-03-20T14:15:47Z

FWIW I've found this a hinderance in the past. System versions can sometimes be out of date and also drift which makes reasoning about which actual version of the library you have built against difficult. At a previous place of work we had a large effort to never depend on any system library for the robotics stack. Might be something to consider for the future.

Heh. That is the exact opposite of our policy, which is to use system versions wherever possible. If you don't do this, then you make it dangerous to use the system version of anything, because then you run the risk of multiple libraries presenting the same API but with different ABIs. It is part of being in the ecosystem.

cottsay · 2024-03-20T14:19:39Z

Worth calling out is that you can absolutely specify the FORCE_BUILD_VENDOR_PKG:BOOL=ON CMake argument to ignore the system packages and always build vendor payloads from source.

I think that for a general user, the risk of ABI collision, additional build time, and additional file size, are not worth the cost, but there has always been a mechanism to opt-out of using system packages for vendor payloads.

clalancette · 2024-03-20T20:55:06Z

Usual CI is happy. I'm going to run a couple of additional pieces of CI just to make sure:

RHEL
Windows Debug

clalancette · 2024-03-21T13:53:13Z

All right, all of the CI failures are known flakes. We still have to merge in ros/rosdistro#40263 first, so I'll wait for that to happen before merging this.

clalancette · 2024-04-03T15:32:17Z

All right, I've merged in all of the necessary changes elsewhere. I'm going to run one more set of CI on this after rebasing on the latest, then merge this in.

This is more explicit, and more like how we vendor the rest of the packages in our ecosystem. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

This is to make it clear that we are only vendoring the library portion, which is BSD, rather than the rest of it, which is GPL. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2024-04-03T15:37:55Z

CI:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

clalancette · 2024-04-09T22:24:02Z

I kind of lost this one in the shuffle, but it looks like CI was all green, and there have been no changes to this repository since then. It is also approved, so going ahead and merging this one.

clalancette requested a review from a team as a code owner March 12, 2024 14:20

clalancette requested review from MichaelOrlov and james-rms and removed request for a team March 12, 2024 14:20

cottsay reviewed Mar 12, 2024

View reviewed changes

lz4_vendor/CMakeLists.txt Outdated Show resolved Hide resolved

lz4_vendor/README.md Outdated Show resolved Hide resolved

lz4_vendor/CMakeLists.txt Outdated Show resolved Hide resolved

lz4_vendor/cmake/Modules/Findlz4.cmake Outdated Show resolved Hide resolved

cottsay approved these changes Mar 13, 2024

View reviewed changes

MichaelOrlov requested changes Mar 14, 2024

View reviewed changes

lz4_vendor/package.xml Outdated Show resolved Hide resolved

MichaelOrlov reviewed Mar 18, 2024

View reviewed changes

lz4_vendor/CMakeLists.txt Outdated Show resolved Hide resolved

clalancette mentioned this pull request Mar 19, 2024

Add in liblz4 and liblz4-dev keys. ros/rosdistro#40263

Merged

MichaelOrlov approved these changes Mar 19, 2024

View reviewed changes

defunctzombie reviewed Mar 20, 2024

View reviewed changes

mcap_vendor/CMakeLists.txt Show resolved Hide resolved

clalancette mentioned this pull request Apr 2, 2024

Install liblz4 for Ubuntu and RHEL. ros2/ci#768

Merged

Switch to using ament_vendor_package for lz4.

e4db0d7

This is more explicit, and more like how we vendor the rest of the packages in our ecosystem. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette added 2 commits April 3, 2024 15:32

Feedback from review.

49566b6

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Rename to liblz4_vendor.

12a6624

This is to make it clear that we are only vendoring the library portion, which is BSD, rather than the rest of it, which is GPL. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette force-pushed the clalancette/split-lz4-vendor branch from 2462660 to 12a6624 Compare April 3, 2024 15:37

clalancette merged commit cba14da into rolling Apr 9, 2024
13 of 14 checks passed

delete-merged-branch bot deleted the clalancette/split-lz4-vendor branch April 9, 2024 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using ament_vendor_package for lz4. #1583

Switch to using ament_vendor_package for lz4. #1583

clalancette commented Mar 12, 2024

clalancette commented Mar 12, 2024 •

edited

clalancette commented Mar 12, 2024 •

edited

MichaelOrlov commented Mar 14, 2024

MichaelOrlov left a comment •

edited

MichaelOrlov commented Mar 14, 2024

clalancette commented Mar 14, 2024

MichaelOrlov commented Mar 14, 2024 •

edited

clalancette commented Mar 14, 2024

clalancette commented Mar 19, 2024

MichaelOrlov left a comment

clalancette commented Mar 19, 2024 •

edited

defunctzombie commented Mar 20, 2024

clalancette commented Mar 20, 2024

defunctzombie commented Mar 20, 2024

clalancette commented Mar 20, 2024

cottsay commented Mar 20, 2024

clalancette commented Mar 20, 2024 •

edited

clalancette commented Mar 21, 2024

clalancette commented Apr 3, 2024

clalancette commented Apr 3, 2024 •

edited

clalancette commented Apr 9, 2024

Switch to using ament_vendor_package for lz4. #1583

Switch to using ament_vendor_package for lz4. #1583

Conversation

clalancette commented Mar 12, 2024

clalancette commented Mar 12, 2024 • edited

clalancette commented Mar 12, 2024 • edited

MichaelOrlov commented Mar 14, 2024

MichaelOrlov left a comment • edited

Choose a reason for hiding this comment

MichaelOrlov commented Mar 14, 2024

clalancette commented Mar 14, 2024

MichaelOrlov commented Mar 14, 2024 • edited

clalancette commented Mar 14, 2024

clalancette commented Mar 19, 2024

MichaelOrlov left a comment

Choose a reason for hiding this comment

clalancette commented Mar 19, 2024 • edited

defunctzombie commented Mar 20, 2024

clalancette commented Mar 20, 2024

defunctzombie commented Mar 20, 2024

clalancette commented Mar 20, 2024

cottsay commented Mar 20, 2024

clalancette commented Mar 20, 2024 • edited

clalancette commented Mar 21, 2024

clalancette commented Apr 3, 2024

clalancette commented Apr 3, 2024 • edited

clalancette commented Apr 9, 2024

clalancette commented Mar 12, 2024 •

edited

clalancette commented Mar 12, 2024 •

edited

MichaelOrlov left a comment •

edited

MichaelOrlov commented Mar 14, 2024 •

edited

clalancette commented Mar 19, 2024 •

edited

clalancette commented Mar 20, 2024 •

edited

clalancette commented Apr 3, 2024 •

edited