QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
228 stars 62 forks source link

Fixes for upgraded DCMTK and web-assembly build #496

Closed jadh4v closed 1 month ago

jadh4v commented 1 month ago

(This is a work in progress) Updates to DCMQI library after upgrading DCMTK to a later commit (https://github.com/InsightSoftwareConsortium/DCMTK/commit/0c722752150f7b9cad822def627e47f8ad9a6ab3). Upgrade CMake minimum version to better support sub-project mode when overriding default CMAKE variable values. These updates are related to the effort to add DICOM SEG Object read/write capabilities in ITK-wasm.

Current DCMTK fork used by ITK-wasm (includes patches for web assembly build): https://github.com/InsightSoftwareConsortium/DCMTK/tree/20240311_DCMTK_PATCHES_FOR_ITK-wasm

fedorov commented 1 month ago

This work might be related, @michaelonken ran into some issues that have not been resolved: https://github.com/QIICR/dcmqi/pull/493.

@rfloca would you like to check this out to see if this creates any issues for MITK?

fedorov commented 1 month ago

For the reference, here's configuration of DCMTK for Slicer: https://github.com/Slicer/Slicer/blob/main/SuperBuild/External_DCMTK.cmake

DCMTK apps selected https://github.com/Slicer/Slicer/blob/main/CMake/SlicerBlockInstallDCMTKApps.cmake#L5

fedorov commented 1 month ago

MITK DCMTK config: https://github.com/MITK/MITK/blob/master/CMakeExternals/DCMTK.cmake (right now uses DCMTK-3.6.7 from upstream https://github.com/MITK/MITK/blob/master/CMakeExternals/DCMTK.cmake#L33-L34).

thewtex commented 1 month ago

We are seeing Windows segfaults on other packages on GitHub Actions starting this week-ish.

One approach that may help is the CMake minimum version bump re::

CMake Warning at D:/a/ITKParabolicMorphology/ITK/CMake/ITKModuleExternal.cmake:15 (message):
-- This is needed to allow proper setting of CMAKE_MSVC_RUNTIME_LIBRARY.
-- Do not be surprised if you run into link errors of the style:
  LNK2038: mismatch detected for 'RuntimeLibrary': value 'MTd_Static' doesn't match value 'MDd_Dynamic' in module.obj
  cmake_minimum_required must be at least 3.16.3
Call Stack (most recent call first):
  CMakeLists.txt:7 (include)

Edit: related commit: https://github.com/InsightSoftwareConsortium/ITK/commit/4b010e724d7d9c14ab21b5db64cbb771c6b6d285

fedorov commented 1 month ago

@jadh4v @thewtex @michaelonken let's try to use this discord channel for coordination: https://discord.gg/3vZakgNS

thewtex commented 1 month ago

FYI re:

The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.

The ITK-Wasm DCMQI / DCMTK modules address this issue without any changes needed to DCMTK. Access in the Wasm runtime is contained only to the directories of the files passed.

thewtex commented 1 month ago

Of interest:

https://github.com/actions/runner-images/commit/a615999e85d5b3f3853c691cd59fcfe89f0495ae#diff-d3119a278ce9ce80fc4b5af63a1112fb3a044aec3e6002d838e8eb26d7a3a499R475-R480

rfloca commented 1 month ago

This work might be related, @michaelonken ran into some issues that have not been resolved: #493.

@rfloca would you like to check this out to see if this creates any issues for MITK?

I'll do. But it might take 2-3 weeks. Currently is a lot todo and I have to see when I find the time. Please excuse that I cannot react faster.

jadh4v commented 1 month ago

I have locally tested the super-build and tests on windows 11 VS2022. All tests are passing. Ran into a build issue regarding DCMTK targets. I have a temp fix for it locally, but will consult with others for more appropriate fix.

jcfr commented 1 month ago

After discussing with @jadh4v , I suggest we ensure dcmqi can support being built against a newer DCMTK without updating the version currently used in its external project.

In the short term, this will allow ITK wasm to build dcmqi against a newer version of DCMTK.

In the medium term (likely during the upcoming Slicer project week), we will work on:

The includes of headers available only in newer DCMTK could be made conditionally after including dcmtk/config/osconfig.h and using PACKAGE_VERSION_NUMBER.

cc: @michaelonken