dgobbi / vtk-dicom

A set of classes for using DICOM in VTK.
BSD 3-Clause "New" or "Revised" License
258 stars 94 forks source link

Compilation as VTK external module on Ubuntu 22.04 #222

Closed kenavolic closed 11 months ago

kenavolic commented 1 year ago

When compiling vtkDicom as external module of VTK (-DVTK_MODULE_ENABLE_VTK_vtkDICOM:STRING=WANT) on Ubuntu 22.04 with the system dcmtk there is a compilation issue:

/usr/include/dcmtk/config/osconfig.h:1144:2: error: invalid preprocessing directive #errorDCMTK
  #error\
      |  ^~~~~~
 DCMTK was configured to use C++17 features, but your compiler does not or was not configured to provide them.

The issue is due to no minimum standard requirement being set and is fixed by adding set(CMAKE_CXX_STANDARD 17) in the main vtkDicom CMakeLists.txt.

OS: Ubuntu 22.04 VTK: 9.2.5 with VTK_MODULE_ENABLE_VTK_vtkDICOM:STRING=WANT cmake: 3.24 system dcmtk: libdcmtk-dev/jammy,now 3.6.6-5 amd64

dgobbi commented 1 year ago

Thanks for the note. When you say "main CMakeLists.txt", you are referring to the VTK CMakeLists.txt? Or to the vtkDICOM CMakeLists.txt? Neither VTK or vtkDICOM is ready to set c++17 as an absolute requirement, so set(CMAKE_CXX_STANDARD 17) would have to be conditional on USE_DCMTK.

I might recommend using gdcm instead of dcmtk, which is what I use for the binary packages.

kenavolic commented 1 year ago

I edited my comment. As we don't install gdcm we rely on dcmtk backend for Dicom handling. A cleaner solution would be to update the Source/CMakelists.txt of vtkDicom with something like that (but better than that ;)):

if(USE_DCMTK)
  find_package(DCMTK)
  if(NOT DCMTK_FOUND)
    message(FATAL_ERROR "DCMTK not found or incomplete.")
  endif()
  foreach(_dcmtk_inc_dir ${DCMTK_INCLUDE_DIRS})
    if(EXISTS ${_dcmtk_inc_dir}/dcmtk/config/osconfig.h)
      file(READ ${_dcmtk_inc_dir}/dcmtk/config/osconfig.h _dcmtk_config)
      string(FIND "${_dcmtk_config}" "#define HAVE_CXX11 1" _cxx_11_std)
      string(FIND "${_dcmtk_config}" "#define HAVE_CXX14 1" _cxx_14_std)
      string(FIND "${_dcmtk_config}" "#define HAVE_CXX17 1" _cxx_17_std)
      if(NOT ${_cxx_17_std} EQUAL -1)
        set(CMAKE_CXX_STANDARD 17)
      elseif(NOT ${_cxx_17_std} EQUAL -1)
        set(CMAKE_CXX_STANDARD 14)
      elseif(NOT ${_cxx_11_std} EQUAL -1)
        set(CMAKE_CXX_STANDARD 11)
      endif()
    endif()  
  endforeach()
  if(${DCMTK_charls_LIBRARY})
    set(DCMTK_LIBS ${DCMTK_LIBRARIES} ${DCMTK_charls_LIBRARY})
  else()
    set(DCMTK_LIBS ${DCMTK_LIBRARIES})
  endif()
  if(APPLE)
    list(APPEND DCMTK_LIBS iconv)
  endif()
  include_directories(${DCMTK_INCLUDE_DIRS})
endif()
dgobbi commented 1 year ago

That looks like a reasonable solution (though a bit fragile because dcmtk might change its osconfig.h in the future). I can add that to the code after testing it on my own linux box.

In the meantime, adding -std=c++17 to CMAKE_CXX_FLAGS during the cmake configuration of VTK might work as a quick-and-dirty fix.

dgobbi commented 1 year ago

I've added 65ec1e7 to take care of this issue. Tomorrow I'll push a change to VTK to pick up this commit when it builds vtkDICOM as a remote module.

kenavolic commented 1 year ago

Much cleaner :) Thanks!

mathstuf commented 1 year ago

FindDCMTK should make this available through a variable. Better would be to put it on its exported targets. But this works in the meantime.

dgobbi commented 11 months ago

The vtk-dicom remote module for VTK 9.3 now has this fix. Closing.