Bug 153554 – [webkitdirs] Clarify logic behind is{PortName} functions.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Konstantin Tokarev]

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.

[Konstantin Tokarev]

Created attachment 270020 [details]
Patch

[Konstantin Tokarev]

Created attachment 270031 [details]
Patch

[Alex Christensen]

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 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 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 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]

Created attachment 270105 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 270105 [details]
Patch

Clearing flags on attachment: 270105

Committed r195742: <http://trac.webkit.org/changeset/195742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r195742 for reason:

Broke EWS

Committed r195746: <http://trac.webkit.org/changeset/195746>

[Chris Dumez]

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

Created attachment 270124 [details]
Patch

[Chris Dumez]

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]

Created attachment 270134 [details]
Patch

[Konstantin Tokarev]

Created attachment 270136 [details]
Patch

[Konstantin Tokarev]

Created attachment 270213 [details]
Patch

[Konstantin Tokarev]

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]

I've tested update-webkit and it should not break EWS this time.

[Michael Catanzaro]

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 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]

Created attachment 270424 [details]
Patch

[Build Bot]

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]

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]

Created attachment 270498 [details]
Patch

[Konstantin Tokarev]

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]

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 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 on attachment 270498 [details]
Patch

Rejecting attachment 270498 [details] from commit-queue.

[e-mail] 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 on attachment 270498 [details]
Patch

Clearing flags on attachment: 270498

Committed r196065: <http://trac.webkit.org/changeset/196065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.