Bug 180829 - [webkitpy] Use PlatformInfo in Executive class.
Summary: [webkitpy] Use PlatformInfo in Executive class.
Status: ASSIGNED
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:
Depends on: 180827
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-14 12:38 PST by Basuke Suzuki
Modified: 2018-03-16 16:10 PDT (History)
8 users (show)

See Also:


Attachments
PATCH (8.76 KB, patch)
2018-03-12 12:32 PDT, Basuke Suzuki
dbates: review-
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.01 MB, application/zip)
2018-03-12 14:42 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-12-14 12:38:44 PST
There're many sys.platform dependencies in Executive class, but they should be replaced by PlatformInfo class.
Comment 1 Basuke Suzuki 2018-03-12 12:32:32 PDT
Created attachment 335624 [details]
PATCH
Comment 2 Daniel Bates 2018-03-12 13:50:41 PDT
Comment on attachment 335624 [details]
PATCH

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

> Tools/ChangeLog:8
> +        Remove direct sys dependencies form Executive class and use PlatformInfo.

Can you explain the motivation for this change? I mean, it creates a circular dependency and needs the workaround you did in PlatformInfo.
Comment 3 Build Bot 2018-03-12 14:42:29 PDT
Comment on attachment 335624 [details]
PATCH

Attachment 335624 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6921075

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 4 Build Bot 2018-03-12 14:42:40 PDT
Created attachment 335642 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 5 Basuke Suzuki 2018-03-12 16:03:27 PDT
(In reply to Daniel Bates from comment #2)
> Comment on attachment 335624 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335624&action=review
> 
> > Tools/ChangeLog:8
> > +        Remove direct sys dependencies form Executive class and use PlatformInfo.
> 
> Can you explain the motivation for this change? I mean, it creates a
> circular dependency and needs the workaround you did in PlatformInfo.

Motivation is simple. Current platform detection is so messy. Every module uses different way to detect running platform, especially detection of cygwin and native_win .

Also direct detect makes mock testing difficult.

My goal is to make PlatformInfo as the unique way of platform detection.

The workaround causes because PlatformInfo do too much by itself. The executive is used to detect Xcode version or total memory size. Both are only for Mac. They can safely move to Mac port, I guess.
Comment 6 Daniel Bates 2018-03-16 15:18:31 PDT
Comment on attachment 335624 [details]
PATCH

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

r-, this creates a cycle between the Executive class and PlatformInfo class. I also see little value in replacing the platform checks in Executive with PlatformInfo because these replacements are mostly cosmetic and do little more than turn around and query sys.platform. PlatformInfo.is_native_win() is one convenience function whose usage improves the readability of the code. However I do not feel that it justifies creating a cycle where Executive depends on PlatformInfo.

Currently Executive is one of the fundamental building blocks of webkitpy. PlatformInfo sits on top of Executive and for the most part is a thin wrapper around Python's sys.platform module and exposes functions to query OS capabilities. The latter requires that is have an Executive. Maybe we can find another place for the latter, but the port is not the appropriate place for this logic because a single platform may support more than one WebKit port. Windows is an example of a platform that supports both the Apple Windows and Win Cairo ports.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:79
>      def executive(self):
>          if self._executive is None:
> -            self._executive = Executive()
> +            from webkitpy.common.system.executive import Executive
> +            self._executive = Executive(self)
>  
>          return self._executive
>  

Please remove this code. As I wrote in bug 180827, comment 12, PlatformInfo should never be instantiated in isolation. It should be instantiated as part of a Host object.
Comment 7 Daniel Bates 2018-03-16 15:19:41 PDT
(In reply to Basuke Suzuki from comment #5)
> My goal is to make PlatformInfo as the unique way of platform detection.
> 

I disagree with this goal. See my remarks in bug 180827, comment 12 and comment #6 for more details.
Comment 8 Basuke Suzuki 2018-03-16 16:10:21 PDT
(In reply to Daniel Bates from comment #7)
> (In reply to Basuke Suzuki from comment #5)
> > My goal is to make PlatformInfo as the unique way of platform detection.
> > 
> 
> I disagree with this goal. See my remarks in bug 180827, comment 12 and
> comment #6 for more details.

> The preferred way to get a PlatformInfo object is to instantiate a Host object.

Okay, we can switch to use Host().platform for casual usage, but still weird to use PlatformInfo inside Executive. There're many platform dependent code in Executive and PlatformInfo should be used to detect such platform. Forget about my patch, I believe you agree with this goal, don't you?

The issue may platform depends on Executive and Executive does not have any on instantiation. How about changing this to both depends host object? If platform requires executive, it can be accessible via self.host.executive. And Executive can use platform via self.host.executive.