Instead of separate variable $isGtk, $isEfl, and $isWinCairo this patch adds one $portName variable, and all code for port type determination is gathered in one function.
Created attachment 270020 [details] Patch
Created attachment 270031 [details] Patch
Comment on attachment 270031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270031&action=review > Tools/Scripts/webkitdirs.pm:103 > + IOS => "IOS", This should probably be iOS.
Comment on attachment 270031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270031&action=review > Tools/ChangeLog:22 > + * Scripts/webkitdirs.pm: > + (determinePortName): > + (portName): > + (isEfl): > + (isGtk): > + (isWinCairo): > + (isAppleMacWebKit): > + (isAppleWinWebKit): > + (isIOSWebKit): > + (cmakeBasedPortName): > + (checkForArgumentAndRemoveFromArrayRef): Deleted. > + (isX86_64): Deleted. > + (isCrossCompilation): Deleted. > + (createiOSSimulatorDevice): Deleted. > + (buildCMakeProjectOrExit): Deleted. In a good quality change log, we make comments saying what we did to each function. >> Tools/Scripts/webkitdirs.pm:103 >> + IOS => "IOS", > > This should probably be iOS. Yes, it should. > Tools/Scripts/webkitdirs.pm:1060 > + $portName = "AppleWin"; Don’t we want to use the constant here? > Tools/Scripts/webkitdirs.pm:1064 > + $portName = "IOS"; Don’t we want to use the constant here? > Tools/Scripts/webkitdirs.pm:1066 > + $portName = "Mac"; Don’t we want to use the constant here?
Comment on attachment 270031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270031&action=review >> Tools/ChangeLog:22 >> + (buildCMakeProjectOrExit): Deleted. > > In a good quality change log, we make comments saying what we did to each function. Thanks, will fix it. >>> Tools/Scripts/webkitdirs.pm:103 >>> + IOS => "IOS", >> >> This should probably be iOS. > > Yes, it should. I know what is the right capitalization of "iOS", however I capitilized "I" here because other port names start with capital and these names are often used in the middle of word, e.g., isIOSWebKit(), and I guess there will be PlatformIOS.cmake when/if iOS port migrates to cmake. However we can just ucfirst portName() where capitalization is needed. >> Tools/Scripts/webkitdirs.pm:1060 >> + $portName = "AppleWin"; > > Don’t we want to use the constant here? Yes, we do.
Comment on attachment 270031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270031&action=review >>> Tools/ChangeLog:22 >>> + (buildCMakeProjectOrExit): Deleted. >> >> In a good quality change log, we make comments saying what we did to each function. > > Thanks, will fix it. BTW, prepare-ChangeLog miserably failed to detect deleted functions here.
Created attachment 270105 [details] Patch
Comment on attachment 270105 [details] Patch Clearing flags on attachment: 270105 Committed r195742: <http://trac.webkit.org/changeset/195742>
All reviewed patches have been landed. Closing bug.
Reverted r195742 for reason: Broke EWS Committed r195746: <http://trac.webkit.org/changeset/195746>
(In reply to comment #10) > Reverted r195742 for reason: > > Broke EWS > > Committed r195746: <http://trac.webkit.org/changeset/195746> Failed to run "['/home/slave/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=ltilve-gtk-ews', 'update', '--port=gtk-wk2']" exit_code: 2 cwd: /home/slave/WebKit Updating working directory Failed to run "['Tools/Scripts/update-webkit']" exit_code: 255 Undefined subroutine &main::determineIsWinCairo called at Tools/Scripts/update-webkit line 46. Failed to run "['Tools/Scripts/update-webkit']" exit_code: 255 Undefined subroutine &main::determineIsWinCairo called at Tools/Scripts/update-webkit line 46. c.f. https://webkit-queues.webkit.org/results/750792
Created attachment 270124 [details] Patch
Our EWS still has not recovered despite the roll out because this patch broke the update-webkit script and thus the bots are unable to update past the rollout :( I think someone is going to have to 'svn up' manually on all these bots :(
Created attachment 270134 [details] Patch
Created attachment 270136 [details] Patch
Created attachment 270213 [details] Patch
New version of patch confesses that we have a state when we don't know which port is chosen but it is not fatal. This situation currently happens on Linux in update-webkit and several other scripts which don't officially accept arguments --gtk or --efl, so both isEfl() and isGtk() are false (and this patch does not change this behavior, it just makes it less subtle). However, build-jsc and build-webkit bail out if port is unknown. I thought about making GTK default port for OS which is not Windows and not Darwin, but I believe that would only make situation worse if people start relying on such behavior, becuase in this case it would be quite possible to have e.g. EFL port run update-webkit without arguments and it would internally assume that it is updating GTK.
I've tested update-webkit and it should not break EWS this time.
Comment on attachment 270213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270213&action=review > Tools/Scripts/build-jsc:86 > + die "Please choose which WebKit port to build"; It would be good to list available ports and show how to select the port. For example: "Please specify which WebKit port to build with --efl or --gtk." This only matters for Linux, since other platforms have default ports. This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk).
Comment on attachment 270213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270213&action=review >> Tools/Scripts/build-jsc:86 >> + die "Please choose which WebKit port to build"; > > It would be good to list available ports and show how to select the port. For example: > > "Please specify which WebKit port to build with --efl or --gtk." > > This only matters for Linux, since other platforms have default ports. > > This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk). It’s slightly more idiomatic perl to write: die "Please specify which WebKit port to build with --efl or --gtk." if $isUnknownPort(); > Tools/Scripts/build-webkit:142 > +if (isUnknownPort()) { > + die "Please choose which WebKit port to build"; > +} Ditto. Quite annoying that this needs to be repeated in two separate scripts. Please find a way to avoid that.
Created attachment 270424 [details] Patch
Comment on attachment 270424 [details] Patch Attachment 270424 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/769419 New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection-1.html
Created attachment 270430 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 270498 [details] Patch
Comment on attachment 270213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270213&action=review >>> Tools/Scripts/build-jsc:86 >>> + die "Please choose which WebKit port to build"; >> >> It would be good to list available ports and show how to select the port. For example: >> >> "Please specify which WebKit port to build with --efl or --gtk." >> >> This only matters for Linux, since other platforms have default ports. >> >> This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk). > > It’s slightly more idiomatic perl to write: > > die "Please specify which WebKit port to build with --efl or --gtk." if $isUnknownPort(); Done, run-minibrowser now dies in the same manner. >> Tools/Scripts/build-webkit:142 >> +} > > Ditto. > > Quite annoying that this needs to be repeated in two separate scripts. Please find a way to avoid that. Done.
I think that prohibitUnknownPort would be much more obvious if pased as import parameter of webkitdirs, i.e. use webkitdirs prohibitUnknownPort => 1; but webkitdirs already uses Exporter. I'm pretty sure that Exporter is not really needed for this module (in fact, it is used with import list only in one place right now), but such refactoring would be clearly too much for this patch.
Comment on attachment 270498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270498&action=review > Tools/Scripts/webkitdirs.pm:101 > + Efl => "Efl", Shouldn't it be EFL in all caps? > Tools/Scripts/webkitdirs.pm:2113 > + return ucfirst portName(); To make sure we have PlatformIOS and OptionsIOS rather than PlatformiOS and OptionsiOS? Good thinking.
Comment on attachment 270498 [details] Patch Rejecting attachment 270498 [details] from commit-queue. annulen@yandex.ru does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 270498 [details] Patch Clearing flags on attachment: 270498 Committed r196065: <http://trac.webkit.org/changeset/196065>