[webkit-reviews] review granted: [Bug 179534] webkitpy: Trying to use iOS versions from machines without iOS SDKs doesn't make sense : [Attachment 327787] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 179534: webkitpy: Trying to use iOS versions from machines without iOS SDKs
doesn't make sense
https://bugs.webkit.org/show_bug.cgi?id=179534

Attachment 327787: Patch

https://bugs.webkit.org/attachment.cgi?id=327787&action=review




--- Comment #12 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 327787
  --> https://bugs.webkit.org/attachment.cgi?id=327787
Patch

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

>>> Tools/Scripts/webkitpy/port/builders.py:92
>>> +	 r"ios-simulator-11-wk2",
>> 
>> It seems like this will require updating every year. Is there really no way
to handle this generically?
> 
> This will require updating every year, which is why I'd like to get rid of
this entire file (the whole thing is out of date).
> 
> But that's separate from this fix.

I'm fine with this as-is if the plan is to make it go away.

>>> Tools/Scripts/webkitpy/port/ios_simulator.py:118
>>> +	     return Version.from_string(self._name.split('-')[2]) if
len(self._name.split('-')) > 2 and self._name.split('-')[2].isdigit() else
self.host.platform.xcode_sdk_version('iphonesimulator')
>> 
>> This is a very complicated line. I think all of this would be much clearer
if we had a way of checking if the iphonesimulator is installed, and what its
version is. I don't really like encoding the value to use if no simulator
exists into the name of the simulator port.
> 
> Generally, we do use the installed simulator.
> 
> However, this specific case involves instantiating an iOS Simulator port
which either does not match the current SDK version, or, the machine doesn't
have an SDK version at all (we do this for linting test expectations, which
requires a port object).
> 
> This is very similar to what we do for Mac, where we can either instantiate a
port with an encoded version (like mac-sierra-wk2) or adopt the version of the
machine currently running tests (webkitpy/port/mac.py, line 55 does almost this
exact same thing)

I'm okay with this approach, but I don't like this complicated line. Can you
break it into a function that describes what it is doing?

if (nameHasVersion(self._name))
    return Version.from_string(self._name.split('-')[2]
return self.host.platform.xcode_sdk_version('iphonesimulator')

... where 'nameHasVersion' (or whatever you call it) does the split and check
for is digit.