Bug 182753 – [Tools] --wincairo should imply --64-bit by default.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ross Kirsling]

[Tools] --wincairo should imply --64-bit by default.

[Ross Kirsling]

Created attachment 333743 [details]
Patch

[Ross Kirsling]

Created attachment 333749 [details]
Patch

[Daniel Bates]

Comment on attachment 333749 [details]
Patch

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

> Tools/ChangeLog:3
> +        [Tools] --wincairo should imply --64-bit by default.

Shouldn’t the tools query the architecture by default? I mean, the WinCairo 64 Bit Release builder does not explicitly pass —64-bit (*) to build-webkit and it builds for 64-bit. I take it from the existence of this patch that the default build architecture of the bot was modified by running script webkit-configuration.

(*) <https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/10929/steps/compile-webkit/logs/stdio>

> Tools/Scripts/webkitdirs.pm:145
> +my $isExplicit32Bit;

Maybe a better name for this would be willBuildFor32Bit or willBuildFor32BitArchitecture and similarly rename the functions below.

> Tools/Scripts/webkitdirs.pm:1258
> +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || (isWinCairo() && !isExplicit32Bit());

The parentheses are not necessary by operator precedence [1].

[1] <https://perldoc.perl.org/perlop.html#Operator-Precedence-and-Associativity>

[Daniel Bates]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 333749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333749&action=review
> 
> > Tools/ChangeLog:3
> > +        [Tools] --wincairo should imply --64-bit by default.
> 
> Shouldn’t the tools query the architecture by default? I mean, the WinCairo
> 64 Bit Release builder does not explicitly pass —64-bit (*) to build-webkit
> and it builds for 64-bit. I take it from the existence of this patch that
> the default build architecture of the bot was modified by running script
> webkit-configuration.
> 

*set-webkit-configuration

[Ross Kirsling]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 333749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333749&action=review
> 
> > Tools/ChangeLog:3
> > +        [Tools] --wincairo should imply --64-bit by default.
> 
> Shouldn’t the tools query the architecture by default? I mean, the WinCairo
> 64 Bit Release builder does not explicitly pass —64-bit (*) to build-webkit
> and it builds for 64-bit. I take it from the existence of this patch that
> the default build architecture of the bot was modified by running script
> webkit-configuration.
> 
> (*)
> <https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/10929/
> steps/compile-webkit/logs/stdio>

Thanks for pointing this out! Experimenting locally, it seems that it's not about `set-webkit-configuration`, but rather that x64 is correctly identified when compiling with Ninja. It's when `--no-ninja` is specified that the script checks `isWin64()`:
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitdirs.pm#L2138-L2148

So fixing the definition of `isWin64()` is still worthwhile, but the reason is slightly more specific than I realized.

> > Tools/Scripts/webkitdirs.pm:1258
> > +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || (isWinCairo() && !isExplicit32Bit());
> 
> The parentheses are not necessary by operator precedence [1].
> 
> [1]
> <https://perldoc.perl.org/perlop.html#Operator-Precedence-and-Associativity>

Whoops, thought this was a style guide requirement.

[Ross Kirsling]

Created attachment 333825 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 333825 [details]:

imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/rsa_pkcs.worker.html bug 182803 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 333825 [details]
Patch for landing

Clearing flags on attachment: 333825

Committed r228480: <https://trac.webkit.org/changeset/228480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37544518>