Bug 187127 – [JSCOnly] Restore Windows build.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSCOnly] Restore Windows build.

[Ross Kirsling]

Created attachment 343821 [details]
Patch

[Ross Kirsling]

Created attachment 343822 [details]
Patch

[Don Olmstead]

Comment on attachment 343822 [details]
Patch

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

Overall LGTM

> Source/cmake/OptionsJSCOnly.cmake:74
> +        set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib64)

This is now listed twice

[Ross Kirsling]

(In reply to Don Olmstead from comment #3)
> Comment on attachment 343822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343822&action=review
> 
> Overall LGTM
> 
> > Source/cmake/OptionsJSCOnly.cmake:74
> > +        set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib64)
> 
> This is now listed twice

It's two different variables. Seems that both are needed (just as in OptionsWin).

[Don Olmstead]

Comment on attachment 343822 [details]
Patch

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

>>> Source/cmake/OptionsJSCOnly.cmake:74
>>> +        set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib64)
>> 
>> This is now listed twice
> 
> It's two different variables. Seems that both are needed (just as in OptionsWin).

Misread my fault there.

[EWS Watchlist]

Comment on attachment 343822 [details]
Patch

Attachment 343822 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8372472

New failing tests:
storage/indexeddb/modern/blob-svg-image.html

[EWS Watchlist]

Created attachment 343838 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Ross Kirsling]

LayoutTest failures are unrelated:
https://bugs.webkit.org/show_bug.cgi?id=187156

[Michael Catanzaro]

Comment on attachment 343822 [details]
Patch

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

> Source/cmake/OptionsJSCOnly.cmake:-21
> -    # FIXME: Enable FTL on Windows. https://bugs.webkit.org/show_bug.cgi?id=145366
> -    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PRIVATE OFF)

You don't expect anyone to build JSCOnly using CMake directly? I guess the build-jsc script suffices... but for GTK/WPE, build-webkit and build-jsc are just wrappers above the real (CMake) build system, which we expect to work independently of it, so I wouldn't consider it misleading to disable an unsupported option here.

> Source/cmake/OptionsJSCOnly.cmake:76
> -        set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
> +        set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin64)

Windows is interesting.

> Tools/Scripts/webkitdirs.pm:1299
>      return if defined($isWin64);
> -    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || isWinCairo() && !shouldBuild32Bit();
> +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isWinCairo() || isJSCOnly) && !shouldBuild32Bit());

This will probably break for Apple Windows since you're missing the parentheses after isJSCOnly()

[Ross Kirsling]

(In reply to Michael Catanzaro from comment #9)
> Comment on attachment 343822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343822&action=review
> 
> > Source/cmake/OptionsJSCOnly.cmake:-21
> > -    # FIXME: Enable FTL on Windows. https://bugs.webkit.org/show_bug.cgi?id=145366
> > -    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PRIVATE OFF)
> 
> You don't expect anyone to build JSCOnly using CMake directly? I guess the
> build-jsc script suffices... but for GTK/WPE, build-webkit and build-jsc are
> just wrappers above the real (CMake) build system, which we expect to work
> independently of it, so I wouldn't consider it misleading to disable an
> unsupported option here.

That's a fair counterpoint! It just surprised me as I was ruling out variables on what was causing the build to fail, but given that there is a scenario in which it has an effect, I'm okay with leaving it. 

> > Source/cmake/OptionsJSCOnly.cmake:76
> > -        set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
> > +        set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin64)
> 
> Windows is interesting.

Yeah... :( It would be fair game to have JSCOnly operate differently, but I figure if it's kept consistent with the full WebKit build, then a JSC test bot wouldn't need to worry about which type of build bot the archive is coming from.

> > Tools/Scripts/webkitdirs.pm:1299
> >      return if defined($isWin64);
> > -    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || isWinCairo() && !shouldBuild32Bit();
> > +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isWinCairo() || isJSCOnly) && !shouldBuild32Bit());
> 
> This will probably break for Apple Windows since you're missing the
> parentheses after isJSCOnly()

Ack! What a silly typo.

[Ross Kirsling]

Created attachment 343890 [details]
Patch

[Ross Kirsling]

Created attachment 343892 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 343892 [details]
Patch for landing

Clearing flags on attachment: 343892

Committed r233363: <https://trac.webkit.org/changeset/233363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41642809>