Bug 179534

<rdar://problem/35469509>

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

Created attachment 326770 [details] Patch

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

Created attachment 327787 [details] Patch

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

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

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

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.

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.

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)

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.

Created attachment 327865 [details] Patch for landing

Comment on attachment 327865 [details] Patch for landing Clearing flags on attachment: 327865 Committed r225274: <https://trac.webkit.org/changeset/225274>

All reviewed patches have been landed. Closing bug.