Bug 179534 – webkitpy: Trying to use iOS versions from machines without iOS SDKs doesn't make sense

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

[Jonathan Bedard]

Some of the code in IOSSimulatorPort gets run on machines without iOS SDKs, and attempts to process iOS versions from Xcode.  This is wrong.

[Radar WebKit Bug Importer]

<rdar://problem/35469509>

[Jonathan Bedard]

Turn out that after <https://trac.webkit.org/changeset/224658>, this bug can break style checking on Mac's missing an iOS SDK.

[Jonathan Bedard]

Created attachment 326770 [details]
Patch

[Jonathan Bedard]

A follow-up patch is required so that the style-checker, in particular, can iterate through each available OS type for each platform.  However, in-order to have this, we need a better way of name-version mapping, discussed here <https://bugs.webkit.org/show_bug.cgi?id=179621>.

[Jonathan Bedard]

Created attachment 327787 [details]
Patch

[Jonathan Bedard]

Attachment 326770 [details] definitely fixes this problem, but it has revealed a new one.

Currently, we pull from builder.py the 'current' builders on WebKit.org.  The trouble is, these are very out of date.

I think we should actually pull from our name mapping, constructing the names of most iOS and Mac testers.  In any case, that is definitely a separate bug, filed here <https://bugs.webkit.org/show_bug.cgi?id=180112>.

[Jonathan Bedard]

*** Bug 180132 has been marked as a duplicate of this bug. ***

[Jonathan Bedard]

*** Bug 180138 has been marked as a duplicate of this bug. ***

[Jonathan Bedard]

Looks like this also fixes another related issue with ios-simulator ports being instantiated on non-mac ports while style checking.

That is the problem bug 180132 and but 180138 were filed about.

I duped those bugs to this one because they all have the same underlying cause.

[Brent Fulgham]

Comment on attachment 327787 [details]
Patch

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

I'd like to see this fixed (I ran into it yesterday), but I don't like this approach. Surely we can do something to identify whether the simulator is installed, and do the right thing in that case?

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

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

[Jonathan Bedard]

Comment on attachment 327787 [details]
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.

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

[Brent Fulgham]

Comment on attachment 327787 [details]
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.

[Jonathan Bedard]

Created attachment 327865 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327865 [details]
Patch for landing

Clearing flags on attachment: 327865

Committed r225274: <https://trac.webkit.org/changeset/225274>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.