Skip to content

BUG: Use imported targets for system Expat, JPEG, and PNG#6492

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:third_party_system
Jun 23, 2026
Merged

BUG: Use imported targets for system Expat, JPEG, and PNG#6492
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:third_party_system

Conversation

@blowekamp

Copy link
Copy Markdown
Member

Replace hardcoded library paths with EXPAT::EXPAT, JPEG::JPEG, and PNG::PNG imported targets. Add find_package export code so downstream consumers re-find the packages rather than using baked-in paths.

Companion to #6491 which applied the same fix to system ZLIB.

Replace hardcoded library paths with EXPAT::EXPAT, JPEG::JPEG, and
PNG::PNG imported targets. Add find_package export code so downstream
consumers re-find the packages rather than using baked-in paths.
@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

💯 Thanks for looking at the other libraries!

@blowekamp

Copy link
Copy Markdown
Member Author

The removal of the LIBRARY_DIR path's in the export code removes the hard coded paths to support relocatable packages. A prime use case of this is in conda-forge builds, were all the dependencies are install in a temporary location. However, when a library using a build or install ITK, it may now require the repeat of the LIBRARY_DIR paths that were used with ITK's configuration, if the package are no in the CMAKE paths to search.

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

greptile-apps Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces hardcoded library paths (EXPAT_LIBRARY, JPEG_LIBRARIES, PNG_LIBRARIES) and *_SYSTEM_INCLUDE_DIRS variables with their corresponding CMake imported targets (EXPAT::EXPAT, JPEG::JPEG, PNG::PNG) for the system-library branches of the Expat, JPEG, and PNG third-party modules. It also adds EXPORT_CODE_INSTALL / EXPORT_CODE_BUILD blocks so downstream consumers re-run find_package rather than relying on baked-in paths.

  • Expat / JPEG / PNG: ITK_USE_SYSTEM_* paths now set *_LIBRARIES to the canonical imported target and drop *_SYSTEM_INCLUDE_DIRS; include directories are now carried by the imported target's INTERFACE_INCLUDE_DIRECTORIES, which is correct modern CMake practice. All three imported targets (EXPAT::EXPAT since CMake 3.10, JPEG::JPEG since 3.12, PNG::PNG since 3.5) are available within ITK's minimum-required CMake version (3.22.1).
  • GIFTI: EXPAT_LIBRARIES is switched from ${ITKExpat_LIBRARIES} to ITK::ITKExpatModule; the alias target exists in the build tree (created by itk_module_target_export) so the link is valid, though this differs in abstraction level from the raw ITKznz / ITKniftiio targets used for ZLIB and NIFTI in the same file.

Confidence Score: 4/5

Safe to merge; the core imported-target migration is correct and all three targets are available in ITK's minimum CMake version.

The Expat/JPEG/PNG changes are straightforward and mechanically correct — imported targets carry include directories transitively, the export code correctly defers library discovery to downstream consumers, and no build-tree include paths are lost. The only non-trivial touch is GIFTI, where EXPAT_LIBRARIES switches to ITK::ITKExpatModule rather than staying at the same abstraction level as ZLIB and NIFTI; the alias target does exist at build time, but the asymmetry could confuse future maintainers.

Modules/ThirdParty/GIFTI/src/gifticlib/CMakeLists.txt — the switch from ${ITKExpat_LIBRARIES} to ITK::ITKExpatModule is worth a second look for consistency with the ZLIB/NIFTI pattern in the same file.

Important Files Changed

