Skip to content

deps: swap in zlib-ng for zlib with ENABLE_ZLIBNG=1 #4639

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
+133 −17
Draft

deps: swap in zlib-ng for zlib with ENABLE_ZLIBNG=1 #4639

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
096508a
deps: swap in zlib-ng for zlib with ENABLE_ZLIBNG=1
zachlewis Feb 11, 2025
f7a5231
fix: Revert changes to pyproject.toml
zachlewis Feb 21, 2025
72e8a29
build(zlib): ENABLE_ZLIB=1 permits building with zlib instead of zlib-ng
zachlewis Feb 22, 2025
34ed8b0
fix(deps): let minizip-ng find locally-built zlibs
zachlewis Mar 8, 2025
bd02b23
build: More changes to be robust for cmake 4.0
zachlewis Apr 2, 2025
06988f1
Merge branch 'main' into zlib_ng_compat
zachlewis Apr 15, 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
2 changes: 2 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ LINKSTATIC = "1"
# Standardize the install directory for libraries, as expected by
# other parts of the wheels build process.
CMAKE_INSTALL_LIBDIR = "lib"
#OpenImageIO_BUILD_LOCAL_DEPS = "all"
IGNORE_HOMEBREWED_DEPS = "1"

# Dynamically set the package version metadata by pasrsing CMakeLists.txt.
[tool.scikit-build.metadata.version]
Expand Down
121 changes: 108 additions & 13 deletions src/cmake/build_ZLIB.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,131 @@
# https://github.com/AcademySoftwareFoundation/OpenImageIO

######################################################################
# ZLIB by hand!
# ZLIB / ZLIB-NG by hand!
######################################################################

set_cache (ZLIB_BUILD_VERSION 1.3.1 "ZLIB version for local builds")
set (ZLIB_GIT_REPOSITORY "https://github.com/madler/zlib")
set (ZLIB_GIT_TAG "v${ZLIB_BUILD_VERSION}")
# This script builds either zlib or zlib-ng (in compatibility mode) from source.

set_cache (ZLIB_BUILD_SHARED_LIBS ${LOCAL_BUILD_SHARED_LIBS_DEFAULT}
DOC "Should execute a local ZLIB build, if necessary, build shared libraries" ADVANCED)
# Zlib-ng is a fork of zlib that is actively maintained and has some
# performance improvments over the original zlib which pertain to
# PNG and WebP plugin performance (on Windows, in particular).

# By default, we build zlib-ng in zlib API compatibility mode. This
# allows OIIO and its dependencies to use zlib-ng v2.2.4 as a drop-in
# replacement for zlib v1.3.1.

# Please note -- this means that `find_package(ZLIB)` will recognize zlib-ng
# as zlib, and set ZLIB_VERSION = "1.3.1" (instead of zlib-ng's version)


# There are two ways to build against zlib instead of zlib-ng:

# 1. At build time, set `ENABLE_ZLIBNG=OFF` to
# to build with the original zlib (defaults to ON).

# 2. When using the `checked_find_package` macro, require a version
# less than 2.0.0, e.g. `find_package(ZLIB 1.3.1 REQUIRED)`.


set (ZLIB_VERSION_LATEST "1.3.1")
set (ZLIBNG_VERSION_LATEST "2.2.4")

# By default, we build zlib-ng in compatibility mode.
# Set ENABLE_ZLIBNG=OFF to build the original zlib.
check_is_enabled(ZLIBNG ENABLE_ZLIBNG)


# Set the version to build; note that 1.x versions will build the original zlib,
# while 2.x or later will build zlib-ng.
if (ENABLE_ZLIBNG)
set_cache (ZLIB_BUILD_VERSION ${ZLIBNG_VERSION_LATEST} "ZLIB or ZLIB-NG version for local builds")
else ()
set_cache (ZLIB_BUILD_VERSION ${ZLIB_VERSION_LATEST} "ZLIB or ZLIB-NG version for local builds")
endif ()


