Support using non-hunter version of bzip2, zlib and libarchive if HUNTER_ENABLED is OFF#1050
Support using non-hunter version of bzip2, zlib and libarchive if HUNTER_ENABLED is OFF#1050traversaro wants to merge 1 commit intoluxonis:developfrom
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.
When have these become available in "upstream CMake"?
Before/after our specified "minimal CMake version"
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Why are these performed twice?
There was a problem hiding this comment.
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.
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.
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.
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
ZLIBis found viafind_package(ZLIB CONFIG)and it defines theZLIB::zlibimported target, while the regular ZLIB does not install any CMake config file, but it can be found by CMake's upstreamFindZLIB.cmakethat defines theZLIB::ZLIBimported target.BZip2: The hunter/luxonis
BZip2is found viafind_package(BZip2 CONFIG)and it defines theBZip2::bz2imported target, while the regular BZip2 does not install any CMake config file, but it can be found by CMake's upstreamFindBZip2.cmakethat defines theBZip2::BZip2imported target.libarchive: The hunter/luxonis
libarchiveis found viafind_package(archive_static CONFIG)and it defines thearchive_staticimported target, while the regular libarchivedoes not install any CMake config file, but it can be found by CMake's upstreamFindLibArchive.cmakethat defines theLibArchive::LibArchiveimported 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_ENABLEDis 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_ENABLEDis set toON, the PR does not change the existing logic.