Skip to content

Fix zlib-ng detection and installation. #901

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix zlib-ng detection and installation. #901

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
627ba93
Fix zlib-ng detection and installation.
colugomusic Aug 30, 2025
93ad6fc
Fix linking of installed zlib-ng
colugomusic Aug 30, 2025
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
9 changes: 3 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ if(MZ_ZLIB)
# 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)
Copy link
Contributor

@sgoth sgoth Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

In perspective, 2.1.7 is from June 2024: https://github.com/zlib-ng/zlib-ng/releases/tag/2.1.7

find_package(ZLIB-NG QUIET CONFIG)
endif()
if (MZ_ZLIB_FLAVOR STREQUAL "zlib" OR MZ_ZLIB_FLAVOR STREQUAL "auto")
find_package(ZLIB QUIET)
Expand All @@ -195,9 +195,7 @@ if(MZ_ZLIB)
if(ZLIB-NG_FOUND AND NOT MZ_FORCE_FETCH_LIBS)
message(STATUS "Using ZLIB-NG")

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was also suggested in #886 so i guess its fine


set(PC_PRIVATE_DEPS "zlib-ng")
set(ZLIB_COMPAT OFF)
Expand All @@ -215,8 +213,7 @@ if(MZ_ZLIB)

clone_repo(zlib https://github.com/zlib-ng/zlib-ng stable)

# 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})
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Keep EXCLUDE_FROM_ALL to prevent other subprojects from building.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@Coeur Coeur Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.


list(APPEND MINIZIP_INC ${ZLIB_SOURCE_DIR})
list(APPEND MINIZIP_INC ${ZLIB_BINARY_DIR})
Expand Down