Bug 204477 - Set 64-bit as default architecture on Windows
Summary: Set 64-bit as default architecture on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-21 15:57 PST by Per Arne Vollan
Modified: 2019-12-02 09:39 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.06 KB, patch)
2019-11-21 15:59 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.74 KB, patch)
2019-11-21 17:11 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.74 KB, patch)
2019-11-22 11:11 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.74 KB, patch)
2019-11-22 14:23 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.71 KB, patch)
2019-11-22 16:10 PST, Per Arne Vollan
ross.kirsling: review+
Details | Formatted Diff | Diff
Patch (2.13 KB, patch)
2019-11-22 17:50 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-11-21 15:57:40 PST
This will make 64-bit building and testing the default.
Comment 1 Per Arne Vollan 2019-11-21 15:59:22 PST
Created attachment 384100 [details]
Patch
Comment 2 Brent Fulgham 2019-11-21 16:06:35 PST
Comment on attachment 384100 [details]
Patch

Yes, please!
Comment 3 Per Arne Vollan 2019-11-21 17:11:06 PST
Created attachment 384107 [details]
Patch
Comment 4 Per Arne Vollan 2019-11-22 11:11:09 PST
Created attachment 384175 [details]
Patch
Comment 5 Jonathan Bedard 2019-11-22 13:10:16 PST
While the code looks fine, I'm a not super excited about landing a Windows change that's failing the Windows build in EWS.
Comment 6 Per Arne Vollan 2019-11-22 14:23:07 PST
Created attachment 384197 [details]
Patch
Comment 7 Per Arne Vollan 2019-11-22 14:48:09 PST
(In reply to Jonathan Bedard from comment #5)
> While the code looks fine, I'm a not super excited about landing a Windows
> change that's failing the Windows build in EWS.

The build failures are addressed in https://bugs.webkit.org/show_bug.cgi?id=204534.
Comment 8 Ross Kirsling 2019-11-22 15:12:17 PST
Comment on attachment 384197 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:1295
> -    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isFTW() || isWinCairo() || isJSCOnly()) && !shouldBuild32Bit());
> +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isFTW() || isWinCairo() || isAppleWinWebKit() || isJSCOnly()) && !shouldBuild32Bit());

Should we do `isAnyWindows()` instead of `isFTW() || isWinCairo() || isAppleWinWebKit()`?

(I guess we have the same trio in determineConfigurationProductDir, but it seems needless to give the complete list, right?)
Comment 9 Ross Kirsling 2019-11-22 15:19:58 PST
Also we can remove this line, which seems to have been added unnecessarily:
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitdirs.pm#L498
Comment 10 Per Arne Vollan 2019-11-22 15:28:51 PST
(In reply to Ross Kirsling from comment #8)
> Comment on attachment 384197 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384197&action=review
> 
> > Tools/Scripts/webkitdirs.pm:1295
> > -    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isFTW() || isWinCairo() || isJSCOnly()) && !shouldBuild32Bit());
> > +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || ((isFTW() || isWinCairo() || isAppleWinWebKit() || isJSCOnly()) && !shouldBuild32Bit());
> 
> Should we do `isAnyWindows()` instead of `isFTW() || isWinCairo() ||
> isAppleWinWebKit()`?
> 

That makes sense, I will update the patch.

> (I guess we have the same trio in determineConfigurationProductDir, but it
> seems needless to give the complete list, right?)

Thanks for reviewing!
Comment 11 Per Arne Vollan 2019-11-22 16:10:41 PST
Created attachment 384212 [details]
Patch
Comment 12 Ross Kirsling 2019-11-22 17:43:50 PST
Comment on attachment 384212 [details]
Patch

r=me, but can we also remove the redundant `push(@args, '--64-bit') if (isFTW());` line I mentioned in comment 9?
Comment 13 Per Arne Vollan 2019-11-22 17:50:51 PST
Created attachment 384217 [details]
Patch
Comment 14 Per Arne Vollan 2019-11-22 17:51:23 PST
(In reply to Ross Kirsling from comment #12)
> Comment on attachment 384212 [details]
> Patch
> 
> r=me, but can we also remove the redundant `push(@args, '--64-bit') if
> (isFTW());` line I mentioned in comment 9?

Done.

Thanks for reviewing!
Comment 15 Per Arne Vollan 2019-11-22 17:52:10 PST
Will land this once the EWS bots are green.
Comment 16 WebKit Commit Bot 2019-11-22 21:37:54 PST
Comment on attachment 384217 [details]
Patch

Clearing flags on attachment: 384217

Committed r252827: <https://trac.webkit.org/changeset/252827>
Comment 17 Radar WebKit Bug Importer 2019-12-02 09:39:23 PST
<rdar://problem/57560495>