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.

Sign up for GitHub

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

Support using non-hunter version of bzip2, zlib and libarchive if HUNTER_ENABLED is OFF #1050

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Support using non-hunter version of bzip2, zlib and libarchive if HUNTER_ENABLED is OFF #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ endif()

# Include project dependencies
set(DEPTHAI_DEPENDENCY_INCLUDE "" CACHE FILEPATH "Optional cmake file to append to dependency processing, e.g. additional find_package()")
# Set as it is used in depthaiDependencies
set(DEPTHAI_HUNTER_ENABLED ${HUNTER_ENABLED})
include(depthaiDependencies)

# Add threads preference
Expand Down Expand Up @@ -461,6 +463,32 @@ if(DEPTHAI_CLANG_FORMAT)
target_clangformat_setup(${TARGET_CORE_NAME} "${header_dirs}")
endif()

# Some libraries have target names that change between the hunter/luxonis fork
# and the upstream version. If HUNTER_ENABLED is ON we want to force the
# use of the hunter/luxonis fork target, otherwise if HUNTER_ENABLED is OFF we
# fallback to the upstream target name if the hunter/luxonis fork one is not available
if(HUNTER_ENABLED)
set(DEPTHAI_BZIP2_TARGET "BZip2::bz2")
set(DEPTHAI_ARCHIVE_TARGET "archive_static")
set(DEPTHAI_ZLIB_TARGET "ZLIB::zlib")
else()
if(TARGET BZip2::bz2)
set(DEPTHAI_BZIP2_TARGET "BZip2::bz2")
else()
set(DEPTHAI_BZIP2_TARGET "BZip2::BZip2")
endif()
if(TARGET archive_static)
set(DEPTHAI_ARCHIVE_TARGET "archive_static")
else()
set(DEPTHAI_ARCHIVE_TARGET "LibArchive::LibArchive")
endif()
if(TARGET ZLIB::zlib)
set(DEPTHAI_ZLIB_TARGET "ZLIB::zlib")
else()
set(DEPTHAI_ZLIB_TARGET "ZLIB::ZLIB")
endif()
endif()

