[CMake] Don't override find_package() for builtins#22604
Conversation
guitargeek
added
in:Build System
clean build
guitargeek
changed the title
find_package() for builtins
Test Results 19 files 19 suites 3d 2h 15m 34s ⏱️ For more details on these failures, see this check. Results for commit cbcfc58. ♻️ This comment has been updated with latest results. |
| set(ZLIB_INCLUDE_DIR ${ZLIB_INCLUDE_DIR} CACHE PATH "Path to the builtin zlib headers" FORCE) | ||
| set(ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY} CACHE FILEPATH "Path to the builtin zlib library" FORCE) |
There was a problem hiding this comment.
| set(ZLIB_INCLUDE_DIR ${ZLIB_INCLUDE_DIR} CACHE PATH "Path to the builtin zlib headers" FORCE) | |
| set(ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY} CACHE FILEPATH "Path to the builtin zlib library" FORCE) | |
| set(ZLIB_INCLUDE_DIR ${ZLIB_INCLUDE_DIR} PARENT_SCOPE) | |
| set(ZLIB_LIBRARY ${ROOT_ZLIB_LIBRARY} PARENT_SCOPE) |
for consistency with above?
There was a problem hiding this comment.
Ok I'll try! As long as FindPNG (which does find_package(ZLIB) internally) picks up the builtin ZLIB we're good. We just need to set the right variables so find_package(ZLIB) knows ZLIB is already there, and it will not look for it again and find the system ZLIB.
There was a problem hiding this comment.
Alright, indeed it both works. The FindPNG -> find_package(ZLIB) happens in SearchInstalledSoftware, which is the parent scope. So setting these variables in the PARENT_SCOPE is indeed good enough. The global cache would only be needed if we frigger find_package(ZLIB) in arbitrary places, but that's not the case.
There was a problem hiding this comment.
+1 for doing it only in the parent scope. The cache is already full enough, and it should (in theory) be settable by the user to show ROOT the path to a different zlib.
(I doubt that this applies here, because the builtin is on when this code runs, but staying out of the cache if we can looks reasonable to me.)
ferdymercury
left a comment
ferdymercury
left a comment
There was a problem hiding this comment.
Thanks, just a minor nitpick
| # Pre-seed the singular variables FindZLIB.cmake consumes (its REQUIRED_VARS: | ||
| # ZLIB_INCLUDE_DIR via find_path(), ZLIB_LIBRARY via its if(NOT ZLIB_LIBRARY) | ||
| # guard), so a real find_package(ZLIB) short-circuits the system search and | ||
| # reuses the builtin ZLIB::ZLIB target above. Setting this variable in the | ||
| # PARENT_SCOPE is sufficient, because that's SearchInstalledSoftware.cmake | ||
| # where also the other find_package(ZLIB) calls happen (indirectly via | ||
| # FindPNG). So a CACHE entry is not required. |
There was a problem hiding this comment.
How about:
# The following variables can be set by users to override the behaviour of FindZLIB.cmake,
# so packages executing their own find_package(ZLIB) will find the builtin library.
# We use this to direct the zlib search of FindPNG to the builtin.
There was a problem hiding this comment.
Sure! Since we all agree here that we don't want the CACHE entry, the comment can be less defensive and shortened.
| # When ROOT provides zlib/zstd as builtins, disable them in the bundled LLVM so it | ||
| # cannot discover and link the *system* copy (mixing two versions in one process). | ||
| # We cannot point LLVM at the builtins instead: it probes them with a configure-time | ||
| # compile/link check, but the builtin libraries are only produced later by their | ||
| # ExternalProject, so the check would always fail. |
There was a problem hiding this comment.
OK, but does it still manage to use those libraries if they are disabled?
There was a problem hiding this comment.
It should! Let's verify this by setting LLVM_ENABLE_ZSTD and LLVM_ENABLE_ZLIB to FORCE_ON (LLVMs equivalent to our fail-on-missing that I just learned about 🙂 ):
builtins/zstd defined the imported ZSTD::ZSTD target but never tied it to the BUILTIN_ZSTD ExternalProject that actually produces libzstd.a. As a result a parallel or targeted build (e.g. building only the Core target) could try to link the archive before it exists. Add the dependency edge, consistent with what builtins/zlib already does for BUILTIN_ZLIB. 🤖 Done by AI.
ROOT redefined the global find_package() command to no-op for its builtins, so that sub-projects added later (notably the bundled LLVM) and transitive find modules (e.g. FindPNG -> find_package(ZLIB)) would reuse ROOT's builtin imported targets instead of re-discovering a system copy. This relied on undocumented CMake behaviour and recursed infinitely under tools that override find_package() themselves, such as vcpkg (root-project#8633). The override only ever did real work for ZLIB and zstd: every other name in ROOT_BUILTINS was unnecessary (no find_package() consumer downstream). Both are now handled directly, so the override and the ROOT_BUILTINS list can be removed. This makes ROOT compatible with vcpkg and other dependency providers without changing behaviour: in builtin builds LLVM is still built without zlib/zstd, and a system build is unaffected since find_package() is no longer shadowed. Closes root-project#8633. 🤖 Done with the help of AI, which suggested an initial solution that I refined.
4 participants
Targeted changes to make the
find_packageoverride unnecessary (more detail in inline comments and commit messages).Closes #8633.
🤖 Done with the help of AI, which suggested an initial solution that I refined.