RESOLVED FIXED 153554
[webkitdirs] Clarify logic behind is{PortName} functions.
https://bugs.webkit.org/show_bug.cgi?id=153554
Summary [webkitdirs] Clarify logic behind is{PortName} functions.
Konstantin Tokarev
Reported 2016-01-27 11:52:15 PST
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.
Attachments
Patch (4.70 KB, patch)
2016-01-27 12:00 PST, Konstantin Tokarev
no flags
Patch (4.72 KB, patch)
2016-01-27 13:10 PST, Konstantin Tokarev
no flags
Patch (4.87 KB, patch)
2016-01-28 03:48 PST, Konstantin Tokarev
no flags
Patch (5.30 KB, patch)
2016-01-28 09:58 PST, Konstantin Tokarev
no flags
Patch (5.50 KB, patch)
2016-01-28 11:37 PST, Konstantin Tokarev
no flags
Patch (6.39 KB, patch)
2016-01-28 11:51 PST, Konstantin Tokarev
no flags
Patch (6.56 KB, patch)
2016-01-29 08:33 PST, Konstantin Tokarev
no flags
Patch (7.28 KB, patch)
2016-02-01 14:02 PST, Konstantin Tokarev
no flags
Archive of layout-test-results from ews114 for mac-yosemite (791.50 KB, application/zip)
2016-02-01 14:57 PST, Build Bot
no flags
Patch (7.71 KB, patch)
2016-02-02 10:34 PST, Konstantin Tokarev
no flags
Konstantin Tokarev
Comment 1 2016-01-27 12:00:14 PST
Konstantin Tokarev
Comment 2 2016-01-27 13:10:39 PST
Alex Christensen
Comment 3 2016-01-27 14:01:41 PST
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.
Darin Adler
Comment 4 2016-01-27 15:28:27 PST
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?
Konstantin Tokarev
Comment 5 2016-01-28 03:14:10 PST
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.
Konstantin Tokarev
Comment 6 2016-01-28 03:36:59 PST
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.
Konstantin Tokarev
Comment 7 2016-01-28 03:48:05 PST
WebKit Commit Bot
Comment 8 2016-01-28 09:18:46 PST
Comment on attachment 270105 [details] Patch Clearing flags on attachment: 270105 Committed r195742: <http://trac.webkit.org/changeset/195742>
WebKit Commit Bot
Comment 9 2016-01-28 09:18:50 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 10 2016-01-28 09:44:12 PST
Reverted r195742 for reason: Broke EWS Committed r195746: <http://trac.webkit.org/changeset/195746>
Chris Dumez
Comment 11 2016-01-28 09:44:55 PST
(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
Konstantin Tokarev
Comment 12 2016-01-28 09:58:54 PST
Chris Dumez
Comment 13 2016-01-28 10:07:23 PST
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 :(
Konstantin Tokarev
Comment 14 2016-01-28 11:37:52 PST
Konstantin Tokarev
Comment 15 2016-01-28 11:51:55 PST
Konstantin Tokarev
Comment 16 2016-01-29 08:33:09 PST
Konstantin Tokarev
Comment 17 2016-01-29 08:44:05 PST
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.
Konstantin Tokarev
Comment 18 2016-01-29 08:48:31 PST
I've tested update-webkit and it should not break EWS this time.
Michael Catanzaro
Comment 19 2016-01-29 15:31:01 PST
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).
Darin Adler
Comment 20 2016-01-30 12:00:27 PST
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.
Konstantin Tokarev
Comment 21 2016-02-01 14:02:54 PST
Build Bot
Comment 22 2016-02-01 14:56:58 PST
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
Build Bot
Comment 23 2016-02-01 14:57:01 PST
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
Konstantin Tokarev
Comment 24 2016-02-02 10:34:10 PST
Konstantin Tokarev
Comment 25 2016-02-02 11:22:59 PST
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.
Konstantin Tokarev
Comment 26 2016-02-02 13:52:39 PST
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.
Michael Catanzaro
Comment 27 2016-02-02 15:16:46 PST
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.
WebKit Commit Bot
Comment 28 2016-02-03 02:46:40 PST
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.
WebKit Commit Bot
Comment 29 2016-02-03 03:38:18 PST
Comment on attachment 270498 [details] Patch Clearing flags on attachment: 270498 Committed r196065: <http://trac.webkit.org/changeset/196065>
WebKit Commit Bot
Comment 30 2016-02-03 03:38:25 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.