WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
,
EWS Watchlist
no flags
Details
View All
Add attachment
proposed patch, testcase, etc.
Basuke Suzuki
Comment 1
2018-03-12 12:32:32 PDT
Created
attachment 335624
[details]
PATCH
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.
Top of Page
Format For Printing
XML
Clone This Bug