Bug 72680 – create a "SystemHost" object for webkitpy to slim down the Host object

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

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

[Eric Seidel (no email)]

Sounds fine to me.

[Adam Barth]

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?

[Dirk Pranke]

(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.

[Adam Barth]

Ok.  That sounds good.  webkitpy.common.system is already not supposed to depend on things outside of itself.

[Dirk Pranke]

Created attachment 115718 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 115718 [details]
Patch

Seems fine.

[Eric Seidel (no email)]

Comment on attachment 115718 [details]
Patch

I'd like to let Adam take a quick peek, but LGTM.

[Dirk Pranke]

I'm in no hurry to land this ... Adam?

[Adam Barth]

LGTM

[Dirk Pranke]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 118162 [details]
merge to r102207

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 118162 [details]
merge to r102207

Consider moving the Envinroment/SCM hacks too.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 118483 [details]
move copy_current_environment to SystemHost, clean up

[Dirk Pranke]

Committed r102393: <http://trac.webkit.org/changeset/102393>