Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: use the new CMake mechanism to specify MSVC's static or DLL CRT #2887

Merged
merged 1 commit into from
Dec 30, 2021

Conversation

dzenanz
Copy link
Member

@dzenanz dzenanz commented Nov 19, 2021

Related to #2872.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

@dzenanz dzenanz requested a review from thewtex November 19, 2021 23:21
@dzenanz dzenanz added this to the ITK 5.3.0 milestone Nov 19, 2021
@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Nov 19, 2021
@dzenanz dzenanz requested a review from blowekamp November 19, 2021 23:24
@dzenanz
Copy link
Member Author

dzenanz commented Nov 19, 2021

@MrTzschr, @liupeiqiHN and @tim-evain your opinion is welcome.

I have not tested to confirm that this fixes #2872. Tim, can you test and report?

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not tried.

Does this happen on other OS?

@tim-evain
Copy link

@dzenanz
New variable ITK_MSVC_STATIC_RUNTIME_LIBRARY does correctly switch ITK config, but has no impact on underlying DCMTK configuration regarding #2872 (i.e. when off, DCMTK is still built with static CRT)

@dzenanz dzenanz marked this pull request as draft November 23, 2021 21:05
@dzenanz
Copy link
Member Author

dzenanz commented Nov 23, 2021

This needs an addition of passing this flag to external projects, such as DCMTK and Expat, but probably others, too.

@hjmjohnson
Copy link
Member

@dzenanz Is this still a draft?

@dzenanz
Copy link
Member Author

dzenanz commented Dec 10, 2021

Yes. While this commit on its own is fine, some logical connected changes are needed to make this resolved, as I commented earlier.

@dzenanz dzenanz marked this pull request as ready for review December 29, 2021 20:09
@github-actions github-actions bot added the area:ThirdParty Issues affecting the ThirdParty module label Dec 29, 2021
@dzenanz
Copy link
Member Author

dzenanz commented Dec 29, 2021

@tim-evain I just updated this. Can you test now?

@tim-evain
Copy link

@dzenanz DCMTK is still built against static CRT when ITK_MSVC_STATIC_RUNTIME_LIBRARY is OFF.
I do see CMAKE_MSVC_RUNTIME_LIBRARY:STRING=MultiThreadedDLL in the CMakeCache of the external project build, but the compiler flags still have /MT(d). I'm not sure how that interacts with the property setting made by CMAKE_MSVC_RUNTIME_LIBRARY but I guess compiler flags overwrite that setting?

@dzenanz
Copy link
Member Author

dzenanz commented Dec 30, 2021

Policy 91 needs to be set to NEW for the CMAKE_MSVC_RUNTIME_LIBRARY to have an effect. The policy could be set manually, or by requiring minimum CMake version of 3.15 or higher. Otherwise compiler flags govern which CRT will be linked.

I guess DCMTK sets those flag somewhere, as it want to link to static CRT. And since it has CMP0091 implicitly as OLD, passed setting has no effect.

@dzenanz
Copy link
Member Author

dzenanz commented Dec 30, 2021

Even the latest DCMTK tops out at CMake 3.13.2.

@dzenanz dzenanz requested a review from hjmjohnson December 30, 2021 15:00
Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a windows expert, but all these changes appear reasonable.

@hjmjohnson hjmjohnson merged commit 4b010e7 into InsightSoftwareConsortium:master Dec 30, 2021
@dzenanz dzenanz deleted the msvcStaticCRT branch December 30, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants