Fix zlib-ng detection and installation.#901
Conversation
|
@sgoth kindly review this change, thanks |
| # Check if zlib is present | ||
| if(NOT MZ_FORCE_FETCH_LIBS) | ||
| if (MZ_ZLIB_FLAVOR STREQUAL "zlib-ng" OR MZ_ZLIB_FLAVOR STREQUAL "auto") | ||
| find_package(ZLIB-NG QUIET) |
There was a problem hiding this comment.
This essentially disables the FindZLIB-NG script.
As stated by @fekstrom in #886 (comment) that would remove support for zlib-ng < 2.1.7
If that is fine, imho it would be best to delete the find script too.
This change wouldn't even be needed then as, in general, only the config would be found anyway. CMake manual recommends not being explicit with CONFIG (https://cmake.org/cmake/help/latest/command/find_package.html#typical-usage) but i guess that is very debatable...
There was a problem hiding this comment.
In perspective, 2.1.7 is from June 2024: https://github.com/zlib-ng/zlib-ng/releases/tag/2.1.7
| list(APPEND MINIZIP_INC ${ZLIB-NG_INCLUDE_DIRS}) | ||
| list(APPEND MINIZIP_LIB ${ZLIB-NG_LIBRARIES}) | ||
| list(APPEND MINIZIP_LBD ${ZLIB-NG_LIBRARY_DIRS}) | ||
| list(APPEND MINIZIP_LIB zlib-ng::zlib) |
|
|
||
| # Don't automatically add all targets to the solution | ||
| add_subdirectory(${ZLIB_SOURCE_DIR} ${ZLIB_BINARY_DIR} EXCLUDE_FROM_ALL) | ||
| add_subdirectory(${ZLIB_SOURCE_DIR} ${ZLIB_BINARY_DIR}) |
There was a problem hiding this comment.
There is no right or wrong here. Just if building the main project automatically builds the thing or not. Probably more newcomer friendly without EXCLUDE_FROM_ALL.
Not my use case so i don't have an opinion really.
There was a problem hiding this comment.
Keep EXCLUDE_FROM_ALL to prevent other subprojects from building.
There was a problem hiding this comment.
There is no right or wrong here. Just if building the main project automatically builds the thing or not. Probably more newcomer friendly without EXCLUDE_FROM_ALL.
Not my use case so i don't have an opinion really.
As I said this also affects the package install, so if you were to run
cmake --install /path/to/minizip/build
With EXCLUDE_FROM_ALL, this will only install the minizip package and not the zlib-ng package. find_package(minizip) will then fail because zlib is not installed which may be surprising if the consumer is paying attention and spots that zlib was automatically downloaded, built, and linked, only for it to not be installed.
So I suppose the "right or wrong" depends on whether or not the minizip-ng repository is intended to be useable for generating both the minizip and zlib-ng packages (I guess not), or if the whole clone_repo/FetchContent dance only exists to simplify things for consumers who are using add_subdirectory to include minizip-ng in their project (rather than find_package.)
What I ended up doing personally is installing the zlib-ng package directly from the zlib-ng source instead, and then installing minizip, so my package installation process doesn't go into the elseif(MZ_FETCH_LIBS) branch at all and the EXCLUDE_FROM_ALL no longer affects me, but I wanted to provide more context for why I suggested this change.
There was a problem hiding this comment.
CMake's conventional behavior is to build all targets by default unless a specific target is named.
I do understand that Nathan may want to only build the strict necessary by default instead of everything by default.
I don't have a strong opinion on this as it's outside all my scenarios.
At the moment, I tend to prefer Colugo removal of EXCLUDE_FROM_ALL, as this seems to be the more conventional approach regarding what a CMake file should do.
There was a problem hiding this comment.
For example zlib-ng includes its own ctests. How do you exclude all their tests if not doing EXCLUDE_FROM_ALL? Otherwise when running ctest in minizip-ng's CI, it will run all over zlib-ng's ctests..
|
I tested my use case (build local installation of minizip-ng against local installation of zlib-ng and then build another project against that) with the proposed patches (93ad6fc) and the latest zlib-ng on Mac (with -D MZ_LIBCOMP=OFF), Ubuntu, and Windows. It worked on all three platforms, so looks good to me. |
Following on from #886 this fixes the following issues
The
EXCLUDE_FROM_ALLhere will prevent the install steps for zlib-ng from being run, so unless the consumer explicitly installs their own zlib-ng package first,zlib-ng-config.cmakewill not be installed and so the eventual call tofind_package(minizip REQUIRED CONFIG)will fail.Even if zlib-ng is installed externally first, the minizip-ng configuration will fall into the
MZ_FETCH_LIBSbranch and clone zlib-ng from github anyway, because the presence of zlib-ng is checked withfind_package(ZLIB-NG QUIET)instead offind_package(ZLIB-NG QUIET CONFIG)When zlib-ng is located via
find_package()the following commands don't work and so minizip-ng will not build:They can be replaced with simply:
which fixes the build.