Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure to build_export_depend liblz4-dev. #1614

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

clalancette
Copy link
Contributor

Otherwise downstream packages that depend on it won't install the package, and thus won't find the library.

@MichaelOrlov We need to break the freeze and land this on rolling so we can get mcap_vendor building on Rolling.

Otherwise downstream packages that depend on it won't
install the package, and thus won't find the library.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette requested a review from a team as a code owner April 17, 2024 00:55
@clalancette clalancette requested review from emersonknapp and hidmic and removed request for a team April 17, 2024 00:55
@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for freeze exception - this is a very safe bugfix

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette @cottsay We can export dependencies from the liblz4-dev but it is not going to help with CI build failure.
The problem is not in the missing export of the dependencies but rather that header lz4.h is not found because libz4-dev is not installed by default.
At least on my local setup I am getting the following error even after the proposed fix:

--- stderr: liblz4_vendor
CMake Error at cmake/Modules/Findlz4.cmake:21 (file):
  file STRINGS file "/usr/include/lz4.h" cannot be read.
Call Stack (most recent call first):
  CMakeLists.txt:10 (find_package)

What it helps is to install liblz4-dev via sudo apt install liblz4-dev.

IMO instead of trying to refer to the lz4.h from the liblz4-dev we need to instruct CMake somehow to refer to the downloaded version of the lz4.h from the https://github.com/lz4/lz4/blob/dev/lib/lz4.h which we are vendoring.
It seems ament_vendor doesn't do a good job for that.

@MichaelOrlov
Copy link
Contributor

@clalancette @cottsay BTW if needs a quick fix
Commenting out find_package(lz4 QUIET)

find_package(lz4 QUIET)

will help.

@MichaelOrlov
Copy link
Contributor

Sorry for my previous messages. You can disregard them. I didn't look at the original error messages from the failed job, and it turns out that they differ from my local setup reproduction.
The original one from the failed job https://build.ros2.org/job/Rbin_uN64__mcap_vendor__ubuntu_noble_amd64__binary/12/console

13:46:16 -- Found liblz4_vendor: 0.26.0 (/opt/ros/rolling/share/liblz4_vendor/cmake)
13:46:16 CMake Error at /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
13:46:16   Could NOT find lz4 (missing: lz4_LIBRARY lz4_INCLUDE_DIR)
13:46:16 Call Stack (most recent call first):
13:46:16   /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
13:46:16   /opt/ros/rolling/share/liblz4_vendor/cmake/Modules/Findlz4.cmake:36 (find_package_handle_standard_args)
13:46:16   CMakeLists.txt:7 (find_package)
13:46:16 

It seems the failure on my local setup is different and somehow Findlz4.cmake found a path for lz4 in my usr/include/ folder in the
find_path(lz4_INCLUDE_DIR NAMES lz4.h PATH_SUFFIXES "lz4" ${lz4_INCLUDE_PATH}) statement but there are no lz4.h file there.

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve

@marcoag marcoag merged commit 13cba14 into rolling Apr 17, 2024
14 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/add-build-export-depend branch April 17, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants