WIP: Use DcmItem in API where possible.

[michaelonken]

This is work in progress, do NOT merge.

The API change in dcmqi requires an updated DCMTK.

However, if I use a recent DCMTK version in the build (by pointing the superbuild to it), I run into linker errors since the build tries to link DCMTK libraries twice (one time each lib with full path, which works fine, and another time by just the library name, which fails), see attached log (linker.log)

@jcfr I think you authored the dcmqi suberbuild: Any idea what goes wrong? I fiddled around with it quite some time but could not find out where the "short" libs are added, or how to remove them. Maybe this is a quick one for you...

[fedorov]

@michaelonken we have been using a patched version of DCMTK: https://github.com/commontk/DCMTK/commits/patched-DCMTK-3.6.6_20210115. I do not know what is the issue, but maybe one of those patches is needed?

[michaelonken]

Thank you Andrey. As far as I can see the patches are just two backported features.

My guess is that it has to do with changes in DCMTKTargets.cmake (in DCMTK). And/Or that somehow the expectations in the ITK build and the dcmqi build are different (the latter one is failing when linking its apps), and the superbuild settings are not forwarded into both projects the same way.

We should update DCMTK in dcmqi to a more recent version, so this is probably not only relevant for this pull request.

[michaelonken]

I got this.

Probably fixed with dfae36d. DCMTK changed its exported targets a bit, mostly moving them into namespace DCMTK. Now I ask CMake to link dcmqi and ITK to DCMTK::DCMTK which will automatically refer to all exported DCMTK libraries.

[michaelonken]

Ok, here is how one could fix it:

Earlier I already changed dcmqi/libsrc/CMakeLists.txt to replace
set(_dcmtk_libs ${DCMTK_LIBRARIES}) with
set(_dcmtk_libs DCMTK::DCMTK)

The change to DCMTK::DCMTK has to do with how DCMTK changed its exports in DCMTKTargets.cmake to a namespaced version (DCMTK::...) which also offers DCMTK::DCMTK for conveniently adding all DCMTK lib targets at once. (Maybe the old version with ${DCMTK_LIBRARIES}) also can be fixed, but why not use DCMTK::DCMTK if its available.)

However, during superbuild, dcmqi also downloads a specific ITK version:
https;//github.com/Slicer/ITK.git, commit be914a
That ITK copy also uses DCMTK.

The ITK version downloaded uses in Modules/ThirdParty/DCMTK/CMakeLists.txt:
set(ITKDCMTK_LIBRARIES ${DCMTK_LIBRARIES})

This would need to be changed to:
set(ITKDCMTK_LIBRARIES DCMTK:::DCMTK)
and then dcmqi successfully builds.

I don't understand the full mechanics of the dcmqi superbuild, so I am not sure what to make out of this:

• Can we fix this in ITK?
• Or can/should we patch the downloaded ITK to use DCMTK::DCMTK?
• And/Or, can/should the downloaded ITK inherit the linked DCMTK libraries from dcmqi (I favor DCMTK::DCMTK)?
• Is there an easier fix?
• Do we need to update Slicer's DCMTK as well? (I think they use the same old DCMTK commit as dcmqi right now)

[fedorov]

Michael, I believe the idea one idea behind the superbuild is to support the situation where dcmqi is built using ITK/DCMTK and other dependencies that are already available outside of dcmqi suiperbuld (which is the case when it is built as a Slicer extension). I do not know for sure, but maybe that somehow contributes to figuring out the solution.

Have you tried building with new DCMTK by pointing to it in the cmake variable as we do here: https://github.com/QIICR/dcmqi/blob/master/.github/workflows/cmake-win.yml#L60 ?

[michaelonken]

Thanks Andrey.

The problem at the core seems to be that ITK uses ${DCMTK_LIBRARIES} which is the list of library names (ofstd, dcmdata,...). ITK as such build and links fine also with a recent DCMTK.

DCMQI then links against ITK and DCMTK, plus as I understand it inherits the library requirements from ITK in ${ITK_LIBRARIES}. The latter still contains a list of DCMTK library names resulting in linker flags -lofstd -ldcmdata and so on. But, DCMQI build does not seem to specify the library path where to find these libraries. This worked for some reason for older DCMTK versions probably due to changed variables in DCMTKTargets/Config.cmake .

I can and want to change DCMQI linkage to link to the newly DCMTK-defined target DCMTK::DCMTK which then links against all DCMTK libraries, using their full library path, i.e. -l/home/test/dcmtk-build/lib/ofstd.a . However, the linker still adds additionally -lofstd -ldcmdata etc. I tried a long time to remove that part from the DCMQI linker command inherited by ${ITK_LIBRARIES}. I tried to remove the pure library names from the linker command by removing them from ${ITK_LIBRARIES} in DCMQI CMake files but without effect; or I removed it too early or too late in the CMake process.

So, if someone knows how to remove these effectively from the DCMQI linker call let me know.

Another solution is to patch the downloaded ITK to also link against DCMTK::DCMTK if it finds a recent DCMTK but that's probably even more hackish (but works if I do this manually). Or wait that DCMTK::DCMTK or another mechanism goes into ITK some day.

An update to a recent DCMTK version is required by this PR but also for the desired feature to not fail writing segmentation objects etc. when some attributes have invalid values. Also I think it makes sense to update DCMTK more often, since the version in DCMQI/Slicer is quite old.

[fedorov]

I agree, it is important to figure this out - both for dcmqi and Slicer! I started a discussion in Slicer forum: https://discourse.slicer.org/t/slicer-dcmtk-needs-an-update/34363.

[fedorov]

Relevant work from @jamesobutler in Slicer/Slicer#6709.

[fedorov]

@jadh4v @thewtex I forgot to bring this up at the call today. There is this pending issue where Michael had troubles building dcmqi with the latest DCMTK version due to linker errors. Were you able to build it with the latest DCMTK?

[thewtex]

We are able to build DCMQI against the latest DCMTK. However, we may want to expose more DCMTK modules in what ITK exposes if people want to use ITK's DCMTK for DCMQI. In the long run, we should find an effort that would enable improvement of ITK's use of CMake targets so they can be used in a more fine-grained way.

[fedorov]

We are able to build DCMQI against the latest DCMTK.

I rebased Michael's branch, but it is still failing...

[fedorov]

@michaelonken it is still failing, but it is not a link error anymore

/Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DDCMTK_BUILD_IN_PROGRESS -DDCMTK_ENABLE_BUILTIN_OFICONV_DATA -DUSE_NULL_SAFE_OFSTRING -D_BSD_COMPAT -D_BSD_SOURCE -D_OSF_SOURCE -D_REENTRANT -D_XOPEN_SOURCE_EXTENDED -Dofstd_EXPORTS -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK-build/config/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/ofstd/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/oflog/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmdata/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmimgle/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmimage/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmjpeg/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmjpls/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmtls/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmnet/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmsr/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmsign/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmwlm/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmqrdb/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmpstat/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmrt/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmiod/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmfg/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmseg/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmtract/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmpmap/include -I/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/dcmect/include -I/include -D_XOPEN_SOURCE_EXTENDED -D_BSD_SOURCE -D_BSD_COMPAT -D_OSF_SOURCE -D_DARWIN_C_SOURCE -fPIC -g -DDEBUG -std=c++14 -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -mmacosx-version-min=14.0 -Wall -MD -MT ofstd/libsrc/CMakeFiles/ofstd.dir/ofchrenc.cc.o -MF ofstd/libsrc/CMakeFiles/ofstd.dir/ofchrenc.cc.o.d -o ofstd/libsrc/CMakeFiles/ofstd.dir/ofchrenc.cc.o -c /Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/ofstd/libsrc/ofchrenc.cc
/Users/runner/work/dcmqi/dcmqi/dcmqi-build/DCMTK/ofstd/libsrc/ofchrenc.cc:299:10: fatal error: 'dcmtk/oficonv/iconv.h' file not found
#include "dcmtk/oficonv/iconv.h"
         ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[michaelonken]

I think the main issue is still the linker. This is what I see:

• Linux: Linker error, DCMTK libraries not found (path to DCMTK libs wrong?).
• Windows: Linker error, DCMTK libraries not found (path to DCMTK libs wrong?)
• Mac: This is different. oficonv.h is not found but other DCMTK includes before are. I suppose it is a cache issue but did not look deeper so far. I experience see the same error if I still have an old version of DCMTK in the dcmqi build tree.

On the linker error on Linux and Windows:

I think this is still the error I saw earlier when trying to link dcmqi to a newer DCMTK. I can see the same behaviour on my own Linux box. The following seems to happen:

dcmqi/libsrc/CMakelists.txt (at least, later more) adds its "own" DCMTK libraries (via ${_dcmtk_libs}) here:

target_link_libraries(${lib_name} PUBLIC ${_dcmtk_libs} ${ITK_LIBRARIES} $<$<NOT:$<BOOL:${DCMQI_BUILTIN_JSONCPP}>>:${JsonCpp_LIBRARY}> )
${_dcmtk_libs} will contain the full paths to dcmtk libs, e.g. /home/michael/b/dcmqi_linux/DCMTK-build/lib/libdcmnet.a on my system. These are also found by the linker.

However, the call above also links to ${ITK_LIBRARIES} which contain the DCMTK libraries again, "imported" from ITK, e.g. "dcmnet;dcmdata;.." and so on, which will not be found later, since there is no related library path set.

So there are three possibilities I think:

1. Remove plain list of DCMTK libs (-ldcmdata -ldcmnet...) from list of link libraries. I suppose its coming from ITK but I am not 100% sure.
2. Add link directory where those libraries reside so linker can find them.
3. Remove dcmqi copy of DCMTK libss (-lhome/michael/b/dcmqi_linux/DCMTK-build/lib/libdcmnet.a..) from the call.

ad 1) I tried to remove DCMTK libs from ${ITK_LIBRARIES} in dcmqi/libsrc/CMakeLists.txt via
list(REMOVE_ITEM ITK_LIBRARIES ofstd oflog oficonv
but it does not help.

So my guess is that ${ITK_LIBRARIES} are added somewhere else as well. Even if remove DCMTK libs from main dcmqi/CMakeLists.txt file the "plain" list of DCMTK libs (-ldcmdata -ldcmnet...) still is being used by the linker. So somewhere in the CMake code there must be a possibility to remove DCMTK from that ITK-imported library list (?).

ad 2) I did not try this, since have bad feeling of linking against two copies of DCMTK libs. We could try to point this directory to the dcmqi copy of DCMTK libs but that would be a workaround; we should make sure every DCMTK lib only shows up once in the linker library list.

ad 3) Add the library dir where the ITK copy of DCMTK libs reside. Since I don't know in which CMake variable this directory could be found, I did not give it try yet.

I assume the preferred solution would be 1. since this would allow us to link against a newer DCMTK in dcmqi independent of what ITK uses (only one explicitly defines DCMTK_DIR or so to point to the ITK copy of DCMTK).

[fedorov]

Thank you @michaelonken.

@jcfr pinging you here since we discussed DCMTK update in Slicer yesterday.

@thewtex I tried to build dcmqi against the latest DCMTK 3.6.8, and it is failing - see #500.

[thewtex]

After @jadh4v has finished the seg and parameter map integration in ITK-Wasm, we should update the dcmtk libraries built and provided by ITK to be the union of what is there now, what dcmqi needs, and what ITK-Wasm needs.

[jcfr]

The API change in dcmqi requires an updated DCMTK.

Working on updating DCMTK in Slicer & CTK, will we have to also backport the changes from https://github.com/michaelonken/dcmtk/commits/use_dcmitem_instead_of_dcmdataset/ ?

Ideally, those should be integrated into DCMTK proper.

[michaelonken]

Thanks for the reminder, I will merge it in this week.

[jcfr]

Thanks for the reminder, I will merge it in this week.

Great 🙏

Once those are merged, we should have integrated the following pull request and it will then be trivial to update DCMTK:

• ENH: Update DCMTK from 3.6.6 to 3.6.8 Slicer/Slicer#8018

Important

We will wait for your changes before initiating the Slicer 5.8 release process 🚀

[fedorov]

Thanks for the reminder, I will merge it in this week.

Sorry, I am confused. Those would be merged upstream into DCMTK, but I thought we wanted to update Slicer/dcmqi to the latest published DCMTK release 3.6.8. Shouldn't we treat those two separately? I would think that update to 3.6.8 should be ready to go.

[sonarcloud]

Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[michaelonken]

The original patch to DCMTK that is required by this issue/patch to dcmqi is now in DCMTK and will show up on master within a few days. We are currently planning for a DCMTK release before the end of the year.