Bug 182753

Created attachment 333743 [details] Patch

Created attachment 333749 [details] Patch

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>

(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

(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.

Created attachment 333825 [details] Patch for landing

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: jiewen_tan@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 333825 [details] Patch for landing Clearing flags on attachment: 333825 Committed r228480: <https://trac.webkit.org/changeset/228480>

All reviewed patches have been landed. Closing bug.

<rdar://problem/37544518>