COMP: Build DCMTK with FetchContent instead of ExternalProject

[hjmjohnson]

Builds DCMTK with FetchContent + in-scope add_subdirectory instead of ExternalProject_Add, so DCMTK is configured in ITK's own CMake scope and consumes ITK's codec libraries as the live imported targets (ITK::itktiff/itkpng/zlib/itkjpeg) — the GDCM model @blowekamp described in #6393, with no $<TARGET_FILE:> marshalling or :::-separated CMAKE_ARGS.

This is the full FetchContent alternative to #6393 (the surgical ExternalProject fix). Structured as one commit per integration hurdle so each can be reviewed independently.

Commits (one per hurdle)

Commit	Hurdle
COMP: Build DCMTK in-scope with FetchContent	core mechanism: FetchContent_MakeAvailable + itk_module_use() dependency bootstrap + codec-variable wiring + DCMTK_USE_ICU-honoring charset config
COMP: Export DCMTK targets through ITK's module namespace	itk_module_target() naming, ITK::<lib> aliases, build-tree targets file
COMP: Silence DCMTK third-party compiler warnings	itk_module_warnings_disable (DCMTK compiles in ITK's /W4 scope)
COMP: Install DCMTK libraries and headers with ITK	install libs into ITK's export set + DCMTK headers into ITK's include tree
COMP: Build in-scope DCMTK alongside MINC on MSVC	snapshot/restore CMAKE_REQUIRED_*; assert MSVC HAVE_TMPNAM so MINC's shared-cache probe is not poisoned

Why DCMTK differs from GDCM (and why this needs FetchContent)

GDCM is add_subdirectory()'d, so it already runs in ITK's scope and can take ITK's imported targets directly. DCMTK was an ExternalProject — a separate CMake process where imported targets can't cross the boundary, forcing the $<TARGET_FILE:> + ::: marshalling. FetchContent_MakeAvailable is essentially "fetch then add_subdirectory", which puts DCMTK in ITK's scope and removes that marshalling. EXCLUDE_FROM_ALL is intentionally not used on the in-scope DCMTK directory: it leaked into ITK's own test targets and dropped them from the ALL build. DCMTK's libraries are required by ITKIODCMTK (so they belong in ALL) and DCMTK builds no apps (BUILD_APPS OFF) or tests, so nothing extraneous is added.

Validation (Windows / MSVC)

• Configure + generate succeed, including install(EXPORT ITKTargets) (where install/export-set inconsistencies surface).
• DCMTK compiles in ITK's Ninja graph and ITKIODCMTK links; DCMTK libraries are ITK-named (itkdcmdata-6.0.lib …).
• The per-file MSVC D9025 warning bleed drops to zero after the warnings commit.
• Full default-modules build + install-tree validation is covered by CI (all required checks green).

AI assistance

• Tool: Claude Code (claude-opus-4-8)
• Role: implemented the FetchContent conversion and worked through each integration hurdle (dependency bootstrapping, module-namespace export, warning isolation, install, MINC cross-module cache poisoning) against ITK's module macros.
• Built and verified on Windows/MSVC (DCMTK + ITKIODCMTK, exit 0) before pushing.

Relates to #6393, #5539; supersedes the ExternalProject approach if the team prefers FetchContent.

[hjmjohnson]

The first Windows CI run surfaced a real bug in the in-scope conversion that my local target-by-target builds had missed (I was building ITKIODCMTK directly, never the full all):

Pixi-Cxx (windows-2022) failed with 3699/3849 tests "Not Run" — the test executables were never built. Root cause: EXCLUDE_FROM_ALL on the in-scope DCMTK add_subdirectory leaked into ITK's own *-test modules (added later in the same ITKModuleEnablement loop) and dropped every test executable from the ALL target.

Fixed in bc50dc5 by dropping EXCLUDE_FROM_ALL entirely: DCMTK's libraries are required by ITKIODCMTK so they belong in ALL anyway, and DCMTK builds no apps (BUILD_APPS OFF) or tests, so nothing extraneous is pulled in. As a bonus this lets plain FetchContent_MakeAvailable() cover every CMake version (>= 3.14), so the 3.30 FetchContent_Populate deprecation (the earlier @greptile-apps P2) is resolved without a version gate.

Verified locally: cmake --build build now links the test drivers and a deliberately-deleted ITKImageGridGTestDriver.exe is rebuilt by ALL. Re-running CI on bc50dc5.

[greptile-apps]

Good diagnosis and clean fix. A few observations on bc50dc5:

The fix is correct. EXCLUDE_FROM_ALL on an add_subdirectory sets a directory property that propagates to all targets defined in that directory and its descendants. When ITK's module enablement loop later calls into subdirectories that inherit that property scope, targets defined there also get excluded from ALL. Dropping it entirely is the right call given:

1. DCMTK's libraries are a hard dependency of ITKIODCMTK — they'd be built regardless.
2. BUILD_APPS OFF and no tests mean nothing extraneous lands in ALL.

The comment block at ~line 190 documents this reasoning well, which is good since "why not EXCLUDE_FROM_ALL" will come up in review.

One thing to double-check with the CI run: ITKDCMTK_NO_SRC 1 (line 7) tells ITK's module machinery there's no source to compile in the module directory itself. That's still accurate post-FetchContent (DCMTK sources live under dcmtk_SOURCE_DIR, not Modules/ThirdParty/DCMTK/src/), so that should be fine — but worth confirming ITKDCMTKModule still links correctly in CI, since ITKDCMTK_NO_SRC affects how itk_module_impl() handles that target.

Minor — ICU path is now inconsistent: The ExternalProject_Add(icu ...) block (lines ~68–95) is gated on DCMTK_USE_ICU which defaults off, but if that path is exercised the ICU_ROOT_DIR wiring into the in-scope DCMTK (set(ICU_ROOT "${ICU_ROOT_DIR}" ...)) relies on the ExternalProject completing before DCMTK's configure step runs — which isn't guaranteed in an in-scope build. Since this path isn't tested by CI, a TODO comment there would be appropriate to flag it for a follow-up.

[hjmjohnson]

Root cause of the Windows CI failure — a fundamental blocker for the in-scope approach

I chased the "3699 tests Not Run" down to its actual cause, and it is not about tests or EXCLUDE_FROM_ALL (that was a red herring — the test failures were just fallout from the build aborting). The Windows build stops here, at compile step ~54 of 5868:

Modules/ThirdParty/MINC/.../libminc/.../volume_io/Prog_utils/files.c.obj
fatal error C1189: "System defines neither mkstemp(), tempnam(), nor tmpnam()"
ninja: build stopped: subcommand failed.

Why

DCMTK and ITK's MINC both probe the C library with identical cache-variable names:

Module	Code
MINC	CHECK_FUNCTION_EXISTS(mkstemp HAVE_MKSTEMP), ... (tmpnam HAVE_TMPNAM), ... (tempnam HAVE_TEMPNAM)
DCMTK	CHECK_FUNCTION_EXISTS(... HAVE_<FUNC>) for a large list of libc functions, run in GenerateDCMTKConfigure.cmake

With FetchContent + add_subdirectory, DCMTK is configured in ITK's shared CMake scope and runs first. CHECK_FUNCTION_EXISTS caches its result in the named variable, and the call is a no-op if that cache entry already exists — so MINC inherits DCMTK's results (and/or DCMTK's leftover CMAKE_REQUIRED_* state), ends up with none of the three defined, and fires its #error. The same class of collision produced the GDCM strncasecmp (GDCM_HAVE__STRNICMP) failure seen earlier.

Why this is the crux

This is precisely what the ExternalProject design prevents: DCMTK is configured in a separate CMake process, so its hundreds of HAVE_* probes and CMAKE_REQUIRED_* mutations can never pollute ITK's cache or sibling third-party modules (MINC, GDCM, …). FetchContent's whole premise — one shared scope — is what reintroduces the pollution.

A fix would mean save/restoring all CMAKE_REQUIRED_* around the DCMTK subdirectory and preventing every HAVE_* cache-name collision between DCMTK and ~15 vendored ITK third-party modules — fragile whack-a-mole that would need revisiting every time DCMTK or a sibling adds a probe.

Recommendation

This confirms the in-scope/FetchContent direction is not viable for DCMTK without process isolation, which is the very thing ExternalProject already provides. I'd suggest keeping this PR as a documented exploration and landing the surgical ExternalProject fix in #6393 instead. Happy to pursue mitigation if the team wants to, but it would be a substantial and brittle undertaking.

[blowekamp]

Using fetch content over external project remove the need to export DCMTK targets with ITK specific code. And similarly describe ITK target to DCMTK with variables. This is extra code burdens ITK developers.

Can we determine what are the potential conflicted CMake Cache variables between ITK and DCMTK. Perhaps by examining a cache of ITK (without DCMTK) and one of a standalone DCMTK? Then we might need to modify a DCMTK or some other ITK subproject to address the conflict.

For the HAVE_MKSTEMP above, perhaps MINC could be modified to use the MINC prefix for more of there CMake variables.

[hjmjohnson]

@blowekamp Took your suggestion — enumerated the ITK↔DCMTK cache-variable collisions and fixed them in-tree (bf19e389). The in-scope build now configures with a deterministic MINC config.h.

Collision analysis (your "diff the caches" approach)

Rather than two separate caches, I diffed MINC's generated config.h with vs. without the in-scope DCMTK in the same build. DCMTK's CHECK_FUNCTION_EXISTS probes leak into MINC two ways:

1. Direct name collisions — DCMTK caches HAVE_MKSTEMP, HAVE_TMPNAM, HAVE_TEMPNAM (and the rest of MINC's libc-probe names). check_function_exists is a no-op when its result var is already cached, so MINC inherited DCMTK's values → all three of mkstemp/tempnam/tmpnam dropped → the Windows #error "System defines neither...".
2. Indirect / vestigial macros — config.h.cmake references HAVE_FORK/HAVE_VFORK, but MINC only probes the HAVE_WORKING_* variants (the C sources key on those). With ExternalProject those macros stayed #undef; in-scope, DCMTK's HAVE_FORK/HAVE_VFORK leaked straight through. (Other gap macros — HAVE_NDIR_H, HAVE_INT16_T, HAVE_INT32_T — are not cached by DCMTK, so they did not leak.)

Fix

• MINC (libminc/CMakeLists.txt): probe into MINC_-prefixed cache variables via small minc_check_function_exists/minc_check_symbol_exists wrappers, then set the unprefixed name locally for config.h — so config.h.cmake and the vendored C are untouched. Pinned the vestigial HAVE_FORK/HAVE_VFORK to MINC's own HAVE_WORKING_* results.
• DCMTK (Modules/ThirdParty/DCMTK/CMakeLists.txt): snapshot/restore CMAKE_REQUIRED_* around FetchContent_MakeAvailable(dcmtk) so DCMTK's probe state can't perturb sibling feature tests.

Validation

Local (Linux, in-scope DCMTK + MINC, internal everything):

• MINC config.h is now byte-identical with vs. without DCMTK — the leak is gone and the result is deterministic.
• The refactor is behavior-preserving: config.h matches the pre-change build except the two intended HAVE_FORK/HAVE_VFORK deltas (now driven by MINC's own fork/vfork probe instead of left unset).
• itkminc2 builds in both configurations; pre-commit run --all-files passes.

The actual Windows #error can't be reproduced on Linux (mkstemp exists here), so Windows CI on bf19e389 is the first executor of the cross-platform resolution. The mechanism — MINC's prefixed probes run independently of DCMTK's cache — is what the Linux determinism test confirms.

[hjmjohnson]

Reworked the MINC fix (f08a836) — the earlier cache-isolation approach was wrong (it made MINC run its own CHECK_FUNCTION_EXISTS(tmpnam), which also fails on MSVC, so it would have broken MINC on Windows even without DCMTK). Replaced with the upstream-blessed override.

Confirmed root cause

main/Windows builds MINC fine. The in-scope DCMTK runs its CHECK_*_EXISTS probes in ITK's shared scope and caches HAVE_TMPNAM=0 on MSVC — its tmpnam probe uses CHECK_FUNCTION_EXISTS, which can't detect MSVC CRT functions (self-declared prototype fails to link). MINC's CHECK_FUNCTION_EXISTS(tmpnam HAVE_TMPNAM) is then a cache-hit no-op, so MINC inherits 0; with none of mkstemp/tempnam/tmpnam defined, files.c / netcdf_convenience.c hit the #error. On main MINC benignly inherits a correct HAVE_TMPNAM=1 cached by another module first.

Fix

• Modules/ThirdParty/MINC/.../libminc/CMakeLists.txt: tmpnam() exists in the MSVC CRT, so if(MSVC) set(HAVE_TMPNAM 1) — mirrors MINC's own config.h.msvc-win32. The local set() also overrides any value left in the shared cache, so it's robust to the DCMTK pollution regardless of probe order.
• Modules/ThirdParty/DCMTK/CMakeLists.txt: snapshot/restore CMAKE_REQUIRED_* around FetchContent_MakeAvailable(dcmtk) (hygiene — keeps DCMTK's probe state from perturbing sibling checks).

Inert on non-MSVC (Linux/macOS config.h unchanged); Windows CI is the first executor of the MSVC path.

[hjmjohnson]

@blowekamp. This ia an attempt to use FetchContent for DCMTK as you previously indicated would be desirable. I don't have a good use case for this, so I would like to leave it in your hands to review and determine next steps.

1). I am happy to abandon this work if it is not what you were hoping for.
2). I am happy for you to take this over and do with it what you want.

Hans

[hjmjohnson]

Assuming this work is not a desirable path, so closing this PR.

[blowekamp]

I would like to come back to this to revisit. It looked like very good progress was made, but I have not had time to look closely.