Skip to content

BUG: Use ZLIB::ZLIB imported target for system ZLIB#6491

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:zlib_system_path
Jun 22, 2026
Merged

BUG: Use ZLIB::ZLIB imported target for system ZLIB#6491
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:zlib_system_path

Conversation

@blowekamp

@blowekamp blowekamp commented Jun 22, 2026

Copy link
Copy Markdown
Member

Replace hardcoded ZLIB paths in ITKTargets and module config files with the ZLIB::ZLIB imported target. This prevents build-host paths from being baked into installed CMake config files.

Supersedes #6489 (incorrect fix — that PR relied on hardcoded paths rather than relying on the imported interface).

Replace hardcoded ZLIB paths in ITKTargets and module config with
the ZLIB::ZLIB imported target. This avoids baking build-host
paths into installed CMake config files.
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels Jun 22, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Ran the synthetic downstream-consumer matrix (6 CMake-consumption idioms × build-tree/install-tree) against this PR, main, and #6489. This PR is identical to main in all 12 cells — it modernizes to the ZLIB::ZLIB imported target without reproducing the install-tree regression that #6489 introduced (a consumer linking ZLIB::ZLIB with no own find_package(ZLIB) failed to configure against the install tree). The difference is that this PR keeps find_package(ZLIB) in both export blocks, where #6489 emptied ITKZLIB_EXPORT_CODE_INSTALL.

Minor, non-blocking nit: the PR drops the set(ZLIB_ROOT …) hint from ITKZLIB_EXPORT_CODE_BUILD but leaves the identical hint in ITKZLIB_EXPORT_CODE_INSTALL, so the two generated export blocks are now slightly asymmetric. Harmless (the install-export hint just pins the consumer to the same zlib ITK used), just flagging for consistency.

Differential matrix — this PR (D) vs main (C) vs #6489 (A)

System ZLIB ON, artifact-scored (a consumer PASSES iff its executable exists on disk):

consumer tree C: main A: #6489 D: this PR
modern_libraries build / install PASS PASS PASS
legacy_usefile build / install PASS PASS PASS
config_nomodule build / install PASS PASS PASS
direct_target build / install PASS PASS PASS
components install PASS PASS PASS
components build FAIL-build¹ FAIL-build¹ FAIL-build¹
naive_thirdparty build PASS PASS PASS
naive_thirdparty install PASS FAIL-configure#6489's regression PASS

¹ Documented harness constant — a COMPONENTS-restricted consumer doesn't get the deep-transitive itk_zlib.h on its include path in the build tree; identical across all three variants, so it cancels in the differential. The install tree exports it fine.

naive_thirdparty is the canary: it links ZLIB::ZLIB with no own find_package(ZLIB), relying on ITK to inject the imported target. It is the only cell that moved between variants — and this PR restores it to the main baseline on both trees.

Method & reproduction

Minimal ITK built twice (-DITK_USE_SYSTEM_ZLIB=ON/OFF, -DITK_BUILD_DEFAULT_MODULES=OFF -DModule_ITKIOPNG=ON), installed, then a family of 6 fake consumers swept against both the build tree and the install tree. Export code is generated at configure time, so each variant only needs a reconfigure + reinstall (no recompile): drop the variant Modules/ThirdParty/ZLIB/CMakeLists.txt in, cmake <build-dir>, cmake --install, sweep.

The self-contained harness used to produce this (driver + 6 consumer projects + the C/A/D orchestrator) is attached to this comment as a tarball for reproduction.

Caveat: this minimal sweep exercises the ImageIO/PNG path, not DCMTK (EXCLUDE_FROM_DEFAULT), which forwards ITKZLIB_LIBRARIES into an external-project -DZLIB_LIBRARIES= string. That path is unchanged in spirit here, but a full forest build would be the belt-and-suspenders confirmation for the ZLIB::ZLIB-as-string-to-subproject case.

@hjmjohnson

Copy link
Copy Markdown
Member

The skill used to run the downstream simulation tests:

itk-test-cmake-changes-downstream-skill.tar.gz

@blowekamp blowekamp marked this pull request as ready for review June 22, 2026 19:34
@greptile-apps

