Windows runtime linking flags (/MD or /MT) are not downstreamed to internal DCMTK build

[tim-evain]

Hello ITK team!

Description

When DCMTK is built through the external project on Windows, the runtime linking flags (/MD, /MT, /MDd, /MTd) don't seem to be downstreamed into the DCMTK config by cmake.
The configuration collides when one try to build an application, as by default ITK config set dynamic linking while DCMTK set static, (ITK and DCMTK do both build seamlessly though).

Steps to Reproduce

1. Build ITK with ITKDCMTK, ITKIODCMTK and ITKIOTransformDCMTK
2. Compare the CMAKE_CXX_FLAGS_*** from ITK and ITKDCMTK_ExtProject cmake caches

ITK cache:

# This is the CMakeCache file.
# For build in directory: c:/Libraries/Builds/ITK_5.2.1

[...]

//Flags used by the CXX compiler during all build types.
CMAKE_CXX_FLAGS:STRING=/DWIN32 /D_WINDOWS /W3 /GR /EHsc

//Flags used by the CXX compiler during DEBUG builds.
CMAKE_CXX_FLAGS_DEBUG:STRING=/MDd /Zi /Ob0 /Od /RTC1

//Flags used by the CXX compiler during MINSIZEREL builds.
CMAKE_CXX_FLAGS_MINSIZEREL:STRING=/MD /O1 /Ob1 /DNDEBUG

//Flags used by the CXX compiler during RELEASE builds.
CMAKE_CXX_FLAGS_RELEASE:STRING=/MD /O2 /Ob2 /DNDEBUG

//Flags used by the CXX compiler during RELWITHDEBINFO builds.
CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=/MD /Zi /O2 /Ob1 /DNDEBUG

ITKDCMTK_ExtProject cache:

# This is the CMakeCache file.
# For build in directory: c:/Libraries/Builds/ITK_5.2.1/Modules/ThirdParty/DCMTK/ITKDCMTK_ExtProject-build

[...]

//msvc compiler flags
CMAKE_CXX_FLAGS:STRING=/DWIN32 /D_WINDOWS /W3 /GR /EHsc

//msvc compiler flags
CMAKE_CXX_FLAGS_DEBUG:STRING=/MTd /Zi /Ob0 /Od /RTC1

//msvc compiler flags
CMAKE_CXX_FLAGS_MINSIZEREL:STRING=/MT /O1 /Ob1 /DNDEBUG

//msvc compiler flags
CMAKE_CXX_FLAGS_RELEASE:STRING=/MT /O2 /Ob2 /DNDEBUG

//msvc compiler flags
CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=/MT /Zi /O2 /Ob1 /DNDEBUG

Expected behavior

ITK config overwriting DCMTK config

Actual behavior

No ITK config overwriting

Reproducibility

100%

Versions

5.2.1

Environment

OS: Windows 10
CMake: 3.21.4
Compiler: MSVC 2019 14.29.30133

Additional Information

N/A

[dzenanz]

We should switch CMake-policy 91 to new style. This was last touched by #2609.

[tim-evain]

@dzenanz Following the update of policy 91 from PR #2887, which doesn't solve this precise bug due to DCMTK using older cmake version, I've looked into their mechanism for flag setting.
It stems in the dcmtkPrepare.cmake file with:

if(DCMTK_OVERWRITE_WIN32_COMPILER_FLAGS AND NOT BUILD_SHARED_LIBS)

  # settings for Microsoft Visual Studio
  if(CMAKE_GENERATOR MATCHES "Visual Studio .*")
    # get Visual Studio Version
    string(REGEX REPLACE "Visual Studio ([0-9]+).*" "\\1" VS_VERSION "${CMAKE_GENERATOR}")
    # these settings never change even for C or C++
    set(CMAKE_C_FLAGS_DEBUG "/MTd /Z7 /Od")
    set(CMAKE_C_FLAGS_RELEASE "/DNDEBUG /MT /O2")
    set(CMAKE_C_FLAGS_MINSIZEREL "/DNDEBUG /MT /O2")
    set(CMAKE_C_FLAGS_RELWITHDEBINFO "/DNDEBUG /MTd /Z7 /Od")
    set(CMAKE_CXX_FLAGS_DEBUG "/MTd /Z7 /Od")
    set(CMAKE_CXX_FLAGS_RELEASE "/DNDEBUG /MT /O2")
    set(CMAKE_CXX_FLAGS_MINSIZEREL "/DNDEBUG /MT /O2")
    set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "/DNDEBUG /MTd /Z7 /Od")
    # specific settings for the various Visual Studio versions
    if(VS_VERSION EQUAL 6)
      set(CMAKE_C_FLAGS "/nologo /W3 /GX /Gy /YX")
      set(CMAKE_CXX_FLAGS "/nologo /W3 /GX /Gy /YX /Zm500") # /Zm500 increments heap size which is needed on some system to compile templates in dcmimgle
    endif()
    if(VS_VERSION EQUAL 7)
      set(CMAKE_C_FLAGS "/nologo /W3 /Gy")
      set(CMAKE_CXX_FLAGS "/nologo /W3 /Gy")
    endif()
    if(VS_VERSION GREATER 7)
      set(CMAKE_C_FLAGS "/nologo /W3 /Gy /EHsc")
      set(CMAKE_CXX_FLAGS "/nologo /W3 /Gy /EHsc")
    endif()
  endif()

  # settings for Borland C++
  if(CMAKE_GENERATOR MATCHES "Borland Makefiles")
    # further settings required? not tested for a very long time!
    set(CMAKE_STANDARD_LIBRARIES "import32.lib cw32mt.lib")
  endif()

endif()

