Bug 170594 - [CMake][Win] Conditionally select DLL CRT or static CRT
Summary: [CMake][Win] Conditionally select DLL CRT or static CRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks: 179908
  Show dependency tree / graph
 
Reported: 2017-04-07 03:18 PDT by Fujii Hironori
Modified: 2017-11-27 13:18 PST (History)
7 users (show)

See Also:

Attachments
WIP patch (2.56 KB, patch)
2017-04-07 03:18 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
WIP patch (2.60 KB, patch)
2017-04-10 00:15 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Similar to the Fujii's patch, but some extra files covered (4.63 KB, patch)
2017-05-19 01:15 PDT, isaac+webkit 		no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2017-11-20 19:48 PST, Don Olmstead 		no flags Details | Formatted Diff | Diff
Patch (7.24 KB, patch)
2017-11-21 11:57 PST, Don Olmstead 		achristensen: review+
Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2017-11-27 11:52 PST, Don Olmstead 		no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2017-11-27 11:53 PST, Don Olmstead 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2017-04-07 03:18:11 PDT 
Brent suggested to use DLL CRT instead of static CRT (bug 157067 comment 6).

MSDN: CRT Library Features
https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx

One benefit of DLL CRT is avoiding heap mismatch crash (Bug 156831).
Any other benefits?
Comment 1 Fujii Hironori 2017-04-07 03:18:46 PDT 
Created attachment 306483 [details]
WIP patch
Comment 2 Fujii Hironori 2017-04-10 00:15:09 PDT 
Created attachment 306665 [details]
WIP patch

AppleWin port reports mismatch CRT type linkage error:

>WebKitSystemInterface.lib(WebKitSystemInterface.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MT_StaticRelease' doesn't match value 'MD_DynamicRelease' in StorageAreaImpl.obj

WebKitLibraries/win/lib32/WebKitSystemInterface.lib needs to be compiled with /MD.
Comment 3 isaac+webkit 2017-05-19 01:15:19 PDT 
Created attachment 310631 [details]
Similar to the Fujii's patch, but some extra files covered

As mentioned on webkit-dev ( https://lists.webkit.org/pipermail/webkit-dev/2017-May/029141.html ), I have been working on this as well.
I've also changed the vsprops files as well, but from looking at Fujii's patch (which I prefer over mine), it looks like these aren't modified. 
Does this mean they are no longer used and can be removed?
Comment 4 Alex Christensen 2017-05-22 10:46:16 PDT 
Unfortunately we can't remove the vsprops files yet.
Comment 5 Don Olmstead 2017-11-20 19:48:30 PST 
Created attachment 327383 [details]
Patch

This patch takes a different approach since WebKitSystemInterface.lib might not be compiled with /MD anytime soon.

This introduces MSVC_STATIC_RUNTIME which AppleWin has as ON. Then the compile and linker flags are modified according to whether /MD or /MT should be specified.
Comment 6 Don Olmstead 2017-11-20 20:38:19 PST 
Error from AppleWin is around FourCC.h again....
Comment 7 Don Olmstead 2017-11-21 11:57:02 PST 
Created attachment 327424 [details]
Patch

Put setting of MSVC_STATIC_RUNTIME before the include of OptionsWin and adding some logging for the value.

This should get AppleWin working.
Comment 8 Alex Christensen 2017-11-27 09:21:25 PST 
Comment on attachment 327424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327424&action=review

> Source/cmake/OptionsAppleWin.cmake:1
> +# FIXME: Remove after WebKitSystemInterface.lib is compiled with /MD

I'm not sure we ever intend to do this.  We would have to do changes to how we distribute Windows binaries and merge modules, etc.

> Source/cmake/OptionsMSVC.cmake:66
> +    set(MSVC_RUNTIME_LINKER_FLAGS "")

I don't think this is necessary.
Comment 9 Don Olmstead 2017-11-27 11:52:53 PST 
Created attachment 327655 [details]
Patch

Fixing review comments
Comment 10 Don Olmstead 2017-11-27 11:53:18 PST 
Created attachment 327656 [details]
Patch
Comment 11 WebKit Commit Bot 2017-11-27 13:16:53 PST 
Comment on attachment 327656 [details]
Patch

Clearing flags on attachment: 327656

Committed r225191: <https://trac.webkit.org/changeset/225191>
Comment 12 WebKit Commit Bot 2017-11-27 13:16:55 PST 
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-11-27 13:18:36 PST 
<rdar://problem/35705731>