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

Build fails on mac due to iconv undefined symbols #395

Closed pieper closed 4 years ago

pieper commented 4 years ago

While testing https://github.com/QIICR/dcmqi/issues/394, I also found that the current master isn't building on mac (10.14.6 mojave). Same machine builds Slicer fine, so there's something off in the dcmqi superbuild configuration.

[  2%] Linking CXX executable ../../bin/ofstd_tests
Undefined symbols for architecture x86_64:
  "_iconv", referenced from:
      OFCharacterEncoding::Implementation::convert(OFString&, char const*, unsigned long) in libofstd.a(ofchrenc.cc.o)
  "_iconv_close", referenced from:
      OFCharacterEncoding::Implementation::~Implementation() in libofstd.a(ofchrenc.cc.o)
  "_iconv_open", referenced from:
      OFCharacterEncoding::Implementation::create(OFString const&, OFString const&, OFCondition&) in libofstd.a(ofchrenc.cc.o)
  "_iconvctl", referenced from:
      OFCharacterEncoding::Implementation::getConversionFlags() const in libofstd.a(ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a(ofchrenc.cc.o)
  "_locale_charset", referenced from:
      OFCharacterEncoding::Implementation::getLocaleEncoding() in libofstd.a(ofchrenc.cc.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[5]: *** [bin/ofstd_tests] Error 1
make[4]: *** [ofstd/tests/CMakeFiles/ofstd_tests.dir/all] Error 2
make[3]: *** [all] Error 2
make[2]: *** [DCMTK-prefix/src/DCMTK-stamp/DCMTK-build] Error 2
make[1]: *** [CMakeFiles/DCMTK.dir/all] Error 2
make: *** [all] Error 2
fedorov commented 4 years ago

I remember seeing something like this before. I think there is some conflict if you have a locally installed iconv - it's picked up by cmake erroneously. I managed to find that discussion - can you look if that helps? https://github.com/Slicer/SlicerBuildEnvironment/issues/10

pieper commented 4 years ago

I managed to find that discussion - can you look if that helps? Slicer/SlicerBuildEnvironment#10

I skimmed through but didn't see a solution. Was there a particular thing you thought we should try?

fedorov commented 4 years ago

Do you have iconv or ICU installed via port or brew?

pieper commented 4 years ago

I do have dcmtk installed via brew.

brew list shows icu4c, which appears to provide it: https://formulae.brew.sh/formula/icu4c

fedorov commented 4 years ago

Yes, most likely this is the culprit. I've just now tested on my system (same macOS, and latest XCode, with all updates applied), and I don't have this build problem.

Are you willing to experiment uninstalling ICU? Perhaps it is only used by DCMTK, and you have DCMTK in Slicer anyway? You will also get the latest release of DCMTK within dcmqi ;-)

The other alternative would be for me to do brew dcmtk and see what is happening, but most likely it is some cmake magic.

I recall at some point some version of VTK in Slicer had a similar issue, but I don't recall where that discussion was happening to find how it was solved there...

pieper commented 4 years ago

Several other brew tools depend on icu4c (e.g. ffmpeg) but as an experiment I was able to force remove it and then dcmtk and dcmqi build as expected (with tons of warnings).

After that, reinstalling dcmtk works, so this approach is a workaround if anyone needs it. But of course fixing the build system would be better.

fedorov commented 4 years ago

Great, thank you for testing and confirming this!

of course fixing the build system would be better

Of course. Probably the solution can be found somewhere in Slicer build system, I am pretty sure this came up there too.

fedorov commented 4 years ago

@michaelonken @jriesmeier do you have any comments on this? I vaguely remember we might have discussed this with Michael earlier. Maybe this came up for DCMTK? It's more a DCMTK than dcmqi issue.

fedorov commented 4 years ago

... and the issues searching DCMTK forum are still there:

image

jriesmeier commented 4 years ago

@pieper First of all, what are the values of the CMake variables DCMTK_ENABLE_CHARSET_CONVERSION, DCMTK_WITH_ICU, DCMTK_WITH_ICONV, DCMTK_WITH_STDLIBC_ICONV?

@fedorov Does searching for "libicu" work for you?

pieper commented 4 years ago

Hi @jriesmeier -

Here's what I did:

rm -rf *
cmake ../dcmqi 2>&1 | tee /tmp/cmake.log
make 2>&1 | tee /tmp/make.log

results in the same build error posted above.

The variables are:

DCMTK_ENABLE_CHARSET_CONVERSION:STRING=<disabled>
DCMTK_WITH_ICU:BOOL=OFF
DCMTK_WITH_ICONV:BOOL=OFF
DCMTK_WITH_STDLIBC_ICONV:BOOL=OFF

I put the cmake and make logs along with the CMakeCache.txt for dcmqi and dcmtk here: https://www.dropbox.com/transfer/AAAAAAPHXrTf61wyET9WTYO5iHsMj0Yuv99OqyK3L2z2jb-ieh17xE8 (valid for 6 days it says).

jriesmeier commented 4 years ago

So character set conversion is disabled (or no support found). Could you also provide the linker output in verbose mode (i.e. show the libraries that are linked to the DCMTK tools)?

pieper commented 4 years ago

Is this enough? I could give you the full verbose build log if you prefer.

[  2%] Linking CXX executable ../../bin/ofstd_tests
cd /Users/pieper/idc/dcmqi-superbuild/DCMTK-build/ofstd/tests && /usr/local/Cellar/cmake/3.15.5/bin/cmake -E cmake_link_script CMakeFiles/ofstd_tests.dir/link.txt --verbose=1
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -D_XOPEN_SOURCE_EXTENDED -D_BSD_SOURCE -D_BSD_COMPAT -D_OSF_SOURCE -D_DARWIN_C_SOURCE -fPIC -g -DDEBUG -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.15 -Wl,-search_paths_first -Wl,-headerpad_max_install_names  CMakeFiles/ofstd_tests.dir/tests.cc.o CMakeFiles/ofstd_tests.dir/tatof.cc.o CMakeFiles/ofstd_tests.dir/terror.cc.o CMakeFiles/ofstd_tests.dir/tmap.cc.o CMakeFiles/ofstd_tests.dir/tvec.cc.o CMakeFiles/ofstd_tests.dir/tfilsys.cc.o CMakeFiles/ofstd_tests.dir/tftoa.cc.o CMakeFiles/ofstd_tests.dir/tthread.cc.o CMakeFiles/ofstd_tests.dir/tbase64.cc.o CMakeFiles/ofstd_tests.dir/tstring.cc.o CMakeFiles/ofstd_tests.dir/tstrutl.cc.o CMakeFiles/ofstd_tests.dir/tlist.cc.o CMakeFiles/ofstd_tests.dir/tstack.cc.o CMakeFiles/ofstd_tests.dir/tofdatim.cc.o CMakeFiles/ofstd_tests.dir/tofstd.cc.o CMakeFiles/ofstd_tests.dir/tmarkup.cc.o CMakeFiles/ofstd_tests.dir/tchrenc.cc.o CMakeFiles/ofstd_tests.dir/txml.cc.o CMakeFiles/ofstd_tests.dir/tuuid.cc.o CMakeFiles/ofstd_tests.dir/toffile.cc.o CMakeFiles/ofstd_tests.dir/tmem.cc.o CMakeFiles/ofstd_tests.dir/toption.cc.o CMakeFiles/ofstd_tests.dir/ttuple.cc.o CMakeFiles/ofstd_tests.dir/tlimits.cc.o CMakeFiles/ofstd_tests.dir/tvariant.cc.o  -o ../../bin/ofstd_tests ../../lib/libofstd.a -lpthread 
Undefined symbols for architecture x86_64:
  "_iconv", referenced from:
      OFCharacterEncoding::Implementation::convert(OFString&, char const*, unsigned long) in libofstd.a(ofchrenc.cc.o)
  "_iconv_close", referenced from:
      OFCharacterEncoding::Implementation::~Implementation() in libofstd.a(ofchrenc.cc.o)
  "_iconv_open", referenced from:
      OFCharacterEncoding::Implementation::create(OFString const&, OFString const&, OFCondition&) in libofstd.a(ofchrenc.cc.o)
  "_iconvctl", referenced from:
      OFCharacterEncoding::Implementation::getConversionFlags() const in libofstd.a(ofchrenc.cc.o)
      OFCharacterEncoding::Implementation::setConversionFlags(unsigned int) in libofstd.a(ofchrenc.cc.o)
  "_locale_charset", referenced from:
      OFCharacterEncoding::Implementation::getLocaleEncoding() in libofstd.a(ofchrenc.cc.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [bin/ofstd_tests] Error 1
make[1]: *** [ofstd/tests/CMakeFiles/ofstd_tests.dir/all] Error 2
make: *** [all] Error 2
jriesmeier commented 4 years ago

Could you please check whether a different DCMTK development package (i.e. with headers and libraries) is installed on this particular system in a standard path (e.g. /usr, /usr/local or the like)?

pieper commented 4 years ago

Hi @jriesmeier - You probably saw that we ran into the same issue on Slicer builds. So I have the homebrew packages uninstalled at the moment. Let me reinstall and get back to you.

pieper commented 4 years ago

Yes, the homebrew package installs /usr/local/include/dcmtk with all the headers.

There are also libs (e.g. libdcmdata.a, libdcmsr.a, etc) in /usr/local/lib.

These didn't previously cause a problem for the dcmtk build in Slicer, but maybe now they do.

jriesmeier commented 4 years ago

Does compilation and linking of dcmqi work when you deinstall the homebew package of dcmtk?

pieper commented 4 years ago

Does compilation and linking of dcmqi work when you deinstall the homebew package of dcmtk?

Yes, that's the workaround for now. Edit: also uninstalled icu4c - let me try only uninstalling dcmtk.

jriesmeier commented 4 years ago

These didn't previously cause a problem for the dcmtk build in Slicer, but maybe now they do.

Which version of dcmtk did Slicer use before?

pieper commented 4 years ago

It's the one described in this diff: https://github.com/Slicer/Slicer/commit/6523a62d776e64f970c554978a3c3a8f26022db5

pieper commented 4 years ago

let me try only uninstalling dcmtk

it's enough just to uninstall dcmtk. Leaving icu4c installed doesn't break the build.

jriesmeier commented 4 years ago

So it seems that the standard library paths (like /usr/local/lib) have higher priorty than the relative path "../../lib/libofstd.a". Don't really know why this should have changed since DCMTK snapshot 3.6.3 (20180621).

pieper commented 4 years ago

Yes, that is odd. Are we sure it was a relative path in the previous version? I guess I could revert the change and try a verbose build.

pieper commented 4 years ago

@jriesmeier - -I/usr/local/include is part of the compile line, so the homebrew dcmtk headers are being picked up. Do you know why that would be there?

cd /Users/pieper/idc/dcmqi-superbuild/DCMTK-build/ofstd/tests && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -DDCMTK_BUILD_IN_PROGRESS -DUSE_NULL_SAFE_OFSTRING -D_BSD_COMPAT -D_BSD_SOURCE -D_OSF_SOURCE -D_REENTRANT -D_XOPEN_SOURCE_EXTENDED -I/usr/local/include -I/usr/local/Cellar/openjpeg/2.3.1/include/openjpeg-2.3 -I/Users/pieper/idc/dcmqi-superbuild/DCMTK-build/config/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/ofstd/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/oflog/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmdata/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmimgle/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmimage/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmjpeg/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmjpls/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmtls/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmnet/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmsr/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmsign/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmwlm/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmqrdb/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmpstat/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmrt/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmiod/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmfg/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmseg/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmtract/include -I/Users/pieper/idc/dcmqi-superbuild/DCMTK/dcmpmap/include  -D_XOPEN_SOURCE_EXTENDED -D_BSD_SOURCE -D_BSD_COMPAT -D_OSF_SOURCE -D_DARWIN_C_SOURCE -fPIC -g -DDEBUG -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.15   -std=c++98 -o CMakeFiles/ofstd_tests.dir/tvariant.cc.o -c /Users/pieper/idc/dcmqi-superbuild/DCMTK/ofstd/tests/tvariant.cc
fedorov commented 4 years ago

@pieper @jriesmeier thank you for continuing to investigate this!

jcfr commented 4 years ago

Do you know why that would be there?

On the build machine, do you have anything in the folder ~/.cmake/packages/ ?

jriesmeier commented 4 years ago

-I/usr/local/include is part of the compile line, so the homebrew dcmtk headers are being picked up. Do you know why that would be there?

I guess this is because the header files of one of more of the external support libraries are located there (e.g. zlib or openssl).

pieper commented 4 years ago

On the build machine, do you have anything in the folder ~/.cmake/packages/ ?

Yes, but I have no idea why...

$ ls -R ~/.cmake/packages/
CIP     RapidJSON

/Users/pieper/.cmake/packages//CIP:
a4c5869efa666c001cb7e06b6d6adc50

/Users/pieper/.cmake/packages//RapidJSON:
8ba20786c81340c6fbe428a36fd5c1f0    bb38345479a9800c463909923aee1699    f76455c02026d255bf544f9d25423c95
pieper commented 4 years ago

I removed ~/.cmake/packages completely and still get the same link error.

jcfr commented 4 years ago

Yes, but I have no idea why...

That happen for CMake project who were using a now deprecated feature (CMake package registry) and this could have unintended side effects.

We ruled this out.

pieper commented 4 years ago

I found this clue:

$ grep -r "/usr/local/include" ./DCMTK-build/CMakeCache.txt 
./DCMTK-build/CMakeCache.txt:SNDFILE_INCLUDE_DIR:PATH=/usr/local/include
jriesmeier commented 4 years ago

Could you check whether the advanced option DCMTK_WITH_SNDFILE is ON or OFF?

pieper commented 4 years ago

It's turned on in DCMTK-build/CMakeCache.txt

//Configure DCMTK with support for SNDFILE.
DCMTK_WITH_SNDFILE:BOOL=ON

But I would guess it should be off, since it appears to be used in a tools that's not even public.

https://github.com/DCMTK/dcmtk/blob/master/CMake/3rdparty.cmake#L27-L30

pieper commented 4 years ago

It's an interesting issue here - it seems to me this isn't just a mac issue, but anytime someone has a system-installed dcmtk but wants to build locally they may end up getting similar errors if some other package's headers (sndfile in this case) are in the same include directory with the system dcmtk.

pieper commented 4 years ago

Hmm, link I pasted above appears to be in a WIN32 block. Not sure where this is getting turned on.

pieper commented 4 years ago

Confirming that if I explicitly turn off the SNDFILE option then the build completes (dcmtk and dcmqi).

I'll make a pull request for dcmqi and Slicer to set the flag off in the superbuild.

fedorov commented 4 years ago

Could this also be the cause of this old issue https://github.com/commontk/CTK/issues/178?

fedorov commented 4 years ago

Resolved by #397

pieper commented 4 years ago

Could this also be the cause of this old issue commontk/CTK#178?

This one is different, but a bit related. In that one CTK turned off iconv in our build. In this one the system dcmtk headers have iconv turned on but our local libraries have them turned off. Since the system headers were used in our build, the mismatch in iconv led to the link error.

But as I mentioned above, this could be a real issue not just on mac, and I think it could lead to very subtle errors that wouldn't be caught at compile time. I don't have a specific example, but some small version skew could change the size of a struct or otherwise lead to an obscure memory corruption. It would be good to find a general way to keep this from happening.

jriesmeier commented 4 years ago

It would be good to find a general way to keep this from happening.

I guess that changing the order of include directories could solve this issue in a general way.

Here's a proposed patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt                                                                                                                         
index 78871ef..5ae6207 100644                                                                                                                                        
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -18,9 +18,6 @@ cmake_policy(VERSION "${DCMTK_CMAKE_POLICY_VERSION}")
 # Declare project
 project(DCMTK)

-# Check the build system
-include(CMake/dcmtkPrepare.cmake NO_POLICY_SCOPE)
-
 #-----------------------------------------------------------------------------
 # General project settings to configure DCMTK build process
 #-----------------------------------------------------------------------------
@@ -33,9 +30,7 @@ set(DCMTK_MODULES ofstd oflog dcmdata dcmimgle
   dcmseg dcmtract dcmpmap dcmect
   CACHE STRING "List of modules that should be built.")

-#-----------------------------------------------------------------------------
 # Include directories
-#-----------------------------------------------------------------------------

 set(DCMTK_INCLUDE_DIR "${DCMTK_BINARY_DIR}/config/include")
 foreach(inc ${DCMTK_MODULES})
@@ -45,6 +40,12 @@ endforeach()
 include_directories(${DCMTK_INCLUDE_DIR})

 #-----------------------------------------------------------------------------
+# Check the build system
+#-----------------------------------------------------------------------------
+
+include(CMake/dcmtkPrepare.cmake NO_POLICY_SCOPE)
+
+#-----------------------------------------------------------------------------
 # Prepare osconfig.h
 #-----------------------------------------------------------------------------
pieper commented 4 years ago

Confirmed @jriesmeier, moving the include(CMake/dcmtkPrepare.cmake NO_POLICY_SCOPE) line to a spot below the include_directories(${DCMTK_INCLUDE_DIR}) calls fixes the issue and will be a more robust solution than just turning off the sndfile option. 👍

I suggest adding this to the dcmtk master and future releases.

jriesmeier commented 4 years ago

@pieper Thank you for the feedback. I will make sure that this patch (or something similar) will be committed to DCMTK's git repository.

HenkMutsaerts commented 1 year ago

Yes @fedorov, we get the same error when we try to recompile dcmtk as MEX in Matlab R2022b (without installing/downloading dcmtk) it seems the libraries are there but it takes the wrong version or something.