-
Notifications
You must be signed in to change notification settings - Fork 127
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
Support using non-hunter version of bzip2, zlib and libarchive if HUNTER_ENABLED is OFF #1050
base: develop
Are you sure you want to change the base?
Conversation
…TER_ENABLED is OFF
find_package(ZLIB CONFIG REQUIRED) | ||
else() | ||
# BZip2 (for bspatch) | ||
find_package(BZip2 ${_QUIET} CONFIG) |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
# ZLIB for compressing Apps | ||
find_package(ZLIB CONFIG) | ||
if(NOT ZLIB_FOUND) | ||
find_package(ZLIB REQUIRED) | ||
endif() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Thanks @traversaro ! Really excited to get these merged in. The changes besides the implicit minimum cmake version look good to me. As I see it this is what has to be done before the merge:
|
Introduction: among our modifications to support the HUNTER_ENABLED=OFF build (see also #1048 and #1049) this is probably the one that complicates the code the most, so I would understand if you prefer to not merge such modification. However, I think it was worth to isolate the change and made it available in the form of a PR.
When setting
HUNTER_ENABLED=OFF
, one can install all the dependencies via the usual CMake workflow in a directory that is added toCMAKE_PREFIX_PATH
, and then build depthai-core . However, there are three packages that are installed in different ways depending of weather the hunter/luxonis fork is used, or if the upstream version is used.The three packages are:
ZLIB: The hunter/luxonis
ZLIB
is found viafind_package(ZLIB CONFIG)
and it defines theZLIB::zlib
imported target, while the regular ZLIB does not install any CMake config file, but it can be found by CMake's upstreamFindZLIB.cmake
that defines theZLIB::ZLIB
imported target.BZip2: The hunter/luxonis
BZip2
is found viafind_package(BZip2 CONFIG)
and it defines theBZip2::bz2
imported target, while the regular BZip2 does not install any CMake config file, but it can be found by CMake's upstreamFindBZip2.cmake
that defines theBZip2::BZip2
imported target.libarchive: The hunter/luxonis
libarchive
is found viafind_package(archive_static CONFIG)
and it defines thearchive_static
imported target, while the regular libarchivedoes not install any CMake config file, but it can be found by CMake's upstreamFindLibArchive.cmake
that defines theLibArchive::LibArchive
imported target.If you have a project that is already using the upstream version, in general you would like for depthai-core to also use the upstream version.
To achieve this, this PR modifies the logic if
HUNTER_ENABLED
is set toOFF
. In that case, first the hunter/luxonis package names and target are tested, and if they can be found they are used, while if they do not exist the logic fallbacks to the upstream package names/target names.If
HUNTER_ENABLED
is set toON
, the PR does not change the existing logic.