Skip to content

COMP: Do not require applications to rediscover zlib.#6489

Closed
jakeforster wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
jakeforster:retain-zlib
Closed

COMP: Do not require applications to rediscover zlib.#6489
jakeforster wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
jakeforster:retain-zlib

Conversation

@jakeforster

Copy link
Copy Markdown
Contributor

'find_package(ITK REQUIRED)' no longer calls 'find_package(ZLIB REQUIRED)'. ITK::ITKZLIBModule already exports the library and include directory of the zlib used by ITK, so rely on that.

  • Modules/ThirdParty/ZLIB/CMakeLists.txt (ITKZLIB_EXPORT_CODE_INSTALL): Make empty.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

'find_package(ITK REQUIRED)' no longer calls 'find_package(ZLIB REQUIRED)'.
ITK::ITKZLIBModule already exports the library and include directory of the
zlib used by ITK, so rely on that.

* Modules/ThirdParty/ZLIB/CMakeLists.txt (ITKZLIB_EXPORT_CODE_INSTALL):
Make empty.
@github-actions github-actions Bot added type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels Jun 22, 2026
@greptile-apps

This comment was marked as resolved.

Comment thread Modules/ThirdParty/ZLIB/CMakeLists.txt

@hjmjohnson hjmjohnson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting one change before this lands: also empty ITKZLIB_EXPORT_CODE_BUILD, so build-tree and install-tree consumers see the same CMake environment (resolves greptile's P2). The build-tree find_package(ZLIB) is redundant for the same reason the install-tree one is — ITK::ITKZLIBModule already carries the absolute zlib library path and system include dirs.

I verified downstream impact empirically (system-zlib ITK, build tree and install tree). Net: safe for every standard consumption idiom; the only hypothetical consumer affected is one that references ZLIB::ZLIB without its own find_package(ZLIB)(no instances found).

Downstream consumer matrix (vs main baseline)
consumer idiom real-world vs baseline
find_package(ITK) + ${ITK_LIBRARIES} BioCell, Cleaver, HASI, RTK, remotes same (pass)
+ include(${ITK_USE_FILE}) c3d, elastix, LesionSizingToolkit same (pass)
find_package(ITK NO_MODULE) IGSIO same (pass)
find_package(ITK COMPONENTS …) elastix, RTK same (pass)
target_link_libraries(ITK::ITKZLIBModule) modern same (pass)
links ZLIB::ZLIB, no own find_package(ZLIB) piggyback (discouraged) install-tree configure fails now; emptying the build block too makes it symmetric in both trees

ITK_USE_SYSTEM_ZLIB=OFF (bundled zlib) is unaffected.

Note: ITK CI builds ITK_USE_SYSTEM_ZLIB=OFF, so CI does not exercise this code path — the matrix above is the evidence. Please also update the commit trailer to name both variables.

Comment thread Modules/ThirdParty/ZLIB/CMakeLists.txt
@blowekamp blowekamp self-requested a review June 22, 2026 13:00
@blowekamp

blowekamp commented Jun 22, 2026

Copy link
Copy Markdown
Member

In a pull request @bradking (which I can't find) recommended doing the find_package but not embedding the 3rd party libraries paths in the export code, and only doing the find package. It is my understanding that this is the best practice not include these absolute package paths to facilitate relocatable packages.

I need to look at this a bit closer.

@blowekamp

Copy link
Copy Markdown
Member

I see the following in my ITKTargets.cmake:


# Create imported target ITK::ITKZLIBModule
add_library(ITK::ITKZLIBModule INTERFACE IMPORTED)

set_target_properties(ITK::ITKZLIBModule PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "/scratch/itk/build-system/Modules/ThirdParty/ZLIB/src;/usr/include"
  INTERFACE_LINK_LIBRARIES "/usr/lib/x86_64-linux-gnu/libz.so"
)

The real bug is that this should refer to the ZLIB::ZLIB interface and not these hard coded path. I'll look into this.

@blowekamp blowekamp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the incorrect solution to the hard coded paths in ITKTargets.cmake.

@hjmjohnson

Copy link
Copy Markdown
Member

Superceeded. Thank you for identifying the problem and proposing a solution.

@hjmjohnson hjmjohnson closed this Jun 22, 2026
@jakeforster

Copy link
Copy Markdown
Contributor Author

Hi @hjmjohnson , @blowekamp

This moved quickly! I would have appreciated an opportunity to respond.

With my package manager hat on, I would like to make the case for a new CMake variable that causes paths to dependencies to be retained in the installed CMake configuration, or a similar functionality. The last time I saw a program take this approach was Geant4, and they provide a variable (GEANT4_INSTALL_PACKAGE_CACHE) to make it possible to retain the paths of dependencies. Here is an excerpt from their documentation:

GEANT4_INSTALL_PACKAGE_CACHE : (DEFAULT : OFF)

If set to ON, install a file containing the locations of external dependencies as used when building this install of Geant4.

If you are packaging (e.g. rpm, deb, spack, Homebrew, etc) Geant4, it is not recommended to set this option in order to make the package relocatable. Your install of Geant4 will then rely on CMAKE_PREFIX_PATH and [other variables used by CMake](https://cmake.org/cmake/help/v3.16/command/find_package.html) to re-find any needed dependencies. Alternately, if your packaging system allows install-time patching, you may install the cache file and patch it at that time with known paths to the dependencies at that point. In both cases, it is your responsibility to setup/patch the environment/cache file correctly and as appropriate.

WARNING: This option is only intended for the packaging case outlined above, and is not recommended for single-user installs of Geant4.

Retaining the paths to dependencies has a couple of advantages when ITK is provided as a package by a package manager. For one, when a user wants to use ITK in their application with find_package(ITK), they only need to install ITK with their package manager and it will "just work". With the current approach, they will have to repeatedly attempt to build their project, each time discovering a new dependency of ITK that they need to install separately from their package manager. Secondly, without retaining the paths, the package manager has no way to associate ITK's dependencies with ITK. That means that when the user uninstalls ITK, or they update ITK and the new version has dropped some dependencies, the package manager cannot uninstall the dependencies for them if they are not being used elsewhere. The onus is placed on the user to do what should be the package manager's job.

As an aside, regarding portability of the install tree when dependency paths are stripped: are you concerned about API/ABI compatibility? Shouldn't all of the find_package calls in ITK specify a valid version range? Even then, you are relying on the dependencies following semantic versioning. As a user, I want to be confident that my ITK is in working order, and I know that the dependencies that were used to build ITK are the ones that ITK used when it passed its test suite. Apologies if I'm missing something.

Thanks for your time.
Jake

@blowekamp

Copy link
Copy Markdown
Member

Hi @jakeforster, apologies for the rapid close. While reviewing the INTERFACE_LINK_LIBRARIES and INTERFACE_INCLUDE_DIRECTORIES entries in ITKTargets.cmake we found build-host absolute paths baked in across many third-party libraries, along with over-linking issues, and decided to address them as a batch. PR #6491 (and follow-ons #6492, #6494) fix this by linking against the standard imported targets (e.g. ZLIB::ZLIB) while retaining the find_package call in the export blocks, so the consumer's CMake resolves the dependency against its own install tree rather than the build host's.

CMake's guidance. The Creating Relocatable Packages section of the official CMake guide is explicit:

"It is not advisable to populate any properties which may contain paths, such as INTERFACE_INCLUDE_DIRECTORIES or INTERFACE_LINK_LIBRARIES, with paths relevant to dependencies. ... This would create a package with hard-coded paths to dependencies not suitable for relocation. Ideally such dependencies should be used through their own IMPORTED targets ... the consumer will run the appropriate find_package() commands to find the dependencies and populate the imported targets with appropriate paths on their own machine."

On the package manager concern. Dependency tracking and version compatibility are responsibilities of the package manager, not CMake config files. Spack, conda-forge, Homebrew, and apt all model transitive dependencies in their own metadata and arrange for CMAKE_PREFIX_PATH or _DIR hints at install time so find_package succeeds. Embedding absolute paths in CMake config files actively conflicts with how those tools relocate packages — which is the root of the problem your PR identified. ABI/version pinning likewise belongs in the package manager's dependency solver, not replicated in CMake.

On the GEANT4_INSTALL_PACKAGE_CACHE idea. Worth noting that Geant4 itself marks this option OFF by default and explicitly warns it is not recommended for standard installs — only for specific packaging workflows. An opt-in variable of this kind is not unreasonable in principle, but we are not going to implement it now. If you'd like to track it, please open a GitHub issue.

Thanks for identifying the problem.

@jakeforster

Copy link
Copy Markdown
Contributor Author

Hi @blowekamp

Thanks for the explanation. No worries if such an option isn't a priority. I'll just explain a bit more about the motivation and where I'm coming from, in case it isn't obvious and this is revisited at some point.

A package manager will probably configure the ITK package to use some system libraries (e.g. openjpeg) to conserve the user's disk space. And I think we are on the same page in thinking that a package manager should allow the user to:

  1. Install ITK.
  2. Configure CMake on a project that includes find_package(ITK REQUIRED) and have it succeed.

Currently, for this to work, the package manager must "propagate" some of the inputs of ITK into the user's "profile" or "environment". Otherwise, when find_package(ITK REQUIRED) invokes e.g. find_package(openjpeg), it would fail. This is suboptimal, as the user has not asked for e.g. openjpeg, yet the opj_dump binary from openjpeg is now in their PATH -- at least, that is how propagated inputs work in the Guix package manager. Since propagated inputs "pollute" the user environment, we prefer to avoid it. Guix (and I assume Nix too) can avoid propagating inputs if references (full paths) to these build inputs are present in the installed CMake configuration.

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

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants