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.