Skip to content

Provide ZLIB::ZLIB as a CMake alias when shared libraries are disabled#1086

Open
ccawley2011 wants to merge 1 commit into
madler:developfrom
ccawley2011:cmake-target
Open

Provide ZLIB::ZLIB as a CMake alias when shared libraries are disabled#1086
ccawley2011 wants to merge 1 commit into
madler:developfrom
ccawley2011:cmake-target

Conversation

@ccawley2011
Copy link
Copy Markdown
Contributor

This matches the behaviour of FindZLIB from standard CMake installations.

@Vollstrecker
Copy link
Copy Markdown
Contributor

Point 1: Why do you checkif the target exists, if you could check for the option if the target should be created?

Point 2: FindZLIB tries to provide something, but the point in the named targets is to know what you link against. If you you want a static linking, there's a target. Imagine you expect dynamic linking and want to install the dll in your script, but there is none, you would have to add additional checks. Yes, with your changes in the config you mimic the old behaviour, but if someone includes the sources Idmeand them to check if there are changes. Such situations happen with apis all the time, so it's not asked too much to check if all the targets are still correct (and if you link against ZLIB:::ZLIB and don't set any options you'll have no problems in this case), and instead of checking for existing dll's I consider it better to check for the version and just use the right targets in you build instead of hoping to get sommething.

@Mizux
Copy link
Copy Markdown

Mizux commented Oct 15, 2025

ZLIB_USE_STATIC_LIBS
Added in version 3.24.
Set this variable to ON before calling find_package(ZLIB) to look for static libraries. Default is OFF.

from: https://cmake.org/cmake/help/latest/module/FindZLIB.html

So having ZLIB::ZLIB fallback to zlibstatic when using ExternalProject/FetchContent/add_subdirectory() could be a reasonable behaviour imho...
also if consumer require to know if it is a static or a shared lib (which also can be see as an integration implementation details) you could still use $<STREQUAL:$<TARGET_PROPERTY:ZLIB::ZLIB,TYPE>,SHARED_LIBRARY>...

@Vollstrecker
Copy link
Copy Markdown
Contributor

So having ZLIB::ZLIB fallback to zlibstatic when using ExternalProject/FetchContent/add_subdirectory() could be a reasonable behaviour imho...

You mean when you
a) use FindZLIB and get no result or
b) when you unconditionally include the lib like you mentioned or
c) when using the new automatism of find_package (didn't test, but from reading it's the same as option a

So in all 3 cases you can set ZLIB_BUILD_SHARED to OFF before doing this and alias this yourself.

also if consumer require to know if it is a static or a shared lib (which also can be see as an integration implementation details) you could still use $<STREQUAL:$<TARGET_PROPERTY:ZLIB::ZLIB,TYPE>,SHARED_LIBRARY>...

Because FindZLIB line 254 sets it to UNKNOWN and you'll never know?

But if you mention genex as an point, what about $&lt;If $&lt;TARGET_EXISTS:ZLIB::ZLIBSTATIC>, ZLIB::ZLIBSTATIC, ZLIB::ZLIB>?

@Neustradamus
Copy link
Copy Markdown

@madler: Have you seen this PR?

@Mizux
Copy link
Copy Markdown

Mizux commented Nov 18, 2025

@Vollstrecker
Copy link
Copy Markdown
Contributor

I usually don't comment such things, but there are some problems with this text:

  • He always refers to "our design criteria" not explaining who's or where they can be viewed or why someone else should care about them.
  • He states that users have to build both version which is time consuming, which is wrong for zlib as you can turn one or even both off. At the same time he refers to vcpkg which by default builds release and debug which is usually not needed.
  • Regarding turning off one version to save time: He states to have a function to turn that switch, so setting a var is good in that case. Small difference in that case: setting ZLIB_BUILD_SHARED can be read afterwards while the CMAKE_BUILD_SHARED in that function is inaccessible afterwards.
  • He states that the type of the library is always determined, which is true in the current state but wrong when using findZLIB or when merging this PR. Atm. we have the same problem with minizip because bz2 is not specific with this and it's possible to link a static libminizip with a shared bz2 or vice versa without knowing it.
  • He also refers to add_subdirectory or fetch_content. For both a function would be needed to set CMAKE_BUILD_SHARED only for the included project while atm. it's just setting a var to choose and include the project the have the same effect.
  • He also states that users usually want on one variant and completely ignores that packagers for distros usually build both variants. In his example it's not even easy to have them both installed and I'm pretty sure distros running the complete configure-process consumes more time than users building a second lib (we're not talking about monsters like Qt where this would make a difference measured in hours not minutes).
  • He talks about an ideal world where users choose easily what they want. My ideal world is you link against a shared libminizip that links against a shared zlib and a shared bz2 OR you link against a static libminizip that links against static zlib and static bz2 just by choosing the target minizip or minizip-static instead of making that setting for all 3 of them.

@zero-rp
Copy link
Copy Markdown

zero-rp commented Nov 23, 2025

#1113

@fabian-philips
Copy link
Copy Markdown

There are several CMake-based packages out there that use zlib by CMake's FIND_PACKAGE(zlib) module, maybe setting ZLIB_USE_STATIC_LIBS and link target ZLIB::ZLIB (which refers to zlibstatic if ZLIB_USE_STATIC_LIBS is turned on).

Looking at the changes in 1.3.2, this would break compatibility, as mentioned above, since as a static link target ZLIB::ZLIBSTATIC is introduced.

Why not making this "symmetric" with respect to static/shared libraries and providing the following explicit link targets

ZLIB::ZLIBSTATIC for the static library
ZLIB::ZLIBSHARED for the shared library

and

ZLIB::ZLIB as a "convenience" alias pointing to the shared library (if ZLIB_USE_STATIC_LIBS is not set) and to the static library if ZLIB_USE_STATIC_LIBS is set?

This would enable legacy packages to continue to compile while allowing them to migrate to the new targets ZLIB::ZLIBSTATIC and ZLIB::ZLIBSHARED. Currently with 1.3.2, changes would requiring breaking changes which are out of control for developers just using them in a development stack.

@Vollstrecker
Copy link
Copy Markdown
Contributor

Looking at the changes in 1.3.2, this would break compatibility, as mentioned above, since as a static link target ZLIB::ZLIBSTATIC is introduced.

Why not making this "symmetric" with respect to static/shared libraries and providing the following explicit link targets

ZLIB::ZLIB was intended to continue to work, so it doesn't change something for most Linux Distros where shared is default. Static is way too often used to stick to a version independet from system, often combined with a vendored copy so no adjustement for changes in newer releases is needed. So current design is encouraging to use shared libs.

This would enable legacy packages to continue to compile while allowing them to migrate to the new targets ZLIB::ZLIBSTATIC and ZLIB::ZLIBSHARED. Currently with 1.3.2, changes would requiring breaking changes which are out of control for developers just using them in a development stack.

Because we all know devs are lazy, if there is a way to keep it working without modification, the modification won't happen (also some just don't know about the changes or see them real late, as your late reply suggests.

What do you mean with out of their control? If there are deps that are actively maintained, just open a report to let them know it's a problem for you (maybe with patch). If the deps are unmaintained legacy code noone cares about either replace the dep, or revive the project and help rescuing projects you rely on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants