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
Sounds fine to me.
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?
(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.
Ok. That sounds good. webkitpy.common.system is already not supposed to depend on things outside of itself.
Created attachment 115718 [details] Patch
Comment on attachment 115718 [details] Patch Seems fine.
Comment on attachment 115718 [details] Patch I'd like to let Adam take a quick peek, but LGTM.
I'm in no hurry to land this ... Adam?
LGTM
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.
(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.
Created attachment 118162 [details] merge to r102207
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 on attachment 118162 [details] merge to r102207 Consider moving the Envinroment/SCM hacks too.
(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.
Created attachment 118483 [details] move copy_current_environment to SystemHost, clean up
Committed r102393: <http://trac.webkit.org/changeset/102393>