# link libraries
target_link_libraries(${TARGET_CORE_NAME}
PUBLIC
Expand All @@ -471,11 +499,11 @@ target_link_libraries(${TARGET_CORE_NAME}
PRIVATE
XLink
Threads::Threads
BZip2::bz2
${DEPTHAI_BZIP2_TARGET}
FP16::fp16
archive_static
${DEPTHAI_ARCHIVE_TARGET}
spdlog::spdlog
ZLIB::zlib
${DEPTHAI_ZLIB_TARGET}
ghcFilesystem::ghc_filesystem
)

Expand Down
3 changes: 3 additions & 0 deletions cmake/depthaiConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ set(DEPTHAI_ENABLE_BACKWARD @DEPTHAI_ENABLE_BACKWARD@)

set(DEPTHAI_ENABLE_CURL @DEPTHAI_ENABLE_CURL@)

# Get if the library was built with Hunter or not
set(DEPTHAI_HUNTER_ENABLED @HUNTER_ENABLED@)

# Specify that this is config mode (Called by find_package)
set(CONFIG_MODE TRUE)

Expand Down
43 changes: 35 additions & 8 deletions cmake/depthaiDependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,44 @@ endif()
# If library was build as static, find all dependencies
if(NOT CONFIG_MODE OR (CONFIG_MODE AND NOT DEPTHAI_SHARED_LIBS))

# BZip2 (for bspatch)
find_package(BZip2 ${_QUIET} CONFIG REQUIRED)

# FP16 for conversions
find_package(FP16 ${_QUIET} CONFIG REQUIRED)

# libarchive for firmware packages
find_package(archive_static ${_QUIET} CONFIG REQUIRED)
find_package(lzma ${_QUIET} CONFIG REQUIRED)
# ZLIB for compressing Apps
find_package(ZLIB CONFIG REQUIRED)
# Some packages have different package names depending on which
# version is installed, if we are using hunter we want to make sure to
# use the hunter/luxonis fork, otherwise we try using the hunter/luxonis
# fork and if that can't be found we found the regular version
if(DEPTHAI_HUNTER_ENABLED)
# BZip2 (for bspatch)
find_package(BZip2 ${_QUIET} CONFIG REQUIRED)

# libarchive for firmware packages
find_package(archive_static ${_QUIET} CONFIG REQUIRED)
find_package(lzma ${_QUIET} CONFIG REQUIRED)

# ZLIB for compressing Apps
find_package(ZLIB CONFIG REQUIRED)
else()
# BZip2 (for bspatch)
find_package(BZip2 ${_QUIET} CONFIG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When have these become available in "upstream CMake"?
Before/after our specified "minimal CMake version"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, here a recap based on CMake docs:

Package Find Imported target
ZLIB Introduced before CMake < 3.0 Introduced in CMake 3.1
BZip2 Introduced before CMake < 3.0 Introduced in CMake 3.12
LibArchive Introduced before CMake < 3.0 Introduced in CMake 3.17

So to actually get BZip2 or LibArchive non-Hunter dependencies to work, one needs a CMake version that is newer then the documented minimum (CMake 3.17, relased in March 2020).

How do you prefer to proceed? We could just have an error message if HUNTER_ENABLED is OFF and CMake version < 3.17, but that would break compilation for people that are setting HUNTER_ENABLED to OFF and then installing BZip2 and LibArchive from Hunter/luxonis fork, for which CMake 3.12/3.17 is not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you prefer to proceed? We could just have an error message if HUNTER_ENABLED is OFF and CMake version < 3.17, but that would break compilation for people that are setting HUNTER_ENABLED to OFF and then installing BZip2 and LibArchive from Hunter/luxonis fork, for which CMake 3.12/3.17 is not needed.

Lets see if we can just include the corresponding FindX.cmake in to our codebase under cmake/ folder and we rely on that instead. (as I think those are just CMake helpers at the end of the day)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of related to #1050 (comment) . Just to understand, you propose to take the FindZLIB.cmake from CMake (i.e. https://github.com/Kitware/CMake/blame/master/Modules/FindZLIB.cmake, copy it to depthai-core, modify it to (optionally) find the ZLIB config installed by Hunter's fork of zlib, and then just use find_package(ZLIB REQUIRED) without CONFIG? I typically avoid to vendor files from CMake as they tend to become outdated over time w.r.t. to the upstream version, introducing hard to debug failures and maintenance burden.

To make an example exactly with ZLIB, one of the reason I had to build depthai-core with HUNTER_ENABLED=FALSE and could not use something like #1021 is that otherwise Hunter forced me to use its own vendored FindZLIB (see https://github.com/cpp-pm/hunter/blob/93329a1bf921d55a7d113288573d19580fe08f09/cmake/find/FindZLIB.cmake#L2) that was shadowing the FindZLIB from upstream cmake, without providing the same functionality (in particular, the Hunter's FindZLIB wanted to use find_package(ZLIB CONFIG) , that has stated in #1050 (comment) does not work with non-Hunter ZLIB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having said that, if instead you prefer to keep a local FindZLIB.cmake I would be happy to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd make sense to include & later on we can "skip" it from including these if version of CMake is adequate, etc...

But LGTM on not tweaking it to also cover Hunter. So I'd just target having a version agnostic (or lower cmake version), that would support FindZLIB, et al

if(NOT BZip2_FOUND)
find_package(BZip2 ${_QUIET} REQUIRED)
endif()

# libarchive for firmware packages
find_package(archive_static ${_QUIET} CONFIG)
if(archive_static_FOUND)
find_package(lzma ${_QUIET} CONFIG REQUIRED)
else()
find_package(LibArchive ${_QUIET} REQUIRED)
endif()

# ZLIB for compressing Apps
find_package(ZLIB CONFIG)
if(NOT ZLIB_FOUND)
find_package(ZLIB REQUIRED)
endif()
Comment on lines +71 to +75
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these performed twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To give the precedence to hunter/luxonis fork of ZLIB and BZip2 (if they are installed) that install cmake config files. By default, if one calls find_package(<pkg>) without the CONFIG option and a Find<pkg>.cmake exists, the <pkg>-config.cmake file is ignored, and the Find<pkg>.cmake is used. By first calling find_package(<pkg> CONFIG) and only later fallback to find_package(<pkg>), we give the precedence back to config files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this case, as Hunter is already disabled, just be find_package(ZLIB CONFIG REQUIRED) ?

I haven't seen this differentiation yet - was there some issues without doing it that way?

Copy link
Contributor Author

@traversaro traversaro Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not strictly related to Hunter. To explain a bit, imagine if you are on Ubuntu 24.04 and you install ZLIB via apt (but you can see the same behaviour with conda, with vcpkg, with spack):

sudo apt install cmake build-essential libz-dev

If you try to put the following content in a CMakeLists.txt:

cmake_minimum_required(VERSION 3.16)

# Set the project name
project(FindZLIBExample)

# Find the ZLIB library without CONFIG
# This works in Ubuntu 24.04 with apt dependencies
find_package(ZLIB REQUIRED)

# Find the ZLIB library with CONFIG
# This does **not** work in Ubuntu 24.04 with apt dependencies as ZLIB does not install a CMake config file
find_package(ZLIB CONFIG REQUIRED)

and then your try to configure it (calling cmake -Bbuild -S. in the folder where the CMakeLists.txt), this will happen:

traversaro@IITBMP014LW012:~/test$ cmake -Bbuild -S.
-- The C compiler identification is GNU 13.2.0
-- The CXX compiler identification is GNU 13.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.3")
CMake Error at CMakeLists.txt:12 (find_package):
  Could not find a package configuration file provided by "ZLIB" with any of
  the following names:

    ZLIBConfig.cmake
    zlib-config.cmake

  Add the installation prefix of "ZLIB" to CMAKE_PREFIX_PATH or set
  "ZLIB_DIR" to a directory containing one of the above files.  If "ZLIB"
  provides a separate development package or SDK, be sure it has been
  installed.


-- Configuring incomplete, errors occurred!

The find_package(ZLIB REQUIRED) call is successful, as it uses the FindZLIB.cmake provided by CMake, while find_package(ZLIB CONFIG REQUIRED) fails as upstream zlib does not install any zlib-config.cmake or ZLIBConfig.cmake file, so it is not possible find_package(ZLIB CONFIG REQUIRED) . This is different if you install somehow (even without Hunter) the Hunter fork of zlib from https://github.com/luxonis/zlib, as that has been modified to install zlib-config.cmake (see https://github.com/luxonis/zlib/blob/hunter-1.2.11/CMakeLists.txt#L258-L262).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the insight - I was not aware these two paths differ
LGTM then retaining the current mechanism

endif()

# spdlog for library and device logging
find_package(spdlog ${_QUIET} CONFIG REQUIRED)
Expand Down
Loading