Summary: | [Tools] --wincairo should imply --64-bit by default. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||
Component: | Tools / Tests | Assignee: | Ross Kirsling <ross.kirsling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, dbates, don.olmstead, ews-watchlist, glenn, lforschler, pvollan, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ross Kirsling
2018-02-13 16:32:28 PST
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. |