Bug 160709 – Move functionality common to Darwin ports into a base class

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

This bug extends https://bugs.webkit.org/show_bug.cgi?id=160672, so that both the Mac ports and IOS ports inherit from a shared Darwin port.

[Jonathan Bedard]

Created attachment 285675 [details]
Patch

[Csaba Osztrogonác]

After this patch only the win port inherits from ApplePort, doesn't it? If yes,you could move the remaining ApplePort to win.py.

[Jonathan Bedard]

Darwin still shares functionality with ApplePort, so Darwin inherits from ApplePort.

I'm not exactly sure the original reason for separating the Windows port from the GTK and EFL, perhaps the Port class could take on some of the implementation from ApplePort?

[Daniel Bates]

Comment on attachment 285675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285675&action=review

> Tools/Scripts/webkitpy/port/darwin.py:59
> +    def supports_per_test_timeout(self):
> +        return True

Following the move of this method from class ApplePort to DarwinPort, the supports_per_test_timeout() returns False for the Windows port by <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.py?rev=204603#L155>. It looks like all ports support per test timeouts. So, we should change the base class implementation to remove True and remove all the port-specific overrides.

> Tools/Scripts/webkitpy/port/darwin.py:199
> +    def xcrun_find(self, command, fallback):
> +        _log.warn("xcrun undefined; falling back to '%s'." % fallback)
> +        return fallback

This does not seem correct. We can and should make use of xcrun to find command line tools for both the Mac and iOS ports.

[Jonathan Bedard]

(In reply to comment #4)
> Comment on attachment 285675 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285675&action=review
....
> 
> > Tools/Scripts/webkitpy/port/darwin.py:199
> > +    def xcrun_find(self, command, fallback):
> > +        _log.warn("xcrun undefined; falling back to '%s'." % fallback)
> > +        return fallback
> 
> This does not seem correct. We can and should make use of xcrun to find
> command line tools for both the Mac and iOS ports.

We do use xcrun for both Mac and iOS, but iOS assumes it's being run through a simulator, so it has a few extra arguments.  ios.py line 400 has the iOS simulator xcrun.

[Jonathan Bedard]

Created attachment 286405 [details]
Patch

[WebKit Commit Bot]

Attachment 286405 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/port/mac.py:37:  No name 'darwin' in module 'webkitpy.port'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/port/mac.py:192:  [MacPort.xcrun_find] Instance of 'MacPort' has no '_executive' member  [pylint/E1101] [5]
ERROR: Tools/Scripts/webkitpy/port/ios.py:36:  No name 'darwin' in module 'webkitpy.port'  [pylint/E0611] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 286407 [details]
Patch

[Daniel Bates]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 285675 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=285675&action=review
> ....
> > 
> > > Tools/Scripts/webkitpy/port/darwin.py:199
> > > +    def xcrun_find(self, command, fallback):
> > > +        _log.warn("xcrun undefined; falling back to '%s'." % fallback)
> > > +        return fallback
> > 
> > This does not seem correct. We can and should make use of xcrun to find
> > command line tools for both the Mac and iOS ports.
> 
> We do use xcrun for both Mac and iOS, but iOS assumes it's being run through
> a simulator, so it has a few extra arguments.  ios.py line 400 has the iOS
> simulator xcrun.

Can we share more of the common code from MacPort.xcrun_find() and IOSPort.xcrun_find()?

[Daniel Bates]

(In reply to comment #9)
> Can we share more of the common code from MacPort.xcrun_find() and
> IOSPort.xcrun_find()?

*IOSSimulatorPort

[Jonathan Bedard]

Comment on attachment 285675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285675&action=review

>>>> Tools/Scripts/webkitpy/port/darwin.py:199
>>>> +        return fallback
>>> 
>>> This does not seem correct. We can and should make use of xcrun to find command line tools for both the Mac and iOS ports.
>> 
>> We do use xcrun for both Mac and iOS, but iOS assumes it's being run through a simulator, so it has a few extra arguments.  ios.py line 400 has the iOS simulator xcrun.
> 
> Can we share more of the common code from MacPort.xcrun_find() and IOSPort.xcrun_find()?

So, just for reference (since the IOS xcrun isn't on this revision, here is that function definition:

# iOS xcrun_find(...) definition
def xcrun_find(self, command, fallback=None):
        fallback = fallback or command
        try:
            return self._executive.run_command(['xcrun', '--sdk', 'iphonesimulator', '-find', command]).rstrip()
        except ScriptError:
            _log.warn("xcrun failed; falling back to '%s'." % fallback)
            return fallback

I think that xcrun_find should remain separate in the two ports, particularly since we may be creating a port for an iOS device, which would use an xcrun_find similar to iOS, but targeting the device instead of the simulator.

[Jonathan Bedard]

Created attachment 286786 [details]
Patch

[Jonathan Bedard]

Created attachment 289235 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289235 [details]
Patch

Clearing flags on attachment: 289235

Committed r206934: <http://trac.webkit.org/changeset/206934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Committed r206940: <http://trac.webkit.org/changeset/206940>