Bug 180827 - [webkitpy] PlatformInfo should have default argument for casual use
Summary: [webkitpy] PlatformInfo should have default argument for casual use
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 81560 180829
  Show dependency treegraph
 
Reported: 2017-12-14 12:19 PST by Basuke Suzuki
Modified: 2018-06-28 15:13 PDT (History)
14 users (show)

See Also:


Attachments
patch (4.25 KB, patch)
2017-12-14 12:36 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
patch (7.97 KB, patch)
2017-12-15 09:57 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
patch (7.96 KB, patch)
2017-12-15 11:20 PST, Basuke Suzuki
ddkilzer: review+
Details | Formatted Diff | Diff
Rolling out (6.80 KB, patch)
2018-06-28 14:25 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Rolling out (6.80 KB, patch)
2018-06-28 14:29 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-12-14 12:19:27 PST
Add singleton class method to PlatformInfo to be used anywhere which need platform information with consistent manner.
Comment 1 Basuke Suzuki 2017-12-14 12:36:36 PST
Created attachment 329384 [details]
patch
Comment 2 Jonathan Bedard 2017-12-15 08:26:12 PST
Comment on attachment 329384 [details]
patch

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

Generally, I like this change, I think it makes PlatformInfo easier to use outside of the host object.

> Tools/Scripts/webkitpy/common/system/systemhost.py:41
> -        self.executive = executive.Executive()
>          self.filesystem = filesystem.FileSystem()
>          self.user = user.User()
> -        self.platform = platforminfo.PlatformInfo(sys, platform, self.executive)
> +        self.platform = platforminfo.PlatformInfo.singleton()
> +        self.executive = self.platform.executive

I think instead of using the singleton instance of the PlatformInfo object (and by extension, a singleton Executive object) each host should continue to own it's own instance of these objects. This may break your plans for <https://bugs.webkit.org/show_bug.cgi?id=180829>, though.  Was the idea behind that bug just to use the singleton PlatformInfo object in place of sys.platform?

Something you should also be aware of regarding the host object is that you can have meaningfully different host objects instantiated on the same machine. This is how on-device testing works. The proposed code here doesn't actually break that functionality, because the device testing host isn't actually SystemHost object, it's a Device object (which uses Python's duck typing to work like a SystemHost). In short, the singleton pattern is the wrong paradigm for the SystemHost object.

As a side note, I think we should probably be passing self.platform into the the User constructor.
Comment 3 Basuke Suzuki 2017-12-15 09:22:48 PST
Got it. So forget about this patch, all I want to achieve is to reconstruct the dependency tree among host, platform info and executive. Currently host depends on platform and executive, platform depends on executive, and executive is there on its foot. In the actual code, though, executive uses many platform dependent code in it.

Singleton is not the objective of me. I just want to make the PlatformInfo more casual. Another possibility is to add default handling of sys_module and platform_module. If they are None, use system's sys and platform module. Then we can easily create own instance of PlatformInfo in many places. What do you think about this idea?

> As a side note, I think we should probably be passing self.platform into the the User constructor.

Agreed.
Comment 4 Basuke Suzuki 2017-12-15 09:57:59 PST
Created attachment 329493 [details]
patch

Stop using singleton pattern. Add default arguments for PlatformInfo instead.
Comment 5 Basuke Suzuki 2017-12-15 10:09:31 PST
Comment on attachment 329493 [details]
patch

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

> Tools/Scripts/webkitpy/common/system/user.py:46
> -    if not sys.platform.startswith('win32'):
> +    if not PlatformInfo().is_native_win():

This is what this patch really achieved. Now PlatformInfo is handy to use.
Comment 6 Jonathan Bedard 2017-12-15 11:05:13 PST
Comment on attachment 329493 [details]
patch

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

I tested this patch with apple_additions(), everything works fine there.

Unofficial R+ from me.

