RESOLVED WONTFIX 180829
[webkitpy] Use PlatformInfo in Executive class.
https://bugs.webkit.org/show_bug.cgi?id=180829
Summary [webkitpy] Use PlatformInfo in Executive class.
Basuke Suzuki
Reported 2017-12-14 12:38:44 PST
There're many sys.platform dependencies in Executive class, but they should be replaced by PlatformInfo class.
Attachments
PATCH (8.76 KB, patch)
2018-03-12 12:32 PDT, Basuke Suzuki
dbates: review-
ews-watchlist: commit-queue-
Archive of layout-test-results from ews205 for win-future (12.01 MB, application/zip)
2018-03-12 14:42 PDT, EWS Watchlist
no flags
Basuke Suzuki
Comment 1 2018-03-12 12:32:32 PDT
Daniel Bates
Comment 2 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.
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
Basuke Suzuki
Comment 5 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.
Daniel Bates
Comment 6 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.
Daniel Bates
Comment 7 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.
Basuke Suzuki
Comment 8 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.
Basuke Suzuki
Comment 9 2020-01-15 12:48:46 PST
Cannot get agreement for the goal.
Note You need to log in before you can comment on or make changes to this bug.