search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.4k stars 661 forks source link

ENH: Bump DCMTK to 2024-03-11 master #4503

Closed thewtex closed 5 months ago

thewtex commented 5 months ago

We can drop the arith.h patch because it was removed in:

https://github.com/DCMTK/dcmtk/commit/5714b4c043dc0c2cbc4c66ea104cc4c9c5659b32

thewtex commented 5 months ago

CC @michaelonken

hjmjohnson commented 5 months ago

@thewtex Build on M2 mac with python and DCMTK options turned on triggers:

DCMTK_USE_ICU:BOOL=OFF
ITK_USE_SYSTEM_DCMTK:BOOL=OFF
Module_IOTransformDCMTK_GIT_TAG:STRING=e97e0e8c27809eea1834dd534a47fc06168e3e45
Module_ITKDCMTK:BOOL=ON
Module_ITKIODCMTK:BOOL=ON
ITK_WRAP_PYTHON:BOOL=ON
Undefined symbols for architecture arm64:
  "_OFiconv", referenced from:
      OFCharacterEncoding::Implementation::convert(OFString&, char const*, unsigned long) in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::convert(OFString&, char const*, unsigned long) in libofstd.a[2](ofchrenc.cc.o)
  "_OFiconv_cleanup", referenced from:
      OFiconvCleanupHelper::~OFiconvCleanupHelper() in libofstd.a[2](ofchrenc.cc.o)
  "_OFiconv_close", referenced from:
      OFCharacterEncoding::Implementation::~Implementation() in libofstd.a[2](ofchrenc.cc.o)
  "_OFiconv_open", referenced from:
      OFCharacterEncoding::Implementation::create(OFString const&, OFString const&, OFCondition&) in libofstd.a[2](ofchrenc.cc.o)
  "_OFiconvctl", referenced from:
      OFCharacterEncoding::getConversionFlags() const in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::getConversionFlags() const in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a[2](ofchrenc.cc.o)
      ...
  "_OFlocale_charset", referenced from:
      OFCharacterEncoding::getLocaleEncoding() in libofstd.a[2](ofchrenc.cc.o)
      OFCharacterEncoding::getLocaleEncoding() in libofstd.a[2](ofchrenc.cc.o)
  "_get_oficonv_logger_callback", referenced from:
      DcmSpecificCharacterSet::DcmSpecificCharacterSet() in libdcmdata.a[48](dcspchrs.cc.o)
  "_set_oficonv_logger_callback", referenced from:
      DcmSpecificCharacterSet::DcmSpecificCharacterSet() in libdcmdata.a[48](dcspchrs.cc.o)
ld: symbol(s) not found for architecture arm64
thewtex commented 5 months ago

@hjmjohnson thanks for testing!

@michaelonken is DCMTK tested on macOS ARM? Do you know the source of these issues?

issakomi commented 5 months ago

is DCMTK tested on macOS ARM?

Not only arm64 macOS. Linking fails on x86_64 Linux too. AFAIK, usual tests triggered by PR do not enable DCMTK IO.

