Bug 72680 - create a "SystemHost" object for webkitpy to slim down the Host object
Summary: create a "SystemHost" object for webkitpy to slim down the Host object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 17:07 PST by Dirk Pranke
Modified: 2011-12-08 16:20 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.67 KB, patch)
2011-11-17 17:59 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge to r102207 (9.87 KB, patch)
2011-12-06 19:28 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
move copy_current_environment to SystemHost, clean up (10.70 KB, patch)
2011-12-08 16:09 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-11-17 17:07:44 PST
There has been a bunch of work lately to consolidate factories into a Host object in webkitpy for easier testing. This is a good goal, but the Host object imports a bunch of network/service-level objects and introduces a number of dependencies; it makes the dependencies of objects that use Host harder to understand.

I think it would make sense to have a thinner object (call it a SystemHost) that only contains the objects from webkitpy.common.system in Host, and modify some of the factories and constructors like Port and PortFactory to take SystemHosts instead, so it can be clear(er?) that they may depend on FileSystem, User, etc. but don't depend on Buildbot, Bugzilla, etc.

The Host object can then subclass the SystemHost object.

Sound good?

-- Dirk
Comment 1 Eric Seidel (no email) 2011-11-17 17:10:38 PST
Sounds fine to me.
Comment 2 Adam Barth 2011-11-17 17:11:23 PST
I support the general idea of more clearly separating webkitpy into layers, including the Host object.  Maybe we should make a top-level package for classes that SystemHost is allowed to depend on and then forbid files in that directory from importing high-level packages?
Comment 3 Dirk Pranke 2011-11-17 17:26:05 PST
(In reply to comment #2)
> I support the general idea of more clearly separating webkitpy into layers, including the Host object.  Maybe we should make a top-level package for classes that SystemHost is allowed to depend on and then forbid files in that directory from importing high-level packages?

I'm thinking that webkit.common.system is the package that includes SystemHost and SystemHost is only allowed to include files from the same package. I would be fine if we wanted to move that to webkitpy.system just to flatten the hierarchy a bit, but it actually feels a bit cleaner to leave it under common to me.
Comment 4 Adam Barth 2011-11-17 17:30:08 PST
Ok.  That sounds good.  webkitpy.common.system is already not supposed to depend on things outside of itself.
Comment 5 Dirk Pranke 2011-11-17 17:59:22 PST
Created attachment 115718 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-11-17 18:00:56 PST
Comment on attachment 115718 [details]
Patch

Seems fine.
Comment 7 Eric Seidel (no email) 2011-11-17 18:01:17 PST
Comment on attachment 115718 [details]
Patch

I'd like to let Adam take a quick peek, but LGTM.
Comment 8 Dirk Pranke 2011-11-17 18:07:46 PST
I'm in no hurry to land this ... Adam?
Comment 9 Adam Barth 2011-11-17 18:24:04 PST
LGTM
Comment 10 Dirk Pranke 2011-11-17 18:42:06 PST
Hmm, I'm working on converting the Port objects over and it looks like the SystemHost object might need to be the one that holds the port_factory (thanks to the dryrun and mockdrt implementations). 

I don't quite understand the circularity that has crept in here, so I need to look a bit further to see if this can be broken up.
Comment 11 Dirk Pranke 2011-11-17 19:12:44 PST
(In reply to comment #10)
> Hmm, I'm working on converting the Port objects over and it looks like the SystemHost object might need to be the one that holds the port_factory (thanks to the dryrun and mockdrt implementations). 
> 
> I don't quite understand the circularity that has crept in here, so I need to look a bit further to see if this can be broken up.

Looks like it can be broken up. I will include that as part of the follow-on patch, so I think this is fine as-is.
Comment 12 Dirk Pranke 2011-12-06 19:28:34 PST
Created attachment 118162 [details]
merge to r102207
Comment 13 Eric Seidel (no email) 2011-12-07 11:14:01 PST
Comment on attachment 118162 [details]
merge to r102207

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

OK.  What about the SCM and Evnironment hacks in Host?  Should those move down too?  Seems they're related to the classes moved.  Thoughts?

> Tools/Scripts/webkitpy/common/host_mock.py:51
> -        self.filesystem = MockFileSystem(dirs=set([self._scm.checkout_root]))
> +        self.filesystem.maybe_make_directory(self._scm.checkout_root)

This is much nicer.  I do like this pattern of amending to the filesystem instead of replacing it.
Comment 14 Eric Seidel (no email) 2011-12-07 14:45:52 PST
Comment on attachment 118162 [details]
merge to r102207

Consider moving the Envinroment/SCM hacks too.
Comment 15 Dirk Pranke 2011-12-07 15:14:31 PST
(In reply to comment #14)
> (From update of attachment 118162 [details])
> Consider moving the Envinroment/SCM hacks too.

copy_current_environment() probably makes sense to move to SystemHost. SCM is less clear to me since it's not obvious that that's really a "system"-level thing, and it's still pretty intertwined w/ Checkout.

I'd like to land this (w/ the environment change), get the layout_tests infrastructure's usage of Hosts and SystemHosts cleaned up, and then hopefully have more insight into where the SCM feels like it should belong.
Comment 16 Dirk Pranke 2011-12-08 16:09:37 PST
Created attachment 118483 [details]
move copy_current_environment to SystemHost, clean up
Comment 17 Dirk Pranke 2011-12-08 16:20:49 PST
Committed r102393: <http://trac.webkit.org/changeset/102393>