Bug 187127 - [JSCOnly] Restore Windows build.
Summary: [JSCOnly] Restore Windows build.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-27 18:09 PDT by Ross Kirsling
Modified: 2018-06-29 10:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.75 KB, patch)
2018-06-28 11:40 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2018-06-28 11:45 PDT, Ross Kirsling
mcatanzaro: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.05 MB, application/zip)
2018-06-28 13:06 PDT, EWS Watchlist
no flags Details
Patch (3.99 KB, patch)
2018-06-28 20:34 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (4.07 KB, patch)
2018-06-28 22:12 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-06-27 18:09:12 PDT
[JSCOnly] Restore Windows build.
Comment 1 Ross Kirsling 2018-06-28 11:40:12 PDT
Created attachment 343821 [details]
Patch
Comment 2 Ross Kirsling 2018-06-28 11:45:30 PDT
Created attachment 343822 [details]
Patch
Comment 3 Don Olmstead 2018-06-28 11:48:57 PDT
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
Comment 4 Ross Kirsling 2018-06-28 11:51:29 PDT
(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).
Comment 5 Don Olmstead 2018-06-28 12:45:07 PDT
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.
Comment 6 EWS Watchlist 2018-06-28 13:06:14 PDT
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
Comment 7 EWS Watchlist 2018-06-28 13:06:15 PDT
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
Comment 8 Ross Kirsling 2018-06-28 15:31:58 PDT
LayoutTest failures are unrelated:
https://bugs.webkit.org/show_bug.cgi?id=187156
Comment 9 Michael Catanzaro 2018-06-28 19:05:34 PDT
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()
Comment 10 Ross Kirsling 2018-06-28 20:25:27 PDT
(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.
Comment 11 Ross Kirsling 2018-06-28 20:34:03 PDT
Created attachment 343890 [details]
Patch
Comment 12 Ross Kirsling 2018-06-28 22:12:44 PDT
Created attachment 343892 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2018-06-29 10:55:31 PDT
Comment on attachment 343892 [details]
Patch for landing

Clearing flags on attachment: 343892

Committed r233363: <https://trac.webkit.org/changeset/233363>
Comment 14 WebKit Commit Bot 2018-06-29 10:55:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-06-29 10:56:22 PDT
<rdar://problem/41642809>