Skip to content

BUG: Remove hardcoded HDF5 system paths from ITKTargets#6494

Merged
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:hdf5_system_path
Jun 24, 2026
Merged

BUG: Remove hardcoded HDF5 system paths from ITKTargets#6494
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:hdf5_system_path

Conversation

@blowekamp

Copy link
Copy Markdown
Member

Two fixes to the ITK_USE_SYSTEM_HDF5 path in ITKHDF5 CMakeLists:

  1. Scope ITKHDF5_SYSTEM_INCLUDE_DIRS to the variable-fallback branch only — imported targets carry their own include interface and do not need baked-in paths in the installed config files.
  2. Consolidate the duplicated shared/static target-detection branches using a single _hdf5_type suffix variable.
  3. Replace the unreachable variable-fallback branch with a FATAL_ERROR: CMake 3.19+ FindHDF5 always provides hdf5::hdf5 imported targets, and ITK requires CMake ≥ 3.22.

Companion to #6491, #6492, and #6493 which applied the same imported-target pattern to ZLIB, Expat, JPEG, PNG, and double-conversion.

@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
@blowekamp blowekamp marked this pull request as ready for review June 23, 2026 13:44
@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR consolidates the duplicated BUILD_SHARED_LIBS branches in the ITK_USE_SYSTEM_HDF5 path into a single _hdf5_type suffix variable, removes ITKHDF5_SYSTEM_INCLUDE_DIRS so that hardcoded HDF5 include paths are no longer baked into the installed ITKTargets files, and converts the old variable-fallback branch into a FATAL_ERROR.

  • Target-detection refactor: A _hdf5_type variable (-static or empty) is used to select targets, collapsing two near-identical blocks into one; however, the shared-build case (_hdf5_type = \"\") makes elseif(TARGET hdf5::hdf5) (line 56) unreachable and silently drops the hdf5::hdf5-shared check that the original code covered.
  • ITKHDF5_SYSTEM_INCLUDE_DIRS removal: Correct for modern imported-target installs where INTERFACE_INCLUDE_DIRECTORIES is set on the target itself; avoids absolute host paths appearing in downstream ITKTargets files.
  • FATAL_ERROR guard: Replaces the old raw-variable fallback branch, clearly signalling misconfiguration instead of silently building against mismatched libraries.

Confidence Score: 3/5

The shared-build target-detection path now skips hdf5::hdf5-shared, which is the target name exported by HDF5's own HDF5Config.cmake in versions 1.10-1.13; those configurations would hit the new FATAL_ERROR instead of configuring successfully.

The _hdf5_type consolidation leaves _hdf5_type empty for shared builds, causing the first branch to check hdf5::hdf5 and the third branch (elseif TARGET hdf5::hdf5) to become unreachable dead code. The original second check for hdf5::hdf5-shared is gone entirely, so any shared-library build against HDF5 1.10-1.13 in config mode will abort at configure time where it previously worked.

Modules/ThirdParty/HDF5/CMakeLists.txt - specifically the shared-build branch of the target-detection block (lines 36-70).

Important Files Changed

Filename Overview
Modules/ThirdParty/HDF5/CMakeLists.txt Consolidates duplicated shared/static target-detection into a single _hdf5_type suffix variable, removes ITKHDF5_SYSTEM_INCLUDE_DIRS (correct for imported-target installs), and replaces the variable-fallback branch with FATAL_ERROR; but the shared build path silently drops the hdf5::hdf5-shared target check, leaving elseif(TARGET hdf5::hdf5) unreachable in shared mode and breaking HDF5 1.10–1.13 config-mode installs.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ITK_USE_SYSTEM_HDF5 ON] --> B{BUILD_SHARED_LIBS?}
    B -- ON --> C["_hdf5_type = empty string"]
    B -- OFF --> D["_hdf5_type = '-static'"]

    C --> E{"TARGET hdf5::hdf5"}
    D --> F{"TARGET hdf5::hdf5-static"}

    E -- yes --> G["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp / hdf5::hdf5 / ..."]
    E -- no --> H{"TARGET hdf5"}
    H -- yes --> I["ITKHDF5_LIBRARIES = hdf5_cpp / hdf5 / ..."]
    H -- no --> J["elseif TARGET hdf5::hdf5 - DEAD CODE in shared mode"]
    J -- never reached --> K["FATAL_ERROR"]

    F -- yes --> L["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp-static / hdf5::hdf5-static / ..."]
    F -- no --> M{"TARGET hdf5-static"}
    M -- yes --> N["ITKHDF5_LIBRARIES = hdf5_cpp-static / hdf5-static / ..."]
    M -- no --> O{"TARGET hdf5::hdf5 (generic fallback)"}
    O -- yes --> P["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp / hdf5::hdf5 / ..."]
    O -- no --> Q["FATAL_ERROR"]

    style J fill:#ff9999,stroke:#cc0000
    style K fill:#ff9999,stroke:#cc0000
    style Q fill:#ffcc99,stroke:#cc6600
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"}}}%%
flowchart TD
    A[ITK_USE_SYSTEM_HDF5 ON] --> B{BUILD_SHARED_LIBS?}
    B -- ON --> C["_hdf5_type = empty string"]
    B -- OFF --> D["_hdf5_type = '-static'"]

    C --> E{"TARGET hdf5::hdf5"}
    D --> F{"TARGET hdf5::hdf5-static"}

    E -- yes --> G["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp / hdf5::hdf5 / ..."]
    E -- no --> H{"TARGET hdf5"}
    H -- yes --> I["ITKHDF5_LIBRARIES = hdf5_cpp / hdf5 / ..."]
    H -- no --> J["elseif TARGET hdf5::hdf5 - DEAD CODE in shared mode"]
    J -- never reached --> K["FATAL_ERROR"]

    F -- yes --> L["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp-static / hdf5::hdf5-static / ..."]
    F -- no --> M{"TARGET hdf5-static"}
    M -- yes --> N["ITKHDF5_LIBRARIES = hdf5_cpp-static / hdf5-static / ..."]
    M -- no --> O{"TARGET hdf5::hdf5 (generic fallback)"}
    O -- yes --> P["ITKHDF5_LIBRARIES = hdf5::hdf5_cpp / hdf5::hdf5 / ..."]
    O -- no --> Q["FATAL_ERROR"]

    style J fill:#ff9999,stroke:#cc0000
    style K fill:#ff9999,stroke:#cc0000
    style Q fill:#ffcc99,stroke:#cc6600
Loading

Reviews (1): Last reviewed commit: "BUG: Require HDF5 imported targets; erro..." | Re-trigger Greptile

Comment thread Modules/ThirdParty/HDF5/CMakeLists.txt Outdated
Scope ITKHDF5_SYSTEM_INCLUDE_DIRS to the variable-fallback branch so
it is only set when no imported target is available. Imported targets
carry their own include interface and do not need baked-in paths.

Also consolidate the shared/static target-detection branches using a
single _hdf5_type suffix variable.
CMake 3.19+ FindHDF5 always provides hdf5::hdf5 imported targets.
Since ITK requires CMake >= 3.22, the variable-only fallback path
is unreachable on any supported configuration. Replace it with a
fatal error directing users to set HDF5_DIR.
@hjmjohnson hjmjohnson merged commit 129c231 into InsightSoftwareConsortium:main Jun 24, 2026
20 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