ENH: Build DCMTK in-scope with FetchContent#6474
Conversation
Replace the DCMTK ExternalProject with FetchContent + add_subdirectory so DCMTK is configured in ITK's own CMake scope (the GDCM model). DCMTK now consumes ITK's codec libraries as the live imported targets (ITK::itktiff/itkpng/zlib/itkjpeg) through the standard *_LIBRARIES variables -- no $<TARGET_FILE:> extraction or :::-separated CMAKE_ARGS. itk_module_use() is called up front so the codec variables are populated before DCMTK is configured. FetchContent_MakeAvailable(dcmtk) does the add_subdirectory; EXCLUDE_FROM_ALL is not used because on the in-scope DCMTK directory it leaks into ITK's own test targets and drops them from the ALL build. Charset conversion honors the DCMTK_USE_ICU opt-in, wiring ICU_ROOT through when set and otherwise using DCMTK's built-in oficonv data. Build-tree namespace aliases are a placeholder pending full module-target export.
Replace the placeholder build-tree aliases with itk_module_target(), which applies ITK's output naming (itk<lib>-X.Y), creates the namespaced ITK::<lib> aliases that itk_module_impl() links ITKDCMTKModule against, and appends DCMTK's targets to ITK's build-tree targets file. The targets file variable is set during module enablement, so this works before itk_module_impl(). Installation is deferred, hence NO_INSTALL.
Disable warnings for the DCMTK subdirectory with itk_module_warnings_disable before it is configured. ITK builds at /W4 while DCMTK builds at /W3, so every DCMTK source previously emitted an MSVC D9025 flag-override warning on top of DCMTK's own diagnostics. itk_module_impl() disables warnings for third-party modules, but DCMTK is added before that call, so apply it here.
Drop NO_INSTALL so itk_module_target() installs the DCMTK libraries into ITK's library directory and export set, and install each DCMTK module's public headers (<module>/include/dcmtk) plus the generated config headers into ITK's include tree so consumers of an installed ITK can include <dcmtk/...>. install(EXPORT ITKTargets) generates cleanly: the DCMTK targets' interface dependencies are ITK's exported codec targets, other DCMTK targets, or system libraries.
Building DCMTK in ITK's shared CMake scope (FetchContent + add_subdirectory) runs DCMTK's configure in that scope, with two side effects on sibling third-party modules: - Leftover CMAKE_REQUIRED_* state from DCMTK's probes. Snapshot and restore it around FetchContent_MakeAvailable(dcmtk). - DCMTK's GenerateDCMTKConfigure.cmake globally redefines standard CMake check commands (CHECK_FUNCTION_EXISTS on Windows, CHECK_CXX_SYMBOL_EXISTS). CMake commands are global, so those overrides leak into ITK modules configured afterward: their CHECK_FUNCTION_EXISTS calls run DCMTK's header-less check_symbol_exists, report the function missing, and the module trips a #error -- MINC on tmpnam, GDCM on _strnicmp, depending on configure order. Restore the real implementations after DCMTK by including a copy of each module under a fresh path (include_guard(GLOBAL) blocks a plain re-include), fixing every dependent module regardless of order. The ExternalProject build never hit either because DCMTK configures in a separate CMake process.
CACHE INTERNAL vars in a FetchContent subdirectory scope pollute the parent CMakeCache.txt. Normal variables suffice since FetchContent runs add_subdirectory in the same scope, so DCMTK reads them directly. Codec libraries updated to ITK module interface targets so transitive include paths and link options propagate automatically.
Replace itk_module_target() wrapping with IMPORTED INTERFACE targets named DCMTK::<lib>. Unlike ALIAS targets, IMPORTED targets preserve their namespaced name in ITKTargets.cmake so downstream consumers can satisfy the dependency via find_package(DCMTK). Add ITKDCMTK_EXPORT_CODE_BUILD/INSTALL pointing consumers at the DCMTKConfig.cmake written by DCMTK's own GenerateCMakeExports.cmake. Drop CMAKE_REQUIRED_* snapshot/restore and CheckFunctionExists restore workarounds; these are no longer needed after DCMTK's check commands were renamed to DCMTK_CHECK_FUNCTION_EXISTS / DCMTK_CHECK_CXX_SYMBOL_EXISTS.
github-actions
Bot
added
type:Infrastructure
|
Hi Brad — looked over this. Two things: Windows is a real failure, not a flake. Commit structure: all 8 commits touch only |
|
Please see InsightSoftwareConsortium/DCMTK#2, which I was using locally to address that issue. |
| IMPORTED_LINK_INTERFACE_LIBRARIES | ||
| ${ITKDCMTK_LIBDEP} | ||
| ) | ||
| list(APPEND ITKDCMTK_LIBRARIES "DCMTK::${_dcmtk_target}") |
There was a problem hiding this comment.
Linking to all DCMTK libraries results is overlooking for many used. For example:
target_link_libraries(
ITKIODCMTK
PUBLIC
DCMTK::dcmdata
DCMTK::dcmimgle
DCMTK::dcmjpeg
DCMTK::dcmjpls
)
Is all that is needed not all of DCMTK. Perhaps it would be best not to list all the libraries here, so that when DCMTK is used all libraries are not linked?
There was a problem hiding this comment.
A more complete investigation of the needed libraries and the optimal set will be conducted. That should also allow less of DCMTK to be built by default.
|
@hjmjohnson I will not be working on this during the week end. You are welcome to rebase, squash and resolve difference with main, etc. Please let me review and merge when complete. The comments are still the lengthly AI style and need to be refined and reviewed. |
2 participants
Replace the ExternalProject-based DCMTK build with FetchContent so DCMTK compiles in ITKs CMake scope, consuming ITKs codec targets directly and exporting namespaced
DCMTK::targets through ITKs module system.DCMTK is exported in its own package and cmake target file. This reduced ITK burden and keeps the export code in DCMTK.
Supersedes #6397.
Changes
add_subdirectoryinside ITKs scopeITK::ITKTIFFModule/ITK::ITKJPEGModule/ITK::ITKPNGModule/ITK::ITKZLIBModuleinterface targetsDCMTK::IMPORTED INTERFACE wrappers created soITKTargets.cmakerecords namespaced target names thatfind_package(DCMTK)can satisfyITKDCMTK_EXPORT_CODE_BUILD/_INSTALLset to locateDCMTKConfig.cmakein build and install treesCHECK_FUNCTION_EXISTS,CHECK_CXX_SYMBOL_EXISTS) renamed toDCMTK_CHECK_*to prevent leaking into sibling ITK modulesCACHE INTERNALvariable noise removed; normal variables suffice in FetchContent scopeAI assistance
GitHub Copilot (Claude Sonnet 4.6) assisted with CMake design, export target strategy, and identifying the global command override leak. All changes reviewed and committed by blowekamp.