greptile-apps Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where build-host ZLIB paths were baked into ITK's installed CMake config files (ITKTargets and module config files) by switching from evaluated variable expansion (${ZLIB_LIBRARIES}, ${ZLIB_INCLUDE_DIRS}, ${ZLIB_ROOT}) to the ZLIB::ZLIB imported target.

  • ITKZLIB_LIBRARIES is now set to the string \"ZLIB::ZLIB\" instead of \"${ZLIB_LIBRARIES}\", so downstream consumers resolve ZLIB via the imported target rather than a build-machine path.
  • ITKZLIB_SYSTEM_INCLUDE_DIRS is removed; include directories are now carried automatically by the ZLIB::ZLIB target's INTERFACE_INCLUDE_DIRECTORIES, which is the correct modern CMake idiom.
  • ZLIB_ROOT is removed from both ITKZLIB_EXPORT_CODE_INSTALL and ITKZLIB_EXPORT_CODE_BUILD, letting downstream find_package(ZLIB REQUIRED) locate ZLIB on the consuming machine rather than relying on the build host's path.

Confidence Score: 5/5

Safe to merge; the change is a minimal, targeted fix that removes build-host path contamination from ITK's installed CMake config files.

The one changed file replaces evaluated variable expansion with the ZLIB::ZLIB imported target. ITKModuleMacros.cmake explicitly handles namespaced targets (line 190 regex check), so the new string value is routed correctly at module configuration time. Removing ITKZLIB_SYSTEM_INCLUDE_DIRS is correct because ZLIB::ZLIB carries INTERFACE_INCLUDE_DIRECTORIES automatically. The ZLIB_ROOT removal from export code is the actual bug fix — it stops a build-machine path from being written into installed config files. No untested edge cases were introduced.

No files require special attention.

Important Files Changed

Filename Overview
Modules/ThirdParty/ZLIB/CMakeLists.txt Switches system ZLIB integration from hardcoded variable expansion to the ZLIB::ZLIB imported target, removing build-host path contamination in installed CMake config files. ITKModuleMacros.cmake already handles namespaced targets (matching ^(.*)::(.*)$) at line 190, so the new value is used correctly.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant BuildHost as Build Host (ITK configure)
    participant ExportCode as Installed ITKConfig.cmake
    participant Consumer as Downstream Consumer

    Note over BuildHost: ITK_USE_SYSTEM_ZLIB=ON

    rect rgb(255, 200, 200)
        Note over BuildHost,Consumer: Before (broken): build-host paths baked in
        BuildHost->>ExportCode: set(ZLIB_ROOT "/build/host/path") + find_package(ZLIB REQUIRED)
        ExportCode-->>Consumer: Hardcoded /build/host/path/libz.so — breaks on different machine
    end

    rect rgb(200, 255, 200)
        Note over BuildHost,Consumer: After (this PR): portable imported target
        BuildHost->>ExportCode: "find_package(ZLIB REQUIRED), ITKZLIB_LIBRARIES = ZLIB::ZLIB"
        ExportCode->>Consumer: find_package(ZLIB REQUIRED)
        Consumer->>Consumer: Resolves ZLIB::ZLIB on own system — works portably
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant BuildHost as Build Host (ITK configure)
    participant ExportCode as Installed ITKConfig.cmake
    participant Consumer as Downstream Consumer

    Note over BuildHost: ITK_USE_SYSTEM_ZLIB=ON

    rect rgb(255, 200, 200)
        Note over BuildHost,Consumer: Before (broken): build-host paths baked in
        BuildHost->>ExportCode: set(ZLIB_ROOT "/build/host/path") + find_package(ZLIB REQUIRED)
        ExportCode-->>Consumer: Hardcoded /build/host/path/libz.so — breaks on different machine
    end

    rect rgb(200, 255, 200)
        Note over BuildHost,Consumer: After (this PR): portable imported target
        BuildHost->>ExportCode: "find_package(ZLIB REQUIRED), ITKZLIB_LIBRARIES = ZLIB::ZLIB"
        ExportCode->>Consumer: find_package(ZLIB REQUIRED)
        Consumer->>Consumer: Resolves ZLIB::ZLIB on own system — works portably
    end
Loading

Reviews (1): Last reviewed commit: "BUG: Use ZLIB::ZLIB imported target for ..." | Re-trigger Greptile

@hjmjohnson hjmjohnson merged commit a5e5aab into InsightSoftwareConsortium:main Jun 22, 2026
22 checks passed
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:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances 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