WIP: Use DcmItem in API where possible.#493
WIP: Use DcmItem in API where possible.#493michaelonken wants to merge 2 commits intoQIICR:masterfrom
Conversation
|
@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? |
|
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. |
|
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. |
|
Ok, here is how one could fix it: Earlier I already changed dcmqi/libsrc/CMakeLists.txt to replace The change to DCMTK::DCMTK has to do with how DCMTK changed its exports in DCMTKTargets.cmake to a namespaced version ( However, during superbuild, dcmqi also downloads a specific ITK version: The ITK version downloaded uses in This would need to be changed to: I don't understand the full mechanics of the dcmqi superbuild, so I am not sure what to make out of this:
|
|
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 ? |
|
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. |
|
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. |
|
Relevant work from @jamesobutler in Slicer/Slicer#6709. |
|
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. |
dfae36d to
dd6e8c3
Compare
I rebased Michael's branch, but it is still failing... |
b17d403 to
dd6e8c3
Compare
|
@michaelonken it is still failing, but it is not a link error anymore |
|
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:
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:
ad 1) I tried to remove DCMTK libs from ${ITK_LIBRARIES} in dcmqi/libsrc/CMakeLists.txt via 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). |
|
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. |
|
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. |
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. |
|
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: Important We will wait for your changes before initiating the |
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. |
|
|
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. |
|
blocked by #500 |
|
@michaelonken This PR can be closed because 09e1833 was ultimately integrated which includes changes to use DcmItem? |
|
No, it cannot be closed since the changes in this PR rely on the updated DCMTK but also in part apply to dcmqi. The dcmqi part must still be merged. I can take care of this if you like? |
|
@michaelonken I was just wondering since this is your PR and it seems to no longer be blocked by #500 (from #493 (comment)). Since DCMQI appears to have already been updated to a DCMTK 3.6.8+ version in 09e1833, it seems like nothing is blocking more DcmItem usage or that it is already complete because DcmItem usage was included as part of 09e1833? The merge conflicts here seem to suggest these changes have already been integrated. |
|
Hey James, No, this PR is not blocked in general. The DCMTK changes are done and ready. But as you can see in the changes, also dcmqi files have to be changed (by this PR), and these changes should undergo final testing before merging them into master. Give me some days to review this and I get back to you. Is that ok for you? |
|
Please let me know if I can be of any help. I planned to finally take care of #390 next month. End if would be realy great, if we would have this PR in by then to base the work on that. 🙏 |
|
@michaelonken Based on my comment in #493 (comment) and your response, it seemed like you were suggesting this PR contained more code changes necessary for this repo? If that's not the case, then I suggest that you close this PR. |
|
Hey James, yes, I thought more changes were needed but after looking more carefully, the referenced commit should already do the trick. However, I would love to have the confirmation from @rfloca before finally closing it. (P.S: Sorry James, seems you have been right earlier) |
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...