[ 84%] Linking CXX executable ../../../../bin/IOTransformDCMTKTestDriver
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libdcmdata.a(dcspchrs.cc.o): in function `DcmSpecificCharacterSet::DcmSpecificCharacterSet()':
dcspchrs.cc:(.text+0xe06): undefined reference to `get_oficonv_logger_callback'
/usr/bin/ld: dcspchrs.cc:(.text+0xe28): undefined reference to `set_oficonv_logger_callback'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::getLocaleEncoding()':
ofchrenc.cc:(.text+0x3f0): undefined reference to `OFlocale_charset'
/usr/bin/ld: ofchrenc.cc:(.text+0x404): undefined reference to `OFlocale_charset'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::~OFCharacterEncoding()':
ofchrenc.cc:(.text+0x4b0): undefined reference to `OFiconv_close'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::operator=(OFCharacterEncoding const&)':
ofchrenc.cc:(.text+0x62e): undefined reference to `OFiconv_close'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::clear()':
ofchrenc.cc:(.text+0x7bf): undefined reference to `OFiconv_close'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::getConversionFlags() const':
ofchrenc.cc:(.text+0x917): undefined reference to `OFiconvctl'
/usr/bin/ld: ofchrenc.cc:(.text+0x930): undefined reference to `OFiconvctl'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::setConversionFlags(unsigned int)':
ofchrenc.cc:(.text+0x9d3): undefined reference to `OFiconvctl'
/usr/bin/ld: ofchrenc.cc:(.text+0xa5f): undefined reference to `OFiconvctl'
/usr/bin/ld: ofchrenc.cc:(.text+0xa7a): undefined reference to `OFiconvctl'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o):ofchrenc.cc:(.text+0xaba): more undefined references to `OFiconvctl' follow
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::selectEncoding(OFString const&, OFString const&)':
ofchrenc.cc:(.text+0xb36): undefined reference to `OFiconv_open'
/usr/bin/ld: ofchrenc.cc:(.text+0xb60): undefined reference to `OFiconvctl'
/usr/bin/ld: ofchrenc.cc:(.text+0xbb1): undefined reference to `OFiconv_close'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFiconvCleanupHelper::~OFiconvCleanupHelper()':
ofchrenc.cc:(.text._ZN20OFiconvCleanupHelperD2Ev[_ZN20OFiconvCleanupHelperD5Ev]+0x5): undefined reference to `OFiconv_cleanup'
/usr/bin/ld: ../../../ThirdParty/DCMTK/ITKDCMTK_ExtProject-build/lib/libofstd.a(ofchrenc.cc.o): in function `OFCharacterEncoding::Implementation::convert(OFString&, char const*, unsigned long)':
ofchrenc.cc:(.text._ZN19OFCharacterEncoding14Implementation7convertER8OFStringPKcm[_ZN19OFCharacterEncoding14Implementation7convertER8OFStringPKcm]+0x6d): undefined reference to `OFiconv'
/usr/bin/ld: ofchrenc.cc:(.text._ZN19OFCharacterEncoding14Implementation7convertER8OFStringPKcm[_ZN19OFCharacterEncoding14Implementation7convertER8OFStringPKcm]+0x10e): undefined reference to `OFiconv'
collect2: error: ld returned 1 exit status
make[2]: *** [Modules/Remote/IOTransformDCMTK/test/CMakeFiles/IOTransformDCMTKTestDriver.dir/build.make:207: bin/IOTransformDCMTKTestDriver] Error 1
make[1]: *** [CMakeFiles/Makefile2:17160: Modules/Remote/IOTransformDCMTK/test/CMakeFiles/IOTransformDCMTKTestDriver.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
michaelonken commented 5 months ago

We don't have an arm64 build right now but x86_64 Linux for sure. It looks like maybe just oficonv is not in the list of link libraries? It's has been added to DCMTK for the builtin iconv functionality.

[EDIT] We also have an arm64 build (right now AppleClang 15.0.0 posix arm64-apple-darwin22.5.) , so it should work in general

issakomi commented 5 months ago

It looks like maybe just oficonv is not in the list of link libraries?

Thank you, Michael. Yes, I have added oficonv here

  set(_ITKDCMTK_LIB_NAMES dcmdata dcmimage dcmimgle dcmjpeg dcmjpls
-      dcmnet dcmpstat dcmqrdb dcmsr dcmtls ijg12 ijg16 ijg8 oflog ofstd)
  set(_ITKDCMTK_LIB_NAMES dcmdata dcmimage dcmimgle dcmjpeg dcmjpls
+      dcmnet dcmpstat dcmqrdb dcmsr dcmtls ijg12 ijg16 ijg8 oflog ofstd oficonv)

Seems to work (Linux x86_64).

michaelonken commented 5 months ago

There is another CMake option that might be interesting in this context: by default, the oficonv library makes use of character set conversion tables that are loaded from files during runtime. In order to pre-compile those tables into the oficonv library, you can set the CMake option DCMTK_ENABLE_BUILTIN_OFICONV_DATA (ON).

thewtex commented 5 months ago

@issakomi @michaelonken thanks for the reviews!

[EDIT] We also have an arm64 build (right now AppleClang 15.0.0 posix arm64-apple-darwin22.5.) , so it should work in general

Yes, I have added oficonv here

Seems to work (Linux x86_64).

I did test locally on Linux x86_64, but it was not a fresh build, so it was probably using the previous ITKIODCMTK test driver binaries.

I added the library in the location noted by @issakomi and I am starting a fresh local build.

I also removed the previous workaround for macOS iconv compatibility. This has not been tested yet, but I will test locally on macOS ARM.

There is another CMake option that might be interesting in this context: by default, the oficonv library makes use of character set conversion tables that are loaded from files during runtime. In order to pre-compile those tables into the oficonv library, you can set the CMake option DCMTK_ENABLE_BUILTIN_OFICONV_DATA (ON).

Cool! I will add and test this in a follow-up commit.

thewtex commented 5 months ago

a fresh local build.

Build succeeds, ctest -L ITKIODCMTK passes with x86_64 Linux.

thewtex commented 5 months ago

Build succeeds, ctest -L ITKIODCMTK passes with x86_64 Linux.

Ditto for macOS ARM for me.

jcfr commented 5 months ago

Similarly, we will also move forward with: