Skip to content

COMP: Link namespaced DCMTK targets in the DCMTK IO modules#6543

Merged
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:fix-dcmtk-build-export-namespace
Jul 4, 2026
Merged

COMP: Link namespaced DCMTK targets in the DCMTK IO modules#6543
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:fix-dcmtk-build-export-namespace

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Jul 2, 2026

Copy link
Copy Markdown
Member

Link the DCMTK IO modules against DCMTK::-namespaced targets so the exported link interface resolves for external consumers, and require DCMTK 3.7.0+ for ITK_USE_SYSTEM_DCMTK (the first release whose package config exports those namespaced targets).

Why

Bare dcmdata/dcmimgle/... names survive only inside ITK's own build scope; the exported link interface then carries $<LINK_ONLY:dcmdata>, which no external consumer of the ITK build tree can resolve. The DCMTK:: wrapper targets export as DCMTK::dcmdata, which consumers resolve through find_package(DCMTK).

BUILD_SINGLE_SHARED_LIBRARY is pinned OFF with a plain set() (per review): a single-library build renames the per-library targets with a suffix the DCMTK::<lib> link names cannot match.

Testing

Configured and built Module_ITKIODCMTK + IOTransformDCMTK locally with the vendored DCMTK; all 46 DCMTK tests pass. Also verified the stale-cache scenario from review: with a poisoned BUILD_SINGLE_SHARED_LIBRARY:BOOL=ON cache entry, configure/generate still produce the per-library targets (the plain set() shadows the cache).

The build-tree export repair for the DCMTK::ITK::*Module mis-prefix (formerly the third commit here) is split out to #6548 (draft) for closer inspection, per @blowekamp. Note: #6548 should land with or promptly after this PR — with the namespaced linking alone, an external build-tree consumer fails at generate resolving DCMTK::ITK::*Module (verified repro in #6548; on current main the same consumer instead fails at link time, so no working flow regresses). Install-tree consumers are unaffected.

Match the vendored DCMTK version and guarantee that the system package
config exports DCMTK::-namespaced imported targets, which the DCMTK IO
modules link against.

@dzenanz dzenanz 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.

Looks good on a glance.

@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:IO Issues affecting the IO module area:ThirdParty Issues affecting the ThirdParty module labels Jul 2, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review July 3, 2026 00:16
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes DCMTK build-tree exports for external ITK consumers. The main changes are:

  • Link DCMTK IO modules through DCMTK:: namespaced targets.
  • Require system DCMTK 3.7.0 or newer for ITK_USE_SYSTEM_DCMTK.
  • Rewrite malformed vendored DCMTK build-tree codec dependencies from DCMTK::ITK::*Module to ITK::*Module.

Confidence Score: 4/5

This PR is close to safe, with one contained build-configuration issue to fix.

The export repair and namespaced target changes match the intended external-consumer flow, while an existing BUILD_SINGLE_SHARED_LIBRARY=ON cache value can bypass the new pin and leave the changed DCMTK::dcmdata links unresolved.

Modules/ThirdParty/DCMTK/CMakeLists.txt

T-Rex T-Rex Logs

What T-Rex did

  • Ran a real ITK vendored DCMTK configure with BUILD_SINGLE_SHARED_LIBRARY ON; CMakeCache showed the value rewritten to INTERNAL=OFF.
  • Executed a minimal CMake harness that exercises the cache and target-creation logic, validating per-library target creation when BUILD_SINGLE_SHARED_LIBRARY is OFF in the cache semantics.
  • Ran a two-step harness that started with a cache seeded as BUILD_SINGLE_SHARED_LIBRARY=ON and reconfigured to observe changes to the cache entry, which changed to INTERNAL=OFF.
  • Reviewed DCMTK build-tree consumer artifacts to understand when and where the consumer failed or halted, noting that the post-run head did not complete the expected rewrite.
  • Verified that the head run did not rewrite the interface to ITK::ITKZLIBModule and observed that the consumer stops at find_package, indicating a contract mismatch before a successful build.
  • Observed that the build's IO/DCMTK module collection shifted from bare libraries to DCMTK::-prefixed targets and IOTransformDCMTK mapping, but end-to-end validation could not complete due to a not-yet-finished libITKIODCMTK-6.0.so.1 dependency.
  • Ran DCMTK system-version checks; found that fake DCMTK 3.7.0 satisfies the version constraint while 3.6.8 triggers an incompatible version error.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/IO/DCMTK/src/CMakeLists.txt Updates ITKIODCMTK to link namespaced DCMTK targets so exported build-tree consumers resolve DCMTK libraries.
Modules/IO/IOTransformDCMTK/src/CMakeLists.txt Updates IOTransformDCMTK to link the namespaced DCMTK dcmdata target.
Modules/ThirdParty/DCMTK/CMakeLists.txt Adds vendored DCMTK namespaced wrappers and build-tree export repair, with one cache override issue in the single-library mode pin.
Modules/ThirdParty/DCMTK/itk-module-init.cmake Raises system DCMTK discovery to require version 3.7.0 for namespaced target availability.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
  participant Consumer as External build-tree consumer
  participant ITK as ITKConfig / ITKDCMTK export code
  participant DCMTK as DCMTKConfig.cmake
  participant Targets as Imported targets

  Consumer->>ITK: find_package(ITK COMPONENTS ITKIODCMTK)
  ITK->>DCMTK: set DCMTK_DIR to ITK build root
  ITK->>DCMTK: find_package(DCMTK REQUIRED NO_MODULE)
  DCMTK->>Targets: import DCMTK::dcmdata and related targets
  ITK->>Targets: "rewrite DCMTK::ITK::*Module links to ITK::*Module"
  Consumer->>Targets: "link ITKIODCMTK / IOTransformDCMTK via DCMTK::* targets"
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 Consumer as External build-tree consumer
  participant ITK as ITKConfig / ITKDCMTK export code
  participant DCMTK as DCMTKConfig.cmake
  participant Targets as Imported targets

  Consumer->>ITK: find_package(ITK COMPONENTS ITKIODCMTK)
  ITK->>DCMTK: set DCMTK_DIR to ITK build root
  ITK->>DCMTK: find_package(DCMTK REQUIRED NO_MODULE)
  DCMTK->>Targets: import DCMTK::dcmdata and related targets
  ITK->>Targets: "rewrite DCMTK::ITK::*Module links to ITK::*Module"
  Consumer->>Targets: "link ITKIODCMTK / IOTransformDCMTK via DCMTK::* targets"
Loading

Comments Outside Diff (1)

  1. General comment

    P1 Build-tree ITK import skips DCMTK target rewrite after DCMTK was imported first

    • Bug
      • The PR claim requires an external build-tree consumer to import DCMTK from the ITK build root before find_package(ITK), then have ITK's package import rewrite already-imported DCMTK:: targets from DCMTK::ITK::*Module references to ITK::*Module. Executed head evidence shows this does not happen: after find_package(ITK REQUIRED COMPONENTS ITKIODCMTK), DCMTK::dcmdata still contains DCMTK::ITK::ITKZLIBModule. This leaves the same broken imported-target namespace that the change was intended to repair.
    • Cause
      • In Modules/ThirdParty/DCMTK/CMakeLists.txt, the generated build-tree export code added at lines 172-187 is wrapped in if(NOT ITK_BINARY_DIR). In the build-tree consumer scenario, importing ITK defines ITK_BINARY_DIR, so the rewrite loop is skipped and previously imported DCMTK:: targets are not repaired.
    • Fix
      • Run the rewrite loop for external consumers even when ITK_BINARY_DIR is defined by a build-tree ITK package import. For example, do not guard the repair with if(NOT ITK_BINARY_DIR), or replace it with a condition that only avoids recursive/inside-ITK execution while still executing during external build-tree package import. Then verify that DCMTK::ITK:: entries become ITK:: after find_package(ITK).

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "COMP: Repair DCMTK build-tree export ref..." | Re-trigger Greptile

Comment thread Modules/ThirdParty/DCMTK/CMakeLists.txt Outdated
@blowekamp

Copy link
Copy Markdown
Member

I was able to inspect the DCMTKTargets.cmake file and verified this but.

The first two commit look like they can quickly go in quickly.

The namespace export changes I would like more time to inspect closer, there may be changes needed in DCMTK to make this fix more normal.

Bare dcmdata/dcmimgle/... names survive only inside ITK's own build
scope; the exported link interface then carries $<LINK_ONLY:dcmdata>,
which no external consumer of the ITK build tree can resolve. The
DCMTK:: wrapper targets export as DCMTK::dcmdata, which consumers
resolve through find_package(DCMTK).

Pin BUILD_SINGLE_SHARED_LIBRARY=OFF for the vendored build: a single-
library build renames the per-library targets with a suffix that the
DCMTK::<lib> link names cannot match, and the suffix variable is not
visible outside DCMTK's directory scope. This also retires the dead
DCMTK_LIBRARY_SUFFIX handling in the wrapper loop.
@hjmjohnson hjmjohnson force-pushed the fix-dcmtk-build-export-namespace branch from bcc43b3 to 0a0a06c Compare July 3, 2026 19:39
@hjmjohnson hjmjohnson changed the title COMP: Fix DCMTK build-tree export for external ITK consumers COMP: Link namespaced DCMTK targets in the DCMTK IO modules Jul 3, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Done as suggested @blowekamp: this PR is now just the first two commits (with the inline set() fix applied, head 0a0a06c), and the namespace export repair moved to draft #6548 for your closer inspection — its body notes the possible upstream-DCMTK route for a more normal fix. Locally re-validated here: 46/46 DCMTK tests, and the poisoned-cache scenario configures correctly.

@blowekamp blowekamp self-requested a review July 4, 2026 10:25
@hjmjohnson hjmjohnson merged commit c811229 into InsightSoftwareConsortium:main Jul 4, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module 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