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
zachlewis wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
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 @@ -76,6 +76,8 @@ LINKSTATIC = "1"
# Standardize the install directory for libraries, as expected by
# other parts of the wheels build process.
CMAKE_INSTALL_LIBDIR = "lib"
# Always build against zlib-ng (instead of regular zlib)
DISABLE_ZLIB = "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,19 +3,81 @@
# 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

@lgritz lgritz Feb 15, 2025

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

@zachlewis zachlewis Feb 17, 2025

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

@lgritz lgritz Feb 17, 2025

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

@zachlewis zachlewis Feb 17, 2025

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

@zachlewis zachlewis Feb 21, 2025

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}
Expand All @@ -26,16 +88,49 @@ build_dependency_with_cmake(ZLIB
# Fix for zlib breaking against cmake 4.0.
# Remove when zlib is fixed to declare its own minimum high enough.
-D CMAKE_POLICY_VERSION_MINIMUM=3.5
${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()
4 changes: 1 addition & 3 deletions 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 Expand Up @@ -45,8 +45,6 @@ build_dependency_with_cmake(minizip-ng
-D MZ_ICONV=OFF
-D MZ_FETCH_LIBS=OFF
-D MZ_FORCE_FETCH_LIBS=OFF
-D ZLIB_LIBRARY=${ZLIB_LIBRARIES}
-D ZLIB_INCLUDE_DIR=${ZLIB_INCLUDE_DIRS}
)


Expand Down
23 changes: 22 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,28 @@ 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
# ZLIB
# We prefer zlib-ng to regular zlib, but we can use either.

check_is_enabled(ZLIBNG ENABLE_ZLIBNG)
check_is_enabled(ZLIB ALLOW_ZLIB)

if (ENABLE_ZLIBNG)
# For zlib <= 1.3.1, we can differentiate between whether
# find_package(ZLIB ...) finds zlib-ng or zlib with the CONFIG
# or PREFER_CONFIG options.
if (ALLOW_ZLIB)
# Prefer zlib-ng if it's available, but fail over to zlib if it isn't.
checked_find_package (ZLIB PREFER_CONFIG REQUIRED)
else ()
checked_find_package (ZLIB CONFIG REQUIRED)
endif ()
else ()
# Starting v1.3.2, zlib will export CMake configs; so once zlib-1.3.2+
# starts shipping with distros, we will have to re-evaluate how (and if)
# we support "ignoring" compatibility-mode-zlib-ng in favor of system zlib.
checked_find_package (ZLIB REQUIRED)
endif ()

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