> Tools/Scripts/webkitpy/common/system/systemhost.py:39
> -        self.executive = executive.Executive()
>          self.filesystem = filesystem.FileSystem()
> -        self.user = user.User()
> -        self.platform = platforminfo.PlatformInfo(sys, platform, self.executive)
> +        self.platform = platforminfo.PlatformInfo()
> +        self.executive = self.platform.executive
> +        self.user = user.User(self.platform)

A bit of nit-pick, I think this is more clear:

self.executive = executive.Executive()
self.filesystem = filesystem.FileSystem()
self.platform = platforminfo.PlatformInfo(executive=self.executive)
self.user = user.User(self.platform)

The code you have makes it seem like it is the PlatformInfo object which owns the executive.
Comment 7 Basuke Suzuki 2017-12-15 11:17:40 PST
Make sense.
Comment 8 Basuke Suzuki 2017-12-15 11:20:19 PST
Created attachment 329499 [details]
patch

Revert to original by passing its own executive to PlatformInfo
Comment 9 David Kilzer (:ddkilzer) 2018-01-09 11:09:31 PST
Comment on attachment 329499 [details]
patch

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

r=me

> Tools/ChangeLog:14
> +        Currently to instanciate PlatformInfo, it requires arguments, which is usually
> +        sys, platform and Executive(). They are straight forward and should be handled
> +        by default arguments. Then we can instanciate PlatformInfo more casual.

Typo × 2:  instanciate => instantiate
Comment 10 David Kilzer (:ddkilzer) 2018-01-09 11:15:14 PST
Committed r226652: <https://trac.webkit.org/changeset/226652>
Comment 11 Radar WebKit Bug Importer 2018-01-09 11:19:40 PST
<rdar://problem/36380742>
Comment 12 Daniel Bates 2018-03-16 14:59:30 PDT
Comment on attachment 329499 [details]
patch

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

I know that this patch was reviewed and landed a long time ago. I disagree with the direction of this patch.

>> Tools/ChangeLog:14
>> +        Currently to instanciate PlatformInfo, it requires arguments, which is usually
>> +        sys, platform and Executive(). They are straight forward and should be handled
>> +        by default arguments. Then we can instanciate PlatformInfo more casual.
> 
> Typo × 2:  instanciate => instantiate

PlatformInfo should never be instantiated in isolation. So, PlatformInfo should not have default argument values. The preferred way to get a PlatformInfo object is to instantiate a Host object. An example of this can be seen in check-webkit-style: <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/main.py?rev=229641#L126>.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:85
> +    @property
> +    def executive(self):
> +        if self._executive is None:
> +            self._executive = Executive()
> +
> +        return self._executive

This function is unnecessary. Notice that SystemHost(), which is responsible for instantiating a PlatformInfo, creates and exposes an Executive object. PlatformInfo should not be responsible for creating an Executive. Instead to construct a PlatformInfo a developer should be required to pass an Executive object.
Comment 13 Basuke Suzuki 2018-06-26 16:39:41 PDT
> PlatformInfo should never be instantiated in isolation. So, PlatformInfo should not have default argument values. The preferred way to get a PlatformInfo object is to instantiate a Host object. An example of this can be seen in check-webkit-style: <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/main.py?rev=229641#L126>.

I've restarted this patch.
Comment 14 Basuke Suzuki 2018-06-28 14:25:02 PDT
Created attachment 343851 [details]
Rolling out
Comment 15 Basuke Suzuki 2018-06-28 14:29:15 PDT
To rolling this out
Comment 16 Basuke Suzuki 2018-06-28 14:29:45 PDT
Created attachment 343853 [details]
Rolling out
Comment 17 WebKit Commit Bot 2018-06-28 15:13:10 PDT
Comment on attachment 343853 [details]
Rolling out

Clearing flags on attachment: 343853

Committed r233332: <https://trac.webkit.org/changeset/233332>
Comment 18 WebKit Commit Bot 2018-06-28 15:13:12 PDT
All reviewed patches have been landed.  Closing bug.