ENH: Build DCMTK in-scope with FetchContent - #6474
Conversation
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. |
|
@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. |
7727bef to
a0e474e
Compare
Replace the ExternalProject DCMTK build with FetchContent so DCMTK configures inside ITK's CMake scope, consumes ITK's codec targets directly, and exports namespaced DCMTK:: targets through ITK's module system. Bump DCMTK_GIT_TAG to the fork commit that prefixes DCMTK's CMake check commands with DCMTK_, preventing the CHECK_FUNCTION_EXISTS override from leaking into sibling modules (MINC tmpnam C1189 on MSVC). Supersedes InsightSoftwareConsortium#6397, InsightSoftwareConsortium#6474 commit series.
Replace the ExternalProject DCMTK build with FetchContent so DCMTK configures inside ITK's CMake scope, consumes ITK's codec targets directly, and exports namespaced DCMTK:: targets through ITK's module system. Bump DCMTK_GIT_TAG to the fork commit that prefixes DCMTK's CMake check commands with DCMTK_, preventing the CHECK_FUNCTION_EXISTS override from leaking into sibling modules (MINC tmpnam C1189 on MSVC). Supersedes InsightSoftwareConsortium#6397, InsightSoftwareConsortium#6474 commit series.
a0e474e to
9ff0b09
Compare
|
@blowekamp I did the squashing, and some small cleanups, and mereged the approved DCMTK PR that was needed to support this. Handing it back to you for next steps. |
Replace the ExternalProject DCMTK build with FetchContent so DCMTK configures inside ITK's CMake scope, consumes ITK's codec targets directly, and exports namespaced DCMTK:: targets through ITK's module system. Bump DCMTK_GIT_TAG to the fork commit that prefixes DCMTK's CMake check commands with DCMTK_, preventing the CHECK_FUNCTION_EXISTS override from leaking into sibling modules (MINC tmpnam C1189 on MSVC). Supersedes InsightSoftwareConsortium#6397, InsightSoftwareConsortium#6474 commit series.
9ff0b09 to
3de8308
Compare
Thanks Hans. The changes look good to me now. |
|
| Filename | Overview |
|---|---|
| Modules/ThirdParty/DCMTK/CMakeLists.txt | Core change replacing ExternalProject_Add with FetchContent_MakeAvailable; two defects found: unconditional ITKDCMTK_LIBRARIES append outside the TARGET guard, and broken non-system ICU path due to configure-time vs build-time ordering. |
| Modules/ThirdParty/DCMTK/DCMTKGitTag.cmake | Updates DCMTK git tag to pick up the DCMTK_ prefix patch for CMake check commands. |
| Modules/ThirdParty/DCMTK/itk-module-init.cmake | Removes lib_prefix/lib_suffix detection block that was only needed for ExternalProject output paths; ICU detection and option declarations retained. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ITK CMake Configure] --> B{ITK_USE_SYSTEM_DCMTK?}
B -- YES --> C[find_package DCMTK System install]
B -- NO --> D{DCMTK_USE_ICU?}
D -- NO --> E[Set DCMTK variables in ITK scope]
D -- YES --> F{ITK_USE_SYSTEM_ICU?}
F -- YES --> G[find_package ICU System install]
F -- NO --> H[ExternalProject_Add ICU - Builds at BUILD time]
H --> I[Set ICU_ROOT to not-yet-built path]
G --> E
I --> E
E --> J[FetchContent_MakeAvailable dcmtk - Runs DCMTK cmake NOW]
J --> K[DCMTK find_package ICU at configure time - ICU not built yet]
J --> L[DCMTK targets created]
L --> M[Create DCMTK:: INTERFACE IMPORTED wrappers]
M --> N{TARGET exists?}
N -- YES --> O[add_library DCMTK::target]
N -- NO --> P[Skip wrapper]
O --> Q[list APPEND ITKDCMTK_LIBRARIES - always runs]
P --> Q
Q --> R[itk_module_impl]
C --> R
%%{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 CMake Configure] --> B{ITK_USE_SYSTEM_DCMTK?}
B -- YES --> C[find_package DCMTK System install]
B -- NO --> D{DCMTK_USE_ICU?}
D -- NO --> E[Set DCMTK variables in ITK scope]
D -- YES --> F{ITK_USE_SYSTEM_ICU?}
F -- YES --> G[find_package ICU System install]
F -- NO --> H[ExternalProject_Add ICU - Builds at BUILD time]
H --> I[Set ICU_ROOT to not-yet-built path]
G --> E
I --> E
E --> J[FetchContent_MakeAvailable dcmtk - Runs DCMTK cmake NOW]
J --> K[DCMTK find_package ICU at configure time - ICU not built yet]
J --> L[DCMTK targets created]
L --> M[Create DCMTK:: INTERFACE IMPORTED wrappers]
M --> N{TARGET exists?}
N -- YES --> O[add_library DCMTK::target]
N -- NO --> P[Skip wrapper]
O --> Q[list APPEND ITKDCMTK_LIBRARIES - always runs]
P --> Q
Q --> R[itk_module_impl]
C --> R
Comments Outside Diff (1)
-
Modules/ThirdParty/DCMTK/CMakeLists.txt, line 5-6 (link)The comment still describes the old
ExternalProjectmodel; DCMTK is now built in ITK's own scope via FetchContent.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "STYLE: Remove redundant /Zc:__cplusplus ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| foreach(_dcmtk_lib IN LISTS _ITKDCMTK_LIB_NAMES) | ||
| set(_dcmtk_target "${_dcmtk_lib}${DCMTK_LIBRARY_SUFFIX}") | ||
| if(TARGET "${_dcmtk_target}" AND NOT TARGET "DCMTK::${_dcmtk_target}") | ||
| add_library("DCMTK::${_dcmtk_target}" INTERFACE IMPORTED GLOBAL) | ||
| target_link_libraries( | ||
| "DCMTK::${_dcmtk_target}" | ||
| INTERFACE | ||
| "${_dcmtk_target}" | ||
| ) | ||
| endif() | ||
|
|
||
| # make the imported library depend on its prerequisite | ||
| # libraries | ||
| set_property( | ||
| TARGET | ||
| ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}${lib} | ||
| PROPERTY | ||
| IMPORTED_LINK_INTERFACE_LIBRARIES | ||
| ${ITKDCMTK_LIBDEP} | ||
| ) | ||
| list(APPEND ITKDCMTK_LIBRARIES "DCMTK::${_dcmtk_target}") | ||
| endforeach() |
There was a problem hiding this comment.
ITKDCMTK_LIBRARIES
list(APPEND ITKDCMTK_LIBRARIES "DCMTK::${_dcmtk_target}") runs unconditionally, outside the if(TARGET "${_dcmtk_target}" AND NOT TARGET "DCMTK::${_dcmtk_target}") guard. If FetchContent_MakeAvailable(dcmtk) did not produce a target for a given library name (e.g., due to a DCMTK version difference or a naming mismatch), the non-existent DCMTK::${_dcmtk_target} is still appended. When itk_module_impl() later processes ITKDCMTK_LIBRARIES, CMake will error on a target that was never defined. The append should move inside — or at least also guard on — the if(TARGET ...) block.
There was a problem hiding this comment.
Fixed in 39f4f94 — the list(APPEND ITKDCMTK_LIBRARIES ...) now lives inside an if(TARGET "${_dcmtk_target}") guard, so a missing or renamed DCMTK library no longer appends a non-existent DCMTK:: target.
| if(DCMTK_USE_ICU) | ||
| set(DCMTK_WITH_ICU ON) | ||
| set(DCMTK_WITH_STDLIBC_ICONV OFF) | ||
| set(DCMTK_ENABLE_CHARSET_CONVERSION "ICU" CACHE INTERNAL "") # NOTE: DCMTK requires a cache entry | ||
| set(ICU_ROOT "${ICU_ROOT_DIR}") | ||
| else() | ||
| set( | ||
| ITKDCMTK_EXPORT_CODE_BUILD | ||
| " | ||
| ${ITKDCMTK_EXPORT_CODE_BUILD} | ||
| foreach(lib ${_ITKDCMTK_LIB_NAMES}) | ||
| if(NOT TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib}) | ||
| add_library(${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} ${_ITKDCMTK_LIB_LINKAGE} IMPORTED) | ||
| set_property(TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} PROPERTY IMPORTED_LOCATION | ||
| \"${lib_dir}/${lib_prefix}\${lib}${lib_suffix}\") | ||
| set_property(TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} PROPERTY | ||
| IMPORTED_LINK_INTERFACE_LIBRARIES | ||
| ${ITKDCMTK_LINK_DEPENDENCIES}) | ||
| endif() | ||
| endforeach() | ||
| " | ||
| ) | ||
| endif() | ||
| # create imported targets when module is loaded from install tree | ||
| set( | ||
| ITKDCMTK_EXPORT_CODE_INSTALL | ||
| " | ||
| foreach(lib ${_ITKDCMTK_LIB_NAMES}) | ||
| if(NOT TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib}) | ||
| add_library(${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} ${_ITKDCMTK_LIB_LINKAGE} IMPORTED) | ||
| set_property(TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} PROPERTY IMPORTED_LOCATION | ||
| \"\${ITK_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${lib_prefix}\${lib}${lib_suffix}\") | ||
| set_property(TARGET ${ITK_MODULE_${itk-module}_TARGETS_NAMESPACE}\${lib} PROPERTY | ||
| IMPORTED_LINK_INTERFACE_LIBRARIES | ||
| ${ITKDCMTK_LINK_DEPENDENCIES}) | ||
| set(DCMTK_WITH_ICU OFF) | ||
| set(DCMTK_ENABLE_BUILTIN_OFICONV_DATA ON) | ||
| set(DCMTK_ENABLE_CHARSET_CONVERSION "oficonv" CACHE INTERNAL "") | ||
| endif() |
There was a problem hiding this comment.
DCMTK_USE_ICU=ON and ITK_USE_SYSTEM_ICU=OFF
FetchContent_MakeAvailable(dcmtk) populates and configures DCMTK at CMake configure time. When DCMTK_USE_ICU=ON and ITK_USE_SYSTEM_ICU=OFF, ICU is still built by ExternalProject_Add(icu …) (line 93), which runs at build time — after the configure phase. DCMTK's cmake, running in-scope during configuration, will call find_package(ICU) with ICU_ROOT pointing at an install directory that doesn't exist yet. The old ExternalProject_Add for DCMTK had an explicit DEPENDS ${ICU_DEPENDENCY} to enforce ordering; FetchContent has no equivalent mechanism.
There was a problem hiding this comment.
Fixed in 39f4f94 — the in-scope FetchContent model cannot sequence a build-time ICU ExternalProject before DCMTK's configure-time find_package(ICU). The from-source ICU path is removed; DCMTK_USE_ICU=ON now requires ITK_USE_SYSTEM_ICU=ON and fails with a clear FATAL_ERROR otherwise.
| set( | ||
| CHARSET_CONVERSION_ARGS | ||
| -DDCMTK_WITH_STDLIBC_ICONV:BOOL=OFF | ||
| -DDCMTK_WITH_ICU:BOOL=ON | ||
| -DDCMTK_ENABLE_CHARSET_CONVERSION:STRING=ICU | ||
| -DICU_ROOT:PATH=${ICU_ROOT_DIR} | ||
| ) | ||
| itk_download_attempt_check(icu) |
There was a problem hiding this comment.
CHARSET_CONVERSION_ARGS assignment — the variable is built up here but is never passed anywhere in the new FetchContent path (the ExternalProject_Add for DCMTK that consumed it is gone). The equivalent settings are now applied as normal CMake variables directly before FetchContent_MakeAvailable. This branch can be simplified by removing the leftover list-build.
| set( | |
| CHARSET_CONVERSION_ARGS | |
| -DDCMTK_WITH_STDLIBC_ICONV:BOOL=OFF | |
| -DDCMTK_WITH_ICU:BOOL=ON | |
| -DDCMTK_ENABLE_CHARSET_CONVERSION:STRING=ICU | |
| -DICU_ROOT:PATH=${ICU_ROOT_DIR} | |
| ) | |
| itk_download_attempt_check(icu) | |
| itk_download_attempt_check(icu) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 39f4f94 — the dead CHARSET_CONVERSION_ARGS assignments were removed (no ExternalProject consumes them anymore; charset selection is applied via DCMTK_WITH_ICU / DCMTK_ENABLE_CHARSET_CONVERSION directly before FetchContent_MakeAvailable).
| else() | ||
| # Built-in oficonv only; disable ICU and external iconv for a deterministic backend. | ||
| list( | ||
| APPEND | ||
| CHARSET_CONVERSION_ARGS | ||
| -DDCMTK_WITH_ICU:BOOL=OFF | ||
| -DDCMTK_WITH_ICONV:BOOL=OFF | ||
| ) | ||
| set(CHARSET_CONVERSION_ARGS -DDCMTK_WITH_ICU:BOOL=OFF) | ||
| endif() |
There was a problem hiding this comment.
CHARSET_CONVERSION_ARGS assignment in the non-ICU else branch — there is no longer a DCMTK ExternalProject_Add to consume this variable. The DCMTK_WITH_ICU OFF setting is now applied as a normal variable directly in the FetchContent configuration block.
| else() | |
| # Built-in oficonv only; disable ICU and external iconv for a deterministic backend. | |
| list( | |
| APPEND | |
| CHARSET_CONVERSION_ARGS | |
| -DDCMTK_WITH_ICU:BOOL=OFF | |
| -DDCMTK_WITH_ICONV:BOOL=OFF | |
| ) | |
| set(CHARSET_CONVERSION_ARGS -DDCMTK_WITH_ICU:BOOL=OFF) | |
| endif() | |
| else() | |
| endif() |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 39f4f94 — dead CHARSET_CONVERSION_ARGS removed here too; the DCMTK_WITH_ICU OFF setting is now applied as a normal variable in the FetchContent block.
There was a problem hiding this comment.
Confirmed and fixed. With the build moved in-scope via FetchContent + add_subdirectory(), there's no separate CMake invocation to receive a -D argument list, so the CHARSET_CONVERSION_ARGS marshalling is dead. The charset configuration is now set directly as ordinary variables in the DCMTK_USE_ICU if/else block (DCMTK_WITH_ICU, DCMTK_ENABLE_BUILTIN_OFICONV_DATA, and the cache entry DCMTK_ENABLE_CHARSET_CONVERSION), matching how every other DCMTK option in this file is passed. The dead CHARSET_CONVERSION_ARGS assignment has been removed.
Replace the ExternalProject DCMTK build with FetchContent so DCMTK configures inside ITK's CMake scope, consumes ITK's codec targets directly, and exports namespaced DCMTK:: targets through ITK's module system. Bump DCMTK_GIT_TAG to the fork commit that prefixes DCMTK's CMake check commands with DCMTK_, preventing the CHECK_FUNCTION_EXISTS override from leaking into sibling modules (MINC tmpnam C1189 on MSVC). Supersedes InsightSoftwareConsortium#6397, InsightSoftwareConsortium#6474 commit series.
Move lib_prefix into CMakeLists.txt next to its only use and reduce it to a single line: ICU is always built statically on non-Windows so CMAKE_STATIC_LIBRARY_PREFIX is the only relevant value. Remove lib_suffix entirely; it was set but never read.
DCMTK's GenerateDCMTKConfigure.cmake already appends /Zc:__cplusplus to CMAKE_CXX_FLAGS after its STL feature probes. The INTERFACE target_compile_options on ITKDCMTKModule at the bottom of this file handles propagation to consumers.
3de8308 to
347daf6
Compare
|
Pushed fixes for the four Greptile findings (force-pushed with lease, your authorship preserved on all three commits, no rebase onto upstream/main):
The first three commits squashed clean; net |
|
@blowekamp Sorry, but Greptile identified 3 more issues. They and the solutions sound correct to me, but they are non-trivial changes. I left the solutions unresolved to make it easier to review. |
@hjmjohnson I am not following what you and your AI have done here nor what you are asking me to do. I'll leave fixing this up to you and your agents and not spend more time here understanding these review comments right now. |
|
So.. The optional ICU dependency also needs to be converted to FetchContent so it's available during configuration of DCMTK. |
I will try to figure that out tomorrow afternoon. |
Thank you Hans. I thought I was working on this branch yesterday and it was changed on me which was discouraging. Just not that with the other changes to third-party libraries now using CMake interface libraries e.g. JPEG::JPEG the ExternalProject DCMTK build with system libraries has broken. The interface libraries can not be communicated through a "-DJPEG_LIBRARIES=JPEG::JPEG". However this tighter coupling with FetchContent does work with the interface libraries. If this PR is good except for the ICU ExternalProject option ( disable by default) it maybe work merging, and making another PR to update the ICU project. |
@blowekamp Sorry, I misinterpreted that statement as hand-off back to me. I should have double checked. I'm approving this and will let you merge. I do think it is best to make an evolutionary step forward to help decouple the issues. |
Perhaps if there a multiple developers handing off who is working on a PR, the "assignee" field can be used. |
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.