[core] Make zlib a builtin obtained from the web and remove the vendored code#21613
Open
dpiparo wants to merge 2 commits intoroot-project:masterfrom
Open
[core] Make zlib a builtin obtained from the web and remove the vendored code#21613dpiparo wants to merge 2 commits intoroot-project:masterfrom
dpiparo wants to merge 2 commits intoroot-project:masterfrom
Conversation
Member
|
Closing to rerun with clean builds |
Test Results 22 files 22 suites 3d 2h 42m 49s ⏱️ For more details on these failures, see this check. Results for commit 4126363. ♻️ This comment has been updated with latest results. |
hageboeck
reviewed
Mar 16, 2026
builtins/zlib/CMakeLists.txt
Outdated
| set(ZLIB_VERSION_TWEAK "${CMAKE_MATCH_1}") | ||
| string(APPEND ZLIB_VERSION_STRING ".${ZLIB_VERSION_TWEAK}") | ||
| endif() | ||
| set(ZLIB_TARGET ZLIB) |
Member
There was a problem hiding this comment.
Presumably unnecessary. (Doesn't seem to be used anywhere):
Suggested change
| set(ZLIB_TARGET ZLIB) |
builtins/zlib/CMakeLists.txt
Outdated
Comment on lines
16
to
21
| set(ROOT_ZLIB_LIBRARY_BASE ${ROOT_ZLIB_PREFIX}/lib) | ||
| if(MSVC) | ||
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY_BASE}/z-static${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
| else() | ||
| add_library(ZLIB STATIC ${ZLIB_PUBLIC_HEADERS} ${ZLIB_PRIVATE_HEADERS} ${ZLIB_SOURCES}) | ||
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY_BASE}/${CMAKE_STATIC_LIBRARY_PREFIX}z${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
| endif() |
Member
There was a problem hiding this comment.
Suggested change
| set(ROOT_ZLIB_LIBRARY_BASE ${ROOT_ZLIB_PREFIX}/lib) | |
| if(MSVC) | |
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY_BASE}/z-static${CMAKE_STATIC_LIBRARY_SUFFIX}) | |
| else() | |
| add_library(ZLIB STATIC ${ZLIB_PUBLIC_HEADERS} ${ZLIB_PRIVATE_HEADERS} ${ZLIB_SOURCES}) | |
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY_BASE}/${CMAKE_STATIC_LIBRARY_PREFIX}z${CMAKE_STATIC_LIBRARY_SUFFIX}) | |
| endif() | |
| set(ROOT_ZLIB_LIBRARY_BASE ) | |
| if(MSVC) | |
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_PREFIX}/lib/z-static${CMAKE_STATIC_LIBRARY_SUFFIX}) | |
| else() | |
| set(ROOT_ZLIB_LIBRARY ${ROOT_ZLIB_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}z${CMAKE_STATIC_LIBRARY_SUFFIX}) | |
| endif() |
builtins/zlib/CMakeLists.txt
Outdated
| BUILD_COMMAND ${CMAKE_COMMAND} --build . --config $<CONFIG> | ||
| INSTALL_COMMAND ${CMAKE_COMMAND} --install . --config $<CONFIG> | ||
| LOG_DOWNLOAD 1 LOG_CONFIGURE 1 LOG_BUILD 1 LOG_INSTALL 1 LOG_OUTPUT_ON_FAILURE 1 | ||
| BUILD_IN_SOURCE 1 |
Member
There was a problem hiding this comment.
Consider removing this:
Suggested change
| BUILD_IN_SOURCE 1 |
builtins/zlib/CMakeLists.txt
Outdated
| if(DEFINED ZLIB_LIBRARY_RELEASE) | ||
| set(ZLIB_LIBRARY_RELEASE ${ZLIB_LIBRARY} CACHE INTERNAL "") | ||
| endif() | ||
| set(ZLIB_INCLUDE_DIR ${ROOT_ZLIB_LIBRARY_BASE}/include) |
Member
There was a problem hiding this comment.
Better to tie everything to the prefix:
Suggested change
| set(ZLIB_INCLUDE_DIR ${ROOT_ZLIB_LIBRARY_BASE}/include) | |
| set(ZLIB_INCLUDE_DIR ${ROOT_ZLIB_PREFIX}/include) |
Member
Author
There was a problem hiding this comment.
This was a bug. The path did not make sense. Thanks for pointing that out!
| set(ZLIB_INCLUDE_DIR ${ROOT_ZLIB_LIBRARY_BASE}/include) | ||
| file(MAKE_DIRECTORY ${ZLIB_INCLUDE_DIR}) | ||
|
|
||
| set_property(GLOBAL APPEND PROPERTY ROOT_BUILTIN_TARGETS ZLIB::ZLIB) |
Member
There was a problem hiding this comment.
This is used to make all builtins build before the rest of ROOT. It could be replaced by some correctly crafted add_dependencies, but it's easier to simply build everything before starting with LLVM.
| list(APPEND ROOT_BUILTINS ZLIB) | ||
| add_subdirectory(builtins/zlib) | ||
| get_target_property(ZLIB_INCLUDE_DIR ZLIB::ZLIB INTERFACE_INCLUDE_DIRECTORIES) | ||
| get_target_property(ZLIB_LIBRARY_LOCATION ZLIB::ZLIB IMPORTED_LOCATION) |
Member
There was a problem hiding this comment.
This seems to be unused up to and including this commit. The canonical names of the find module would be:
ZLIB_INCLUDE_DIRS
ZLIB_LIBRARIES
we should also set ZLIB_FOUND, so subsequent attempts at find_package don't try again.
Member
Author
There was a problem hiding this comment.
this is true for all builtins, yes?
core/zip/CMakeLists.txt
Outdated
Comment on lines
+18
to
+21
| if(builtin_zlib) | ||
| add_dependencies(Core BUILTIN_ZLIB) | ||
| endif() | ||
|
|
Member
There was a problem hiding this comment.
Maybe this would better be specified as
add_dependencies(ZLIB BUILTIN_ZLIB) in builtins/zlib/CMakeLists.txt
| add_library(ZLIB STATIC IMPORTED GLOBAL) | ||
| add_library(ZLIB::ZLIB ALIAS ZLIB) | ||
| target_include_directories(ZLIB INTERFACE ${ZLIB_INCLUDE_DIR}) | ||
| set_target_properties(ZLIB PROPERTIES IMPORTED_LOCATION ${ROOT_ZLIB_LIBRARY}) |
Member
There was a problem hiding this comment.
Without adding it to all builtin targets (see other comment), one should add:
Suggested change
| set_target_properties(ZLIB PROPERTIES IMPORTED_LOCATION ${ROOT_ZLIB_LIBRARY}) | |
| set_target_properties(ZLIB PROPERTIES IMPORTED_LOCATION ${ROOT_ZLIB_LIBRARY}) | |
| add_dependencies(ZLIB BUILTIN_ZLIB) |
builtins/libpng/CMakeLists.txt
Outdated
Comment on lines
+41
to
+42
| -DZLIB_INCLUDE_DIR=${ZLIB_INCLUDE_DIR} | ||
| -DZLIB_LIBRARY=${ZLIB_LIBRARY_LOCATION} |
Member
There was a problem hiding this comment.
OK, I see now that it's used, but isn't it better to use the canonical names?
https://cmake.org/cmake/help/latest/module/FindZLIB.html
Or was the idea to specifically not use these?
Member
Author
Summary of the test failures Using zlib-ng 2.3.3
193 - test-stress (Failed)1336 - roottest-root-io-filemerger-merged-zlib (Failed)Error in : Disk size of mzlibfile1-4.root should have been 5014 but is 5384 (tolerance 20 bytes) 1361 - roottest-root-io-filemerger-simple-default-compr-level9 (Failed)Error in : Disk size of hsimple9.root should have been 431015 but is 433535 (tolerance 25 bytes) 1362 - roottest-root-io-filemerger-simple-zlib-compr-level1 (Failed)Error in : Disk size of hsimple101.root should have been 415025 but is 518165 (tolerance 25 bytes) 1363 - roottest-root-io-filemerger-simple-zlib-compr-level6 (Failed)Error in : Disk size of hsimple106.root should have been 431303 but is 441579 (tolerance 25 bytes) 1364 - roottest-root-io-filemerger-simple-zlib-compr-level9 (Failed)Error in : Disk size of hsimple109.root should have been 431024 but is 433545 (tolerance 25 bytes) 1365 - roottest-root-io-filemerger-simplex2-default-compr-level9 (Failed)Error in : Disk size of hsimple9x2.root should have been 850127 but is 855171 (tolerance 30 bytes) 1366 - roottest-root-io-filemerger-simplex2-zlib-compr-level9 (Failed)Error in : Disk size of hsimple109x2.root should have been 850136 but is 855177 (tolerance 30 bytes) 1373 - roottest-root-io-filemerger-simplef-default-compr-level9 (Failed)Error in : Disk size of hsimpleF.root should have been 12581462 but is 12649062 (tolerance 30 bytes) 1685 - roottest-root-io-treeForeign-testForeignDraw2328 - roottest-root-tree-fastcloning-execCheckClusterRange3322 - roottest-root-tree-cloning-exectrim3298 - roottest-root-tree-cache-autocache |
dpiparo
force-pushed
the
builtin_zlib
branch
4 times, most recently
from
bf522c0 to
e2ab09d
Compare
And simplify the way it is included in Core, by replicating what is done for lzma. The version of zlib is zlib-ng 2.3.3, compiled in compatibility mode to offer the same interface of zlib.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is similar to previous changes regarding builtin compression libraries
This PR does what was done for lzma by #21571.
ZLib is now fetched in its zlib-ng implementation (default on Alma10) in its version 2.3.3, from LCG, and treated as lzma (and libpng, libgif and libjpeg, and others). Moreover, a dependency to it in builtin libpng had to be added.
What is different with zlib
These changes break several tests, in a way that is known: zlib-ng is already used by default on Fedora43/44 as well as Alma10, all in ROOT's CI. The sizes of several files, not only ROOT files, but also png and pdf images, change: some are larger, some are smaller. Compression rations, obviously, also change and some tests fail since the references are different. The summary of the differences is available below. This will hopefully allow us to reason better about what changes in terms of compression and what measures we can take before merging this PR.
@jblomer @hahnjo