Bug 177279

Summary: [Win][JSCOnly] Make jsconly build testapi and dlls and copy dlls when running tests
Product: WebKit Reporter: Stephan Szabo <stephan.szabo>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, don.olmstead, lforschler, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Build jsc/testapi/testmasm on windows jsc-only similar to win platform
ysuzuki: review+
Build jsc/testapi/testmasm on windows jsc-only similar to win platform - added fixme none

Description Stephan Szabo 2017-09-20 15:58:56 PDT
Non-jsc-only Windows builds of the JSC shell build a jscLib.dll and testapi* differently from other builds.
Do the same for jsc-only windows builds and copy the dll along with the jsc executable when copying in run-jsc-stress-tests on Windows.
Comment 1 Stephan Szabo 2017-09-20 16:13:28 PDT
Also, there are some definitions we set up in OptionsWin that we probably should also be setting in OptionsJSCOnly when we're on Windows.
Comment 2 Stephan Szabo 2017-09-21 15:55:32 PDT
Created attachment 321487 [details]
Build jsc/testapi/testmasm on windows jsc-only similar to win platform
Comment 3 Stephan Szabo 2017-09-22 10:19:36 PDT
Comment on attachment 321487 [details]
Build jsc/testapi/testmasm on windows jsc-only similar to win platform

So, it seems like we do need more of the Windows configuration than initially thought in order for the whole test side to be happier. I'm not sure if it's time to move some of those options elsewhere or if this is still okay.

Added Yusuke Suzuki to cc list because he made a comment on the subject in https://bugs.webkit.org/show_bug.cgi?id=172833 and seemed like it would be good to get opinion.
Comment 4 Yusuke Suzuki 2017-10-11 10:27:18 PDT
Comment on attachment 321487 [details]
Build jsc/testapi/testmasm on windows jsc-only similar to win platform

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

Making JSC tests work in Windows (in JSCOnly port) is great. I have several questions.

> Source/JavaScriptCore/shell/PlatformJSCOnly.cmake:3
> +    add_definitions(-DWIN_CAIRO)

Is this necessary? JSCOnly port is not relying on Cairo.

> Source/JavaScriptCore/shell/PlatformJSCOnly.cmake:4
> +endif ()

Why can't we do the same to the other non-Windows port (and removing PlatformWin.cmake)?
Is there any specific issues for Windows port?
Comment 5 Stephan Szabo 2017-10-11 10:52:54 PDT
> > Source/JavaScriptCore/shell/PlatformJSCOnly.cmake:3
> > +    add_definitions(-DWIN_CAIRO)
> Is this necessary? JSCOnly port is not relying on Cairo.

Right now shell/DLLLauncherMain.cpp checks for WIN_CAIRO in places to separate it from applewin, for example 
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/shell/DLLLauncherMain.cpp#L125
where the library path added differs. And we're currently really only handling the wincairo paths in jsc-only, but that might change.

That file's related to the tiny launcher app mentioned in the below.

> Why can't we do the same to the other non-Windows port (and removing  PlatformWin.cmake)?
> Is there any specific issues for Windows port?

I'm not sure of the history, but the shell, testapi, etc on Windows (non-jsc-only) build a separate <something>Lib.dll and then a small launcher to run the code from the dll. The description from when the launcher seemed to be added seemed like it was related to getting the apple support libraries for the testapi and similar apps.
Comment 6 Stephan Szabo 2017-10-11 11:00:02 PDT
And I guess even for wincairo-ish builds it prevents you from needing to add the support library directory to your PATH if you have WEBKIT_LIBRARIES defined properly.
Comment 7 Yusuke Suzuki 2017-10-24 10:30:24 PDT
(In reply to Stephan Szabo from comment #5)
> Right now shell/DLLLauncherMain.cpp checks for WIN_CAIRO in places to
> separate it from applewin, for example 
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/shell/
> DLLLauncherMain.cpp#L125
> where the library path added differs. And we're currently really only
> handling the wincairo paths in jsc-only, but that might change.
> 
> That file's related to the tiny launcher app mentioned in the below.
>
> I'm not sure of the history, but the shell, testapi, etc on Windows
> (non-jsc-only) build a separate <something>Lib.dll and then a small launcher
> to run the code from the dll. The description from when the launcher seemed
> to be added seemed like it was related to getting the apple support
> libraries for the testapi and similar apps.

OK, if there is no reason for this, I believe Windows difference should be removed. But it's ok for now.
Could you add FIXME to clean up these things if there is no reason to make Win build results separated from others?
Adding the FIXME comment to PlatformJSCOnly.cmake is preferable.

# FIXME: https://bugs.webkit.org/url-to-newly-filed-bug
# description...
Comment 8 Stephan Szabo 2017-10-24 11:00:17 PDT
Created attachment 324684 [details]
Build jsc/testapi/testmasm on windows jsc-only similar to win platform - added fixme
Comment 9 WebKit Commit Bot 2017-10-24 11:46:35 PDT
Comment on attachment 324684 [details]
Build jsc/testapi/testmasm on windows jsc-only similar to win platform - added fixme

Clearing flags on attachment: 324684

Committed r223904: <https://trac.webkit.org/changeset/223904>
Comment 10 Radar WebKit Bug Importer 2017-11-15 13:06:28 PST
<rdar://problem/35568802>