Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for upgraded DCMTK and web-assembly build #496

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

jadh4v
Copy link
Collaborator

@jadh4v jadh4v commented Jun 7, 2024

(This is a work in progress)
Updates to DCMQI library after upgrading DCMTK to a later commit (InsightSoftwareConsortium/DCMTK@0c72275).
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
Copy link
Member

fedorov commented Jun 7, 2024

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?

@fedorov
Copy link
Member

fedorov commented Jun 7, 2024

@fedorov
Copy link
Member

fedorov commented Jun 7, 2024

@thewtex
Copy link
Contributor

thewtex commented Jun 7, 2024

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: InsightSoftwareConsortium/ITK@4b010e7

@fedorov
Copy link
Member

fedorov commented Jun 7, 2024

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

fedorov added a commit to jadh4v/dcmqi that referenced this pull request Jun 7, 2024
Checking to see if this would address Win runtime error in QIICR#496
@thewtex
Copy link
Contributor

thewtex commented Jun 7, 2024

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
Copy link
Contributor

thewtex commented Jun 7, 2024

@rfloca
Copy link

rfloca commented Jun 10, 2024

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
Copy link
Collaborator Author

jadh4v commented Jun 11, 2024

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.

@jadh4v jadh4v self-assigned this Jun 11, 2024
@jcfr
Copy link
Contributor

jcfr commented Jun 11, 2024

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

CMakeLists.txt Outdated Show resolved Hide resolved
@jadh4v jadh4v changed the title WIP: fixes for upgraded DCMTK and web-assembly build Fixes for upgraded DCMTK and web-assembly build Jun 21, 2024
@jadh4v jadh4v marked this pull request as ready for review June 21, 2024 13:06
@fedorov fedorov merged commit 2a2d359 into QIICR:master Jun 21, 2024
6 checks passed
@jadh4v jadh4v deleted the wasm-dcmqi branch June 27, 2024 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants