Bug 173107 – webkitpy: iOS Simulator uses Mac as its platform

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Jonathan Bedard]

Currently, iOS simulators will report that their platform is a Mac.  Notably, asking for the os_name of an iOS Simulator in webkitpy will return 'mac.'

[Radar WebKit Bug Importer]

<rdar://problem/32856125>

[Jonathan Bedard]

We should distinguish between a Mac and an iOS simulator platform.  Although, it should be noted that apart from version information, an iOS simulator platform should be identical to the Mac platform on a given machine.

[Jonathan Bedard]

Created attachment 313333 [details]
Patch

[Daniel Bates]

Comment on attachment 313333 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/platforminfo.py:64
>      def is_mac(self):
> -        return self.os_name == 'mac'
> +        return self.os_name == 'mac' or self.os_name == 'ios-simulator'

I'm confused. I was under the impression from both the description of this bug (comment #0) and the first sentence of comment #2 that we want to distinguish between the Mac and the iOS simulator platform. But PlatformInfo.is_mac() will return true for the iOS simulator platform.

How many callers would we need to update to use the Mac code path when the platform is iOS simulator  if we had this method only return true for the Mac platform?

> Tools/Scripts/webkitpy/port/ios_simulator_platform.py:59
> +    def __init__(self, executive, version=None):
> +        version_results = executive.run_command(['/usr/bin/xcodebuild', '-showsdks'], error_handler=lambda error: 0)
> +        simulator_sdks = []
> +        for line in version_results.splitlines():
> +            match = IOSSimulatorPlatform.simulator_sdk_re.match(line)
> +            if match:
> +                simulator_sdks.append(match.group('version'))
> +        if version == None and simulator_sdks:
> +            self._version = simulator_sdks[0]
> +        elif version:
> +            if version not in simulator_sdks:
> +                _log.warn('{} iOS Simulator SDK is not installed'.format(version))
> +            self._version = version
> +        else:
> +            raise RuntimeError('No iOS Simulator SDK is installed')

Is it necessary to do this? I mean, the one caller you have in this patch in SimulatedDevice.__init__() passes the runtime version that was parsed from the output of "xcrun simctl list" as version.

> Tools/Scripts/webkitpy/port/ios_simulator_platform.py:61
> +    @memoized

It is unnecessary to mark this function as @memoized as we are not computing anything. We should only use @memoized when the function performs an expensive computation.

> Tools/Scripts/webkitpy/port/ios_simulator_platform.py:65
> +    @memoized

It is unnecessary to mark this function as @memoized because the computation is not expensive.

[Jonathan Bedard]

Created attachment 313403 [details]
Patch

[Jonathan Bedard]

There are a few existing calls to platform.is_mac().  But I have audited them, and all are either a) Used right after a SystemHost() constructor or b) In layout test setup code, before any devices have been created.

[Daniel Bates]

Comment on attachment 313403 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/platforminfo_mock.py:37
> +        return self.os_name == 'mac' or self.os_name == 'ios-simulator'

Is this change still needed?

> Tools/Scripts/webkitpy/port/ios_simulator_platform.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

For the classes in this file I suggest that we mimic the classes in FakeSysModule and FakePlatformModule defined in <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/platforminfo_unittest.py> both in terms of use of the suffix "Module" in the name of the classes and the way they are implemented without a __init__(). The former removes the need to add comments in the code to explain how these classes will be used as substitutes for the Python standard modules. The latter makes the implementation of these classes similar to the implementation of FakeSysModule and FakePlatformModule.

[Jonathan Bedard]

Created attachment 313642 [details]
Patch

[Daniel Bates]

Comment on attachment 313642 [details]
Patch

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

From quickly grepping the code, I see various call sites that use is_mac() that likely need to also check is_ios_simulator() in crashlogs.py, http_server_base.py, http_server.py.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:104
> +        if self.is_mac() or self.is_ios():

Following this change this method now returns None when invoked for the iOS Simulator platform. Is this OK?

> Tools/Scripts/webkitpy/common/system/platforminfo.py:140
> +        if not (self.is_mac() or self.is_ios_simulator()):

Now that we can differentiate between Mac and iOS Simulator, we can limit this function to iOS Simulator:

if not  self.is_ios_simulator():
    return ()

> Tools/Scripts/webkitpy/common/system/platforminfo.py:146
> +        if not (self.is_mac() or self.is_ios_simulator()):

It is difficult to reason about negations. I suggest that we push the negation through this expression and write it as:

if not self.is_mac() and not self.is_ios_simulator():

> Tools/Scripts/webkitpy/common/system/platforminfo.py:147
>              raise NotImplementedError

I know you did not modify this code in this patch. I suggest we return an empty string instead of raising an exception as there is no way to implement this function on non Mac/iOS Simulator platforms.

[Jonathan Bedard]

Dan and I discussed this in person today (6/22).  The utility this patch gives us will be easier to achieve by adding a function to ask the Mac platform what the iOS SDK.  Once that functionality is added, I will close this bug.