# Choose the git repository, tag format, and extra arguments based on version.
# For now, we're assuming that the original zlib will never reach version 2.0,
# so anything greater than or equal to 2.0 is zlib-ng.
if (ZLIB_BUILD_VERSION VERSION_LESS "2.0.0")
# Original zlib: use the 'v' prefix in the tag.
set (ZLIB_GIT_REPOSITORY "https://github.com/madler/zlib")
set (ZLIB_GIT_TAG "v${ZLIB_BUILD_VERSION}")
set (ZLIB_BUILD_OPTIONS "")
set (ZLIBNG_USED FALSE)
if (ENABLE_ZLIBNG)
message (STATUS "Building zlib version ${ZLIB_BUILD_VERSION}, even though ENABLE_ZLIBNG=${ENABLE_ZLIBNG}")
message (STATUS "If you believe this is a mistake, check usages of `checked_find_package(ZLIB ...)` in the code base.")
message (STATUS "See src/cmake/build_ZLIB.cmake for more details.")
else ()
message (STATUS "Building zlib version ${ZLIB_BUILD_VERSION}")
endif ()
else ()
# zlib-ng: omit the 'v' prefix for the git tag and enable compatibility mode.
set (ZLIB_GIT_REPOSITORY "https://github.com/zlib-ng/zlib-ng")
set (ZLIB_GIT_TAG "${ZLIB_BUILD_VERSION}")
set (ZLIB_BUILD_OPTIONS "-DZLIB_COMPAT=ON;-DWITH_GTEST=OFF;-DWITH_GZFILEOP=OFF;-DZLIBNG_ENABLE_TESTS=OFF;-DZLIB_ENABLE_TESTS=OFF")
set (ZLIBNG_USED TRUE)
message (STATUS "Building zlib-ng version ${ZLIB_BUILD_VERSION}")
endif ()


set_cache (ZLIB_BUILD_SHARED_LIBS OFF
DOC "Should execute a local ZLIB build, if necessary, build shared libraries" ADVANCED)
Comment on lines +75 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

Concern: by building zlib-ng as a static library, won't that prevent it from being used by libpng, which is where much of the benefit of the faster zlib implementation would come from? Any static libraries we incorporate into libOpenImageIO have their symbols hidden by default, specifically so nothing else we link will find those symbols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah? You'd definitely know better than I; I only barely understand what I'm doing! But yes, we specifically want PNG to find and use zlib-ng; the rest doesn't really matter.

I was hoping by placing the local build prefix at the front of CMAKE_PREFIX_PATH, and passing CMAKE_IGNORE_PATH to child build processes, that would coerce the PNG build process to find and link zlib-ng before any other zlibs. I'll should turn on shared libs and turn off plugin bundling to see what's actually happening...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I see what you mean. I'm honestly not sure. You might be right that it could be picked up by libpng and other dependencies IF we build them ourselves. But I think the usual case is that some/most/all of the dependencies are already built.

