Bug 172230 – [Win][CMake] Move MSVC compiler options to a common location

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Don Olmstead]

Currently all the MSVC definitions are in OptionsWin.cmake. This means they aren't available for different ports such as JSCOnly.

[Don Olmstead]

Created attachment 310439 [details]
Patch

This moves the MSVC options from OptionsWin over to OptionsCommon.

This is work towards enabling JSCOnly builds on windows.

[Alex Christensen]

Comment on attachment 310439 [details]
Patch

There is no way to use MSVC on a non-Windows platform.  I see no reason to move this from a Windows-specific file to a file that's used on Linux and Mac.

[Don Olmstead]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 310439 [details]
> Patch
> 
> There is no way to use MSVC on a non-Windows platform.  I see no reason to
> move this from a Windows-specific file to a file that's used on Linux and
> Mac.

Would you prefer compiler options in their own files? Like a OptionsMSVC.cmake?

I'm trying to consolidate the options so they can be used to build JSCOnly on Windows. As it is JSCOnly does not build on Windows at all.

[Alex Christensen]

OptionsMSVC.cmake would be better than OptionsCommon.cmake

[Don Olmstead]

Created attachment 310581 [details]
Patch

Moving MSVC options to a new file as requested.

[Konstantin Tokarev]

I would prefer if the following options were not moved to OptionsMSVC.cmake: /Zi for release builds, disabling RTTI (GR-), using MT runtime instead of MD or MDd. 

Rationale is that this options are not universally suitable, and it's not possible to revert their effect for downstream port, without duplicating or patching OptionsMSVC.cmake (I'm speaking for Qt port now). Also, there was a proposal in the mailing list to make WinCairo using MD runtime, I think it would be wise thing to do.

I also don't think that JSCOnly should use this options.

OTOH, I would suggest moving "INCREMENTAL:NO" code from Source/WebKit/PlatformWin.cmake to OptionsMSVC.cmake, because large linking times affects pretty much any WebKit port if MSVC is used, and these options should affect binaries built in Tools too.

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #6)
> I would prefer if the following options were not moved to OptionsMSVC.cmake:
> /Zi for release builds, disabling RTTI (GR-), using MT runtime instead of MD
> or MDd. 
> 
> Rationale is that this options are not universally suitable, and it's not
> possible to revert their effect for downstream port, without duplicating or
> patching OptionsMSVC.cmake (I'm speaking for Qt port now). Also, there was a
> proposal in the mailing list to make WinCairo using MD runtime, I think it
> would be wise thing to do.
> 
> I also don't think that JSCOnly should use this options.
> 
> OTOH, I would suggest moving "INCREMENTAL:NO" code from
> Source/WebKit/PlatformWin.cmake to OptionsMSVC.cmake, because large linking
> times affects pretty much any WebKit port if MSVC is used, and these options
> should affect binaries built in Tools too.

I agree. This was just a blind copy and I was going to clean up this file in a later patch.

[Don Olmstead]

Created attachment 310682 [details]
Patch

Moving linking options into the file as requested.

[Alex Christensen]

Comment on attachment 310682 [details]
Patch

Looks good.  Let's let EWS succeed first

[Don Olmstead]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 310682 [details]
> Patch
> 
> Looks good.  Let's let EWS succeed first

Success!

[Yusuke Suzuki]

Comment on attachment 310682 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 310682 [details]
Patch

Clearing flags on attachment: 310682

Committed r217207: <http://trac.webkit.org/changeset/217207>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.