search

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
240 stars 61 forks source link

WIP: Use DcmItem in API where possible. #493

Open michaelonken opened 9 months ago

michaelonken commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

I got this.

Probably fixed with dfae36d1ab896fad294caa148a92b9864061280a. 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 commented 9 months ago

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:

fedorov commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Relevant work from @jamesobutler in https://github.com/Slicer/Slicer/pull/6709.

fedorov commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

We are able to build DCMQI against the latest DCMTK.

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

fedorov commented 5 months ago

@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 commented 5 months ago

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

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 commented 5 months ago

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 commented 5 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

jcfr commented 3 weeks ago

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

Great :pray:

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

[!IMPORTANT] We will wait for your changes before initiating the Slicer 5.8 release process :rocket:

fedorov commented 3 weeks ago

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[bot] commented 2 days ago

Quality Gate Failed 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