One possibility is that we should build statically, but expose the symbols by adding them to the global list in hidesymbols.map, so that libpng and others will find them inside our library at actual load time. We may need to also modify libOpenImageIO/CMakeLists where we have -Wl,--exclude-libs,ALL. (By default, we hide all symbols from static library dependencies so that we only use them internally but don't expose their symbols for dependencies to link against.)

But once we expose them, they might be picked up by anything that links us as a dependency, too, depending on the link order. So, for example, an app that itself uses zlib and OIIO might end up itself using zlib-ng inadvertently. Is that always ok? Maybe it's fine, I just think we need to be confident that this is the behavior we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, for example, an app that itself uses zlib and OIIO might end up itself using zlib-ng inadvertently. Is that always ok? Maybe it's fine, I just think we need to be confident that this is the behavior we want.

Hmm. Very interesting. If the zlib-ng documentation is to be believed, I would imagine we'd want this behavior; but I don't feel qualified to so say so for sure...

Would exposing zlib-ng-compatibility-mode symbols in OIIO impose unexpected constraints or build-time requirements on building apps linking libraries that themselves link OIIO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update: @joaovbs96 tested one of the Windows bdist artifacts produced by the last Wheels run for this thread, and has confirmed PNG write speed improvements consistent with having linked against zlib-ng instead of zlib -- in his use case (which inspired this PR), write times for very large PNGs decreased from ~20s to ~8s.


string (MAKE_C_IDENTIFIER ${ZLIB_BUILD_VERSION} ZLIB_VERSION_IDENT)

build_dependency_with_cmake(ZLIB
build_dependency_with_cmake (ZLIB
VERSION ${ZLIB_BUILD_VERSION}
GIT_REPOSITORY ${ZLIB_GIT_REPOSITORY}
GIT_TAG ${ZLIB_GIT_TAG}
CMAKE_ARGS
-D BUILD_SHARED_LIBS=${ZLIB_BUILD_SHARED_LIBS}
-D CMAKE_POSITION_INDEPENDENT_CODE=ON
-D CMAKE_INSTALL_LIBDIR=lib
${ZLIB_BUILD_OPTIONS}
)

# Set some things up that we'll need for a subsequent find_package to work
set (ZLIB_ROOT ${ZLIB_LOCAL_INSTALL_DIR})

# Signal to caller that we need to find again at the installed location
set (ZLIB_REFIND TRUE)
set (ZLIB_VERSION ${ZLIB_BUILD_VERSION})
set (ZLIB_REFIND_VERSION ${ZLIB_BUILD_VERSION})

if (ZLIB_BUILD_SHARED_LIBS)
install_local_dependency_libs (ZLIB ZLIB)
if (ZLIBNG_USED)
# zlib-ng provides a CMake config file, so we can use find_package directly.
find_package (ZLIB CONFIG REQUIRED HINTS ${ZLIB_LOCAL_INSTALL_DIR} NO_DEFAULT_PATH)

# First, locate the directory containing zlib.h.
find_path (ZLIB_INCLUDE_DIR
NAMES zlib.h
HINTS ${ZLIB_ROOT}/include
)

if (NOT ZLIB_INCLUDE_DIR)
message (FATAL_ERROR "Could not locate zlib-ng include directory.")
endif ()

message (STATUS "Found zlib-ng header directory: ${ZLIB_INCLUDE_DIR}")

# Read the contents of zlib.h
file (READ "${ZLIB_INCLUDE_DIR}/zlib.h" ZLIB_HEADER_CONTENT)

# Use a regular expression to search for the ZLIB_VERSION macro.
# This regex looks for a line like: #define ZLIB_VERSION "1.3.1.zlib-ng"
string (REGEX MATCH "#[ \t]*define[ \t]+ZLIB_VERSION[ \t]+\"([^\"]+)\"" _match "${ZLIB_HEADER_CONTENT}")

if (_match)
# The first capture group is stored in CMAKE_MATCH_1.
set (ZLIB_VERSION "${CMAKE_MATCH_1}")
endif ()


else ()
# Vanilla ZLIB doesn't ship with a CMake config file, so we'll just "refind" it with the
# usual arguments.
set (ZLIB_REFIND TRUE)
set (ZLIB_REFIND_VERSION ${ZLIB_BUILD_VERSION})
endif ()

if (ZLIB_BUILD_SHARED_LIBS)
install_local_dependency_libs(ZLIB ZLIB)
endif()
2 changes: 1 addition & 1 deletion src/cmake/build_minizip-ng.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ set (minizip-ng_GIT_TAG "${minizip-ng_BUILD_VERSION}")
set_cache (minizip-ng_BUILD_SHARED_LIBS OFF
DOC "Should a local minizip-ng build, if necessary, build shared libraries" ADVANCED)

checked_find_package (ZLIB REQUIRED)
find_package (ZLIB REQUIRED)


build_dependency_with_cmake(minizip-ng
Expand Down
10 changes: 9 additions & 1 deletion src/cmake/externalpackages.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,15 @@ include (FindThreads)
# Dependencies for required formats and features. These are so critical
# that we will not complete the build if they are not found.

checked_find_package (ZLIB REQUIRED) # Needed by several packages
check_is_enabled(ZLIBNG ENABLE_ZLIBNG)
if (ENABLE_ZLIBNG)
checked_find_package (ZLIB REQUIRED
VERSION_MIN 2.2.4 # ZLIB-NG
BUILD_LOCAL missing
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's your rationale for putting BUILD_LOCAL missing here? We do that very sparingly. Do you think we should make zlib-ng require a specific "opt out", meaning that it's used unless you specifically disable it, and furthermore should building it automatically be opt-in or opt-out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that old ZLIB was not 'BUILD_LOCAL missing', it would just fail if not found, unless the build was initiatied with zlib (or all) in the list of things to build automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is one of those things I was struggling with that keeps this PR in "draft" form -- I need to further explore, and wanted to get your opinion on this. Looking at what I've done a few days later, I'm not sure how much sense I was making. I was definitely overthinking it.

So, what I was trying to do is always try to locally build_ zlib-ng by default (i.e., users could "opt-out" by setting ENABLE_ZLIBNG=0 etc). The plan was for the "ENABLE_ZLIBNG=1" option to always attempt to build zlib-ng when the option is enabled, because find_package(zlib) will (currently) never find a zlib greater than version 1.x, if zlib-ng is installed in compatibility mode. Setting the "VERSION_MIN" to something above 2.0 causes build_ZLIB.cmake to treat zlib as missing, build zlib-ng in compatibility mode, and subsequently "find" zlib-ng 2.2.4 "as" zlib-1.3.1.

My thinking was, if the ZLIBNG building behavior was "opt-out" by default, if I didn't have the BUILD_LOCAL missing bit, users would suddenly be forced to set OpenImageIO_BUILD_MISSING_DEPS or OpenImageIO_BUILD_LOCAL_DEPS by default.

I think now maybe the best course of action is to instead make building zlib-ng "opt-in" behavior, and have users also provide the _BUILD_MISSING_DEPS / _BUILD_LOCAL_DEPS options as necessary; and then I could get rid of the BUILD_LOCAL missing bit (unless you felt that opting in to enabling zlib-ng should always imply BUILD_LOCAL missing).

Also, ENABLE_ZLIBNG as an option name is a bit of a misnomer, because, if disabled, we'd need considerably more complex logic to skip over any found (e.g. user-provided) zlib-ng-compatibility-mode libs in favor of vanilla zlib, since they cannot be differentiated by CMake without inspecting the headers... so, if you have a better name for the option, I'm all ears.

The more I think about it, the more sense it makes to "opt-in" to zlib-ng.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the logic that makes most sense and is symmetric with how we do it elsewhere (like with libjpeg-turbo) is:

  • If zlib-ng is found, it is used by default instead of zlib (but of course with ENABLE_ZLIBNG=OFF able to turn it off even if found).
  • If zlib-ng is not found, it is not auto-built by default (keeping in line with how there's very little that we auto-build by default), but we can auto-build if the right options are set (just like any other package).

To put it another way: using is opt-out, but autobuilding it is opt-in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To put it another way: using is opt-out, but autobuilding it is opt-in.

Aye-aye.

What makes this tricky is that find_package(ZLIB) can't distinguish zlib from zlib-ng-compatibility-mode (by design), other than that zlib-ng provides a proper CMake config, and original gangster zlib does not. That makes it easy to only find zlib-ng, but difficult to never find zlib-ng.

(but of course with ENABLE_ZLIBNG=OFF able to turn it off even if found)

I'm unsure of the best way to force find_package to always ignore zlib-ng...

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it'll get you unstuck, I'm ok accepting "opt in to build", but "always use if found". We can keep it in main for a while, and if it's a problem in practice (or somebody demands to build against zlib even though they also have zlib-ng), then we can try to solve that later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No rush to get this in place. Let's be sure this does what people expect it to do, and doesn't cause problems.

Keep in mind, we're only talking about zlib-ng libs built expressly to look and quack exactly like zlib. Is it safe to assume that the presence of compatibility-mode-zlib-ng itself is indicative of intent? It's not impossible that this could happen by accident, but I feel like users would be best served by adjusting their build environments, rather than risk making OIIO's build system much smarter than anyone else's...

But who knows why people need to do things. I'll do my best to try to accommodate everyone...

This probably isn't the most intuitive way of presenting these options to users -- very open to suggestions -- but at the moment, here's what I'm doing:

  • DISABLE_ZLIBNG is the legacy behavior "opt-out" switch -- controls whether we find + autobuild zlib-ng (default) over zlib
  • DISABLE_ZLIB controls how strongly we prefer zlib-ng over zlib -- a little (we'd allow zlib if zlib-ng isn't available) (default), or a lot (we'd autobuild a missing zlib-ng)

Allowing for:

  • Default behavior: try to find zlib-ng, else try to find zlib, else try to autobuild zlib-ng.
  • Wheels: DISABLE_ZLIB=1 and OpenImageIO_BUILD_MISSING_DEPS=zlib will force the build system to always find + autobuild zlib-ng.
  • Legacy: DISABLE_ZLIBNG=1 will force the build system to only find + autobuild zlib, ignoring zlib-ng. (Setting DISABLE_ZLIB=1 isn't meaningful when DISABLE_ZLIBNG=1).

Here's the thing: all of this assumes that zlib-ng exports a CMake config, and vanilla zlib does not. And this was reliably the case, until about three weeks ago: there's a commit in zlib's development branch implementing support for exporting configs...!

So, eventually, zlib-1.3.2's exporting of CMake configs will interfere with our heuristic for differentiating zlib-ng from zlib. In theory, a system-level zlib-1.3.2 could cause an expected zlib-ng autobuild to surprisingly fail to occur. (But I think it would also allow those fearful of picking up zlib-ng to point ZLIB_ROOT to the system).

In any case, -DOpenImageIO_BUILD_LOCAL_DEPS=zlib should reliably behave as expected -- we can always force the build system to favor whatever we autobuild. It's important to note that whether zlib-ng or zlib is autobuilt is governed by the *_ZLIBNG options, rather than the value (i.e., "zlib" vs "zlib-ng") passed to the *_DEPS options.

)
else ()
checked_find_package (ZLIB REQUIRED) # Needed by several packages
endif ()

# Help set up this target for libtiff config file when using static libtiff
if (NOT TARGET CMath::CMath)
Expand Down
Loading