Bug 172412 - Switch windows build to Visual Studio 2017
Summary: Switch windows build to Visual Studio 2017
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Windows 10
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 174195 (view as bug list)
Depends on: 174979
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-19 23:57 PDT by isaac+webkit
Modified: 2017-12-06 20:08 PST (History)
10 users (show)

See Also:


Attachments
Patch to change to use Visual Studio 2017 for building (8.50 KB, patch)
2017-05-19 23:57 PDT, isaac+webkit
no flags Details | Formatted Diff | Diff
WIP patch (Moved #pragma init_seg) (4.21 KB, patch)
2017-07-30 16:03 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Require VS2017 for building (18.46 KB, patch)
2017-11-30 11:26 PST, Stephan Szabo
no flags Details | Formatted Diff | Diff
Require VS2017 for building (18.48 KB, patch)
2017-11-30 13:12 PST, Stephan Szabo
no flags Details | Formatted Diff | Diff
Require VS2017 for building (18.30 KB, patch)
2017-12-05 14:59 PST, Stephan Szabo
pvollan: review+
Details | Formatted Diff | Diff
Require VS2017 for building - reup (18.30 KB, patch)
2017-12-05 16:06 PST, Stephan Szabo
no flags Details | Formatted Diff | Diff
Require VS2017 for building - setting reviewer based on pre-reupload r=me (18.31 KB, patch)
2017-12-05 17:47 PST, Stephan Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description isaac+webkit 2017-05-19 23:57:20 PDT
Created attachment 310766 [details]
Patch to change to use Visual Studio 2017 for building

Switch the target compiler and build environment to Visual Studio 2017.

N.B. The attached patch seems to cause MiniBrowser at startup with the following stack trace:
WebKit\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp

 	WTF.dll!WTFCrash() Line 292	C++
 	WebKit.dll!WTF::StringImpl::assertHashIsCorrect() Line 889	C++
>	WebKit.dll!WebCore::HTMLNames::init() Line 1666	C++
 	WebKit.dll!WebCore::Frame::Frame(WebCore::Page & page, WebCore::HTMLFrameOwnerElement * ownerElement, WebCore::FrameLoaderClient & frameLoaderClient) Line 175	C++
 	WebKit.dll!WebCore::MainFrame::MainFrame(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 56	C++
 	WebKit.dll!WebCore::MainFrame::create(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 68	C++
 	WebKit.dll!WebCore::Page::Page(WebCore::PageConfiguration && pageConfiguration) Line 270	C++
 	WebKit.dll!WebView::initWithFrame(tagRECT frame, wchar_t * frameName, wchar_t * groupName) Line 3122	C++
 	MiniBrowserLib.dll!MiniBrowser::prepareViews(HWND__ * mainWnd, const tagRECT & clientRect, wchar_t * const & requestedURL, HWND__ * & viewHwnd) Line 100	C++
 	MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 160	C++
 	MiniBrowserLib.dll!dllLauncherEntryPoint(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 857	C++
 	MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 249	C++
Comment 1 Fujii Hironori 2017-05-22 18:53:17 PDT
Your callstack looks similar with Bug 171800.
Comment 2 isaac+webkit 2017-05-22 19:46:34 PDT
Yes, I thought so too. I've experimented with re-enabling static constructors by removing the line below from Source/WTF/wtf/StaticConstructors and it then works.
#pragma init_seg(".unwantedstaticinits")
Comment 3 isaac+webkit 2017-05-22 19:47:55 PDT
Also, I've managed to launch MiniBrowser in the debugger and it appears that the aData statics aren't initialized, unless that #pragma is removed from StaticConstructors.h
Comment 4 Fujii Hironori 2017-07-26 23:08:11 PDT
The init_seg trick doesn't work for VS 2017 as expected.
How should we do?

1) Look for the solution or ask MS how to solve this
2) Stop using the init_seg trick for both VS 2015 and 2017
3) Use the init_seg trick only for VS 2015

Anyone have an idea?
Comment 5 Fujii Hironori 2017-07-27 18:15:33 PDT
Oh, no. Just removing pragma init_seg causes a crash when destructors are called at shutdown.
Comment 6 Fujii Hironori 2017-07-30 16:03:50 PDT
Created attachment 316752 [details]
WIP patch (Moved #pragma init_seg)

In the compilation units of generated *Names.cpp files, some global variable need to be constructed, but other doesn't.
Put the global variable needing to be constructed in the front of one doesn't, and put #pragma init_seg just after the former.
Comment 7 Yusuke Suzuki 2017-07-30 19:49:50 PDT
For a long term solution, I think we should use LazyNeverDestroyed instead of DEFINE_GLOBAL hack.
Comment 8 Fujii Hironori 2017-07-31 02:26:35 PDT
Sounds very interesting. I'll try it in Bug 174979. Thank you.
Comment 9 Fujii Hironori 2017-08-15 23:15:00 PDT
The pragma init_seg issue will be solved in Bug 175209. This bug doesn't block Bug 174195 for now.
Comment 10 Stephan Szabo 2017-11-30 11:26:40 PST
Created attachment 327998 [details]
Require VS2017 for building

This merges together pieces of the earlier patch and https://bug-174195-attachments.webkit.org/attachment.cgi?id=314698 and updates the webkitdirs pieces to deal with the vswhere changes. 

I ended up not giving a specific location relating to the location of VS2017 for vcvars.bat in some messages as both previous patches seemed to update the text to a location I didn't actually get with a VS2017 install and from my install it seemed like the location would be dependent on the particular VS2017 flavor (enterprise, professional, etc) you installed.

Uploading first to throw at bots.
Comment 11 Build Bot 2017-11-30 11:28:46 PST
Attachment 327998 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Stephan Szabo 2017-11-30 13:12:08 PST
Created attachment 328010 [details]
Require VS2017 for building
Comment 13 Per Arne Vollan 2017-12-05 14:23:06 PST
(In reply to Stephan Szabo from comment #12)
> Created attachment 328010 [details]
> Require VS2017 for building

Does this patch need rebasing?
Comment 14 Stephan Szabo 2017-12-05 14:59:54 PST
Created attachment 328508 [details]
Require VS2017 for building

Rebased to re-check on bot
Comment 15 Per Arne Vollan 2017-12-05 15:25:19 PST
Comment on attachment 328508 [details]
Require VS2017 for building

Looks good! R=me. Please wait until bots are green before committing.
Comment 16 Stephan Szabo 2017-12-05 16:06:12 PST
Created attachment 328518 [details]
Require VS2017 for building - reup
Comment 17 Stephan Szabo 2017-12-05 17:47:13 PST
Created attachment 328539 [details]
Require VS2017 for building - setting reviewer based on pre-reupload r=me
Comment 18 WebKit Commit Bot 2017-12-05 18:19:01 PST
Comment on attachment 328539 [details]
Require VS2017 for building - setting reviewer based on pre-reupload r=me

Clearing flags on attachment: 328539

Committed r225563: <https://trac.webkit.org/changeset/225563>
Comment 19 WebKit Commit Bot 2017-12-05 18:19:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2017-12-06 16:47:50 PST
<rdar://problem/35896610>
Comment 21 Radar WebKit Bug Importer 2017-12-06 16:47:57 PST
<rdar://problem/35896611>
Comment 22 Don Olmstead 2017-12-06 20:08:37 PST
*** Bug 174195 has been marked as a duplicate of this bug. ***