Open tim-evain opened 2 years ago
We should switch CMake-policy 91 to new style. This was last touched by #2609.
@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?
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.
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.
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?
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?
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.
With #3119 merged, this should be a bit easier.
DCMTK is bumped again in #4503 .
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
ITK cache:
ITKDCMTK_ExtProject cache:
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