Filename Overview
Modules/ThirdParty/Expat/CMakeLists.txt Replaces raw EXPAT_LIBRARY path and SYSTEM_INCLUDE_DIRS with EXPAT::EXPAT imported target; adds EXPORT_CODE_INSTALL/BUILD for downstream find_package — pattern is correct and consistent with the PR's intent.
Modules/ThirdParty/JPEG/CMakeLists.txt Replaces JPEG_LIBRARIES path and SYSTEM_INCLUDE_DIRS with JPEG::JPEG imported target; adds EXPORT_CODE — identical pattern to Expat, no issues.
Modules/ThirdParty/PNG/CMakeLists.txt Replaces PNG_LIBRARIES path and both PNG_INCLUDE_DIRS/PNG_INCLUDE_DIR system include vars with PNG::PNG imported target; adds EXPORT_CODE — correct, previously dual include-dir entries are now embedded in the imported target.
Modules/ThirdParty/GIFTI/src/gifticlib/CMakeLists.txt Switches EXPAT_LIBRARIES from ${ITKExpat_LIBRARIES} to ITK::ITKExpatModule; functionally correct (alias exists via itk_module_target_export), but diverges from raw-target style used for ZLIB and NIFTI in the same file.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["ITK_USE_SYSTEM_EXPAT / JPEG / PNG ON"] --> B["find_package(EXPAT/JPEG/PNG REQUIRED)"]
    B --> C["Set *_LIBRARIES = EXPAT::EXPAT / JPEG::JPEG / PNG::PNG"]
    C --> D["Set *_NO_SRC = 1"]
    D --> E["Set EXPORT_CODE_INSTALL\nfind_package(X REQUIRED)"]
    D --> F["Set EXPORT_CODE_BUILD\nif(NOT ITK_BINARY_DIR)\n  find_package(X REQUIRED)\nendif()"]
    E --> G["itk_module_impl()"]
    F --> G
    G --> H["ITK::ITKExpatModule / ITK::ITKJPEGModule / ITK::ITKPNGModule\n(interface library aliases)"]
    H --> I["Downstream consumer re-runs\nfind_package via EXPORT_CODE"]
    J["ITK_USE_SYSTEM_EXPAT OFF"] --> K["Set *_LIBRARIES = ITKEXPAT / itkjpeg / itkpng"]
    K --> G
    L["GIFTI (within ITK build)"] --> M["EXPAT_LIBRARIES = ITK::ITKExpatModule"]
    M --> N["target_link_libraries(ITKgiftiio ITK::ITKExpatModule ITKznz ITKniftiio)"]
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_EXPAT / JPEG / PNG ON"] --> B["find_package(EXPAT/JPEG/PNG REQUIRED)"]
    B --> C["Set *_LIBRARIES = EXPAT::EXPAT / JPEG::JPEG / PNG::PNG"]
    C --> D["Set *_NO_SRC = 1"]
    D --> E["Set EXPORT_CODE_INSTALL\nfind_package(X REQUIRED)"]
    D --> F["Set EXPORT_CODE_BUILD\nif(NOT ITK_BINARY_DIR)\n  find_package(X REQUIRED)\nendif()"]
    E --> G["itk_module_impl()"]
    F --> G
    G --> H["ITK::ITKExpatModule / ITK::ITKJPEGModule / ITK::ITKPNGModule\n(interface library aliases)"]
    H --> I["Downstream consumer re-runs\nfind_package via EXPORT_CODE"]
    J["ITK_USE_SYSTEM_EXPAT OFF"] --> K["Set *_LIBRARIES = ITKEXPAT / itkjpeg / itkpng"]
    K --> G
    L["GIFTI (within ITK build)"] --> M["EXPAT_LIBRARIES = ITK::ITKExpatModule"]
    M --> N["target_link_libraries(ITKgiftiio ITK::ITKExpatModule ITKznz ITKniftiio)"]
Loading

Reviews (1): Last reviewed commit: "BUG: Use imported targets for system Exp..." | Re-trigger Greptile

Comment thread Modules/ThirdParty/GIFTI/src/gifticlib/CMakeLists.txt
@hjmjohnson hjmjohnson merged commit 0cba496 into InsightSoftwareConsortium:main Jun 23, 2026
22 checks passed
find_package(JPEG REQUIRED)
set(ITKJPEG_INCLUDE_DIRS ${ITKJPEG_BINARY_DIR}/src)
set(ITKJPEG_SYSTEM_INCLUDE_DIRS ${JPEG_INCLUDE_DIR})
set(ITKJPEG_LIBRARIES "${JPEG_LIBRARIES}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this change caused my ITK build to fail at the configure step with:

-- Found JPEG: /gnu/store/....-libjpeg-turb-3.1.2/lib/libjpeg.so (found version "62")
...
CMake Error at Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat/CMakeLists.txt (target_link_libraries):
  Target "gdcmMSFF" links to:
     JPEG::JPEG

  but the target was not found...

I guess this is because libjpeg-turbo provides libjpeg-turbo::jpeg and libjpeg-turbo::turbojpeg, and ${JPEG_LIBRARIES} is no longer used.

@blowekamp blowekamp Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for testing out the bleeding edge here. I see you are using a system libjpeg-turbo, and ITK's vendored GDCM. It's not clear what version of CMake you are using, bu that may not matter.

The intention of the find_package(JPEG REQUIRED) is to use CMake's findjpeg module which constructs JPEG::JPEG for both jpeg libraries.

I tested locally against the older ubuntu libjpeg-turbo package, and a very recent manual libjpeg-turbo build and install. For the latter I set JPEG_ROOT to the correct path. Both worked as expected.

Perhaps you are configuring things differently? Or there are some additional CMake module in your path?

EDIT: I was able to reproduce now...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a draft PR which should address this issue: #6502

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.

4 participants