if(WIN32 AND CMAKE_GENERATOR MATCHES "Visual Studio .*")
  # Evaluate the DCMTK_COMPILE_WIN32_MULTITHREADED_DLL option and adjust
  # the runtime library setting (/MT or /MD) accordingly
  set(CompilerFlags
        CMAKE_CXX_FLAGS
        CMAKE_CXX_FLAGS_DEBUG
        CMAKE_CXX_FLAGS_RELEASE
        CMAKE_CXX_FLAGS_MINSIZEREL
        CMAKE_CXX_FLAGS_RELWITHDEBINFO
        CMAKE_C_FLAGS
        CMAKE_C_FLAGS_DEBUG
        CMAKE_C_FLAGS_RELEASE
        CMAKE_C_FLAGS_MINSIZEREL
        CMAKE_C_FLAGS_RELWITHDEBINFO
        )

  if(DCMTK_COMPILE_WIN32_MULTITHREADED_DLL OR BUILD_SHARED_LIBS)
    # Convert any /MT or /MTd option to /MD or /MDd
    foreach(CompilerFlag ${CompilerFlags})
        string(REPLACE "/MT" "/MD" ${CompilerFlag} "${${CompilerFlag}}")
        set(${CompilerFlag} "${${CompilerFlag}}" CACHE STRING "msvc compiler flags" FORCE)
    endforeach()
  else()
    # Convert any /MD or /MDd option to /MT or /MTd
    foreach(CompilerFlag ${CompilerFlags})
        string(REPLACE "/MD" "/MT" ${CompilerFlag} "${${CompilerFlag}}")
        set(${CompilerFlag} "${${CompilerFlag}}" CACHE STRING "msvc compiler flags" FORCE)
    endforeach()
  endif()
endif()

In the CMakeList of the project, there is an attempt to prevent DCMTK to override the default cmake flags:

  if(MSVC)
    list(APPEND DCMTK_EP_FLAGS -DDCMTK_OVERWRITE_WIN32_COMPILER_FLAGS:BOOL=OFF)
  endif()

but this flag alone is not enough to prevent overwritting, DCMTK_COMPILE_WIN32_MULTITHREADED_DLL must also be used. (btw this is a bit counter-intuitive, and DCMTK has updated that in the upstream dcmtkPrepare.cmake).

Using the new variable, I've tried to modify the CMakeList with:

  if(MSVC)
    list(APPEND DCMTK_EP_FLAGS -DDCMTK_OVERWRITE_WIN32_COMPILER_FLAGS:BOOL=OFF)
    # As of 2021-12-31, DCMTK tops out at Cmake version 3.13.2,
    # thus doesnt support new policy CMP0091 (introduced in cmake 3.15)
    # for MSVC ABI runtime flag selection.
    # We rely on DCMTK custom options to overwrite the flags 
    # based on the CMAKE_MSVC_RUNTIME_LIBRARY variable 
    # (set by the global ITK_MSVC_STATIC_CRT option)
    # until DCMTK is updated to newer cmake version
    string(FIND ${CMAKE_MSVC_RUNTIME_LIBRARY} "DLL" MSVC_DLL_TAG_POSITION)
    if(${MSVC_DLL_TAG_POSITION} GREATER_EQUAL "0") 
      # then overwrite for DLL runtime
      list(APPEND DCMTK_EP_FLAGS -DDCMTK_COMPILE_WIN32_MULTITHREADED_DLL:BOOL=ON)
    else() 
      # then overwrite for static runtime
      list(APPEND DCMTK_EP_FLAGS -DDCMTK_COMPILE_WIN32_MULTITHREADED_DLL:BOOL=OFF)
    endif()
  endif()

and that now correctly switch between static and dynamic CRT based on the global ITK config through ITK_MSVC_STATIC_CRT.

I can make a PR with that, but I have a doubt with what to do with the DCMTK_OVERWRITE_WIN32_COMPILER_FLAGS option. As of today, we can let it OFF, but if an update on the DCMTK for ITK version is made to newer DCMTK, this will break this fix, and lock DCMTK in cmake default flags (i.e. dynamic CRT) due to the new dcmtkPrepare.cmake; except if this new version handle policy 91.
Or the option can be set to ON to be prepared, but then the compiler flags config under DCMTK_OVERWRITE_WIN32_COMPILER_FLAGS would be applied until DCMTK for ITK version update. I'm not sure of its impact.

What would be the preference here?

[dzenanz]

My preference is to first update the version of DCMTK used with ITK. I made a new fork in place of the old and pushed there the branches from the old (technically non-fork).

Hopefully, it shouldn't be hard to reapply the patched on top of the current master. I will try that now. Then the only thing left is to update the tag in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/DCMTK/DCMTKGitTag.cmake.

Then your patch for the new version should be the right thing to apply.

[dzenanz]

Sadly, reapplying those patches is not trivial due to conflicts. Luckily, it does not seem to be too hard either. Tim, you could take a crack at it.

[tim-evain]

Yes, I can try to take a look at it.
Just to be sure of the course of action because I'm never sure with git, what we need here is to git cherry-pick the 5 patching latest commits from 20200214_DCMTK_PATCHES_FOR_ITK branch into the master and resolve conflicts, is that correct?

[dzenanz]

That is the first part of it. I have already done that (sloppily) in master branch of my fork. That should be redone better. But also, each commit should be examined, and determined whether changes to it are necessary in light of other changes made to main repository since 2020-02-14. For example, in the last commit, should changes from lines 381-390 also be analogously applied to lines 266-279?

[tim-evain]

Make sense, thanks @dzenanz . That requires some focus to dig in DCMTK, I'm not sure when I'll have a moment to do so. I'll report here if progress is made.