Bug 102007 – webkitpy: consolidate webkit-base-finding code

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkitpy: consolidate webkit-base-finding code

[Dirk Pranke]

Created attachment 173756 [details]
Patch

[Dirk Pranke]

Comment on attachment 173756 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/systemhost.py:51
> +    def webkit_base_dir(self):

I'm not actually sure that SystemHost is the best place for this. We know we don't want it in layout_tests.port, and it doesn't seem like common/config/Checkout is the right place (since that is too version-control-centric).

Arguably workspace.Workspace() might be the right place, but I'm not actually sure why that class exists and it seems to introduce an unnecessary additional concept. Maybe Workspace() should be merged into SystemHost()?

[Eric Seidel (no email)]

Checkout seems like the right place for finding your chromium vs. no checkout, etc.  It could also be exteneded to support tarballs, or whatever other diretory structures folks want.

Workspace was just a place to stick the zip and "find me a filename" logic, which seemed above FileSystem, but not really fitting for Checkout.  I'm open to other names or organizations.

[Dirk Pranke]

(In reply to comment #3)
> Checkout seems like the right place for finding your chromium vs. no checkout, etc.  It could also be exteneded to support tarballs, or whatever other diretory structures folks want.
> 

Checkout seems to require an scm object in order to function, and seems to be concerned with changelogs and other stuff specific to actually doing things with version-control. It doesn't seem like the rest of the class would work without a real scm object, so moving these routines there seems like a bad fit.

> Workspace was just a place to stick the zip and "find me a filename" logic, which seemed above FileSystem, but not really fitting for Checkout.  I'm open to other names or organizations.

Do you dislike just keeping this stuff in SystemHost (and maybe getting rid of Workspace)?

[Eric Seidel (no email)]

Maybe Checkout should have some sort of superclass (like Workspace?) which handled the idea of a general place where the code is stored, and was not dependent on SCM?

[Eric Seidel (no email)]

SystemHost shouldn't need to know anything about WebKit, should it?  It's supposed to be your basic mocking root for how you talk to the outside world?

[Dirk Pranke]

(In reply to comment #5)
> Maybe Checkout should have some sort of superclass (like Workspace?) which handled the idea of a general place where the code is stored, and was not dependent on SCM?

I'm open to something like this, though it's not clear to me that there's really anything really in common with Checkout and Workspace at all.

(In reply to comment #6)
> SystemHost shouldn't need to know anything about WebKit, should it?  It's supposed to be your basic mocking root for how you talk to the outside world?

I don't know that SystemHost has this specifically as its raison d'etre. I wonder how many things we write at all in webkitpy that want a systemhost (or anything) but don't want either a full host or at least something that implements these few new routines (the layout_tests.port stuff may be the only code that depends on systemhost rather than host, and that's true to avoid circular dependencies and because I don't want the port stuff to depend on SCM objects). 

I also worry a bit about a proliferation of fine-grained (mostly base) classes. It looks like the two routines in Workspace are only used by tool.bot.layouttestresultsreader; I'd probably just roll them into that class until someone else actually has a need for them.

WDYT?

[Dirk Pranke]

Created attachment 173770 [details]
add missed tests

[Eric Seidel (no email)]

(In reply to comment #7)
> (In reply to comment #5)
> I also worry a bit about a proliferation of fine-grained (mostly base) classes. It looks like the two routines in Workspace are only used by tool.bot.layouttestresultsreader; I'd probably just roll them into that class until someone else actually has a need for them.
> 
> WDYT?

Thats fine, or you could simply move them into that class.

One of my goals with webkitpy was to write re-usable code. :)  When useful I tried to split things out into classes.  i agree that Workspace has seen little other use (although I just used the find_unused_filename code in my --profile patch last week).

In any case, you're in this code much more than I am these days.  I don't think small classes are bad.  I do think monolithic classes are.  And files with lots of classes are similarly bad.

[Eric Seidel (no email)]

Comment on attachment 173770 [details]
add missed tests

SystemHost currently ahs nothing to do with WebKit.  it's simply a mock-point for writing python code which can be easily mocked.  I dont' think this is the right direction.  I do support consolidating this logic. But SystemHost is the wrong place IMO.

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 173770 [details])
> SystemHost currently ahs nothing to do with WebKit.  it's simply a mock-point for writing python code which can be easily mocked.  I dont' think this is the right direction.  I do support consolidating this logic. But SystemHost is the wrong place IMO.

Okay, I've got no real problem with that.

How about I move these functions into Workspace, and then (in separate follow-on patches) move the existing functions in Workspace into LayoutTestsResultReader and make Checkout derive from Workspace?

[Dirk Pranke]

Whoops, didn't see this comment ...

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #5)
> > I also worry a bit about a proliferation of fine-grained (mostly base) classes. It looks like the two routines in Workspace are only used by tool.bot.layouttestresultsreader; I'd probably just roll them into that class until someone else actually has a need for them.
> > 
> > WDYT?
> 
> Thats fine, or you could simply move them into that class.
> 
> One of my goals with webkitpy was to write re-usable code. :)  When useful I tried to split things out into classes.  i agree that Workspace has seen little other use (although I just used the find_unused_filename code in my --profile patch last week).
> 

Don't think I saw the reference to that ..


> In any case, you're in this code much more than I am these days.  I don't think small classes are bad.  I do think monolithic classes are.  And files with lots of classes are similarly bad.

I don't think small classes are bad per se, and I definitely agree that both big classes and "catch-all" classes (like Port) are bad. Of course, having to jump between multiple places and files to follow the code incurs its own cost. I tend to like to break things out into their own files only if they are actually being reused or if reducing the size of the files does make things easier to file

I agree that putting webkit_base_dir() in SystemHost is at best a grey area, and wouldn't really argue that it's just bad :).

[Dirk Pranke]

Created attachment 173987 [details]
Patch

[Dirk Pranke]

Created attachment 174296 [details]
rework into a separate new WebKitFinder module

[Dirk Pranke]

fourth times' a charm?

[Eric Seidel (no email)]

Comment on attachment 174296 [details]
rework into a separate new WebKitFinder module

LGTM.

[Dirk Pranke]

Committed r134701: <http://trac.webkit.org/changeset/134701>