Bug 160709

Summary: Move functionality common to Darwin ports into a base class
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, dbates, glenn, lforschler, ossy, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jonathan Bedard 2016-08-09 14:25:05 PDT
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.
Comment 1 Jonathan Bedard 2016-08-09 14:35:38 PDT
Created attachment 285675 [details]
Patch
Comment 2 Csaba Osztrogon√°c 2016-08-10 00:29:17 PDT
After this patch only the win port inherits from ApplePort, doesn't it? If yes,you could move the remaining ApplePort to win.py.
Comment 3 Jonathan Bedard 2016-08-10 08:15:30 PDT
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?
Comment 4 Daniel Bates 2016-08-18 12:09:16 PDT
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.
Comment 5 Jonathan Bedard 2016-08-18 15:31:10 PDT
(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.
Comment 6 Jonathan Bedard 2016-08-18 15:44:59 PDT
Created attachment 286405 [details]
Patch
Comment 7 WebKit Commit Bot 2016-08-18 15:47:05 PDT
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.
Comment 8 Jonathan Bedard 2016-08-18 15:51:24 PDT
Created attachment 286407 [details]
Patch
Comment 9 Daniel Bates 2016-08-18 18:09:42 PDT
(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()?
Comment 10 Daniel Bates 2016-08-18 18:10:37 PDT
(In reply to comment #9)
> Can we share more of the common code from MacPort.xcrun_find() and
> IOSPort.xcrun_find()?

*IOSSimulatorPort
Comment 11 Jonathan Bedard 2016-08-23 08:54:14 PDT
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.
Comment 12 Jonathan Bedard 2016-08-23 15:07:48 PDT
Created attachment 286786 [details]
Patch
Comment 13 Jonathan Bedard 2016-09-19 10:08:11 PDT
Created attachment 289235 [details]
Patch
Comment 14 WebKit Commit Bot 2016-10-07 14:09:15 PDT
Comment on attachment 289235 [details]
Patch

Clearing flags on attachment: 289235

Committed r206934: <http://trac.webkit.org/changeset/206934>
Comment 15 WebKit Commit Bot 2016-10-07 14:09:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Jonathan Bedard 2016-10-07 15:19:36 PDT
Committed r206940: <http://trac.webkit.org/changeset/206940>