Bug 36483 – webkitpy: Move the commands folder into webkitpy/patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

Part of the master bug to move files and folders into appropriate sub-packages:

https://bugs.webkit.org/show_bug.cgi?id=36093

[Chris Jerdonek]

Created attachment 51402 [details]
Proposed patch

For future reference, the command I used to do this was as follows:

find WebKitTools -type f \
\! \( -wholename '*.pyc' -or \
      -wholename '*.svn*' -or \
      -wholename '*ChangeLog*' \) | \
sed -e 's/ /\\ /g' | \
xargs grep -l 'webkitpy\.commands' | \
xargs sed -i '' 's/webkitpy\.commands/webkitpy\.patch\.commands/'

(Thanks to Maciej for the tip: 
 https://lists.webkit.org/pipermail/webkit-dev/2010-March/012064.html )

[Adam Barth]

Comment on attachment 51402 [details]
Proposed patch

+ from webkitpy.patch.commands.download_unittest import *

Shouldn't these be in a unittests.py file in the commands module?

Please coordinate with Eric when landing this patch.  He has some in-flight changes to these files.

[Chris Jerdonek]

(In reply to comment #2)

Thanks, Adam!

> (From update of attachment 51402 [details])
> + from webkitpy.patch.commands.download_unittest import *
> 
> Shouldn't these be in a unittests.py file in the commands module?

Personally, I think so.  (I myself haven't worked on the commands unit tests.)  We can do that in a separate patch.

> Please coordinate with Eric when landing this patch.  He has some in-flight
> changes to these files.

Will do!

[Eric Seidel (no email)]

Comment on attachment 51402 [details]
Proposed patch

I'm not sure this change really helps anything.

[Eric Seidel (no email)]

Comment on attachment 51402 [details]
Proposed patch

I think that since commands and steps contain so much stuff that is not relating to "patching" it makes no sense to put them under a "patch" directory.  And especially since the system is designed for other tools to be able to include these command and steps, it seems wrong to put them under the directory of some specific tool.

I'm against this change.

[Chris Jerdonek]

(In reply to comment #5)
> (From update of attachment 51402 [details])
> I think that since commands and steps contain so much stuff that is not
> relating to "patching" it makes no sense to put them under a "patch" directory.
>  And especially since the system is designed for other tools to be able to
> include these command and steps, it seems wrong to put them under the directory
> of some specific tool.
> 
> I'm against this change.

What's nice about this change is that now nothing outside of patch/ imports anything in the commands/ folder.  It helps to organize and control the dependencies.  Only patch/patcher.py and the command modules themselves import from it (as well as unittests.py which will change once command/ gets its own unittests.py).

It sounds like maybe "patch" is just a bad name for the top-level package.  I just chose "patch" because it held the main() method for webkit-patch.  Adam was okay with this, and we agreed we can choose a better name in a later patch:

https://bugs.webkit.org/show_bug.cgi?id=31533#c7

Basically, I guess "webkitpy/patch" is becoming what webkitpy was in the very beginning (just getting pushed down a level).  Originally that folder was called something like "modules" if I remember right.  But it contained only bugzilla-tool related code.  Any suggestions for a better name?

[Eric Seidel (no email)]

So when/if we add another tool for non-patch related things, where does that go?  Another top level?  or inside "patch/"

[Eric Seidel (no email)]

Eventually we'll probably want to break down the individual files in commands/ into smaller chunks.  possibly even one-file-per-class.

some of the command infrastructure is designed to be general however.  Like "Command" itself, and the whole AbstractDeclaritiveComamnd and MultiCommandTool and steps etc.  it's unclear to me why that goes under patch/ if patch is just for "webkit-patch".

I see more scripts moving to be more on more of the wekbit-tools infrastructure (because it was designed with re-use inmind) not the other way around.

Adam talked with breaking out all the data-storage from bugzilla-tool (now patcher.py I think?) into its own tool.py which dealt with holding all the central state that we expect "tool" to have.  If/when that ever happens than it becomes trivial to write additional tools and single-command tools, as we have long envisoned.  Tools which have nothing to do with patching, simply exist on top of the various infrastructure pieces we've made to make it easy to write webkit-aware python.

[Eric Seidel (no email)]

I think what I'm unclear on is what "patch/" is for.  What goes in it.  What doesn't.

[Chris Jerdonek]

The comments here are relevant to this discussion:

https://bugs.webkit.org/show_bug.cgi?id=28459

This is the original report in which bugzilla-tool was renamed to webkit-patch -- getting into some of the issues around how webkit-patch has a fair amount of "non-patch" functionality.

[Chris Jerdonek]

(In reply to comment #9)
> I think what I'm unclear on is what "patch/" is for.  What goes in it.  What
> doesn't.

My thinking was that it should contain the code that is used by webkit-patch and none of the other scripts.

There is a parallel set of questions about what functionality should go in webkit-patch as opposed to separate scripts:

https://bugs.webkit.org/show_bug.cgi?id=28459#c8

[Eric Seidel (no email)]

I don't think we know what all the pieces of webkit-patch will get re-used.  I think much more will be re-used than currently is.  If we ever have a second multi-command tool certainly many of the steps will get re-used.

I don't think commiters.py is used by anything else yet, but it's clearly common infrastructure.  scm.py is also not re-used yet, but I expect new-run-webkit-tests to use it eventually.  likewise bugzilla, buildbot, statusbot, irc, etc.  I think eventually command and multicommandtool and many of the steps.  Some of the command would be specific to webkit-patch in any reasonable future of course.

[Chris Jerdonek]

(In reply to comment #7)
> So when/if we add another tool for non-patch related things, where does that
> go?  Another top level?  or inside "patch/"

My thinking on this was that we could consider having a separate top-level folder for each major script -- with the package name ideally having some resemblance to the script name (check-webkit-style -> style, webkit-patch -> patch, run-webkit-tests -> layout_tests, etc).  If two scripts are very closely related or variants of each other, we may want to consider putting them in the same top-level package.

Code used by multiple top-level folders could go in common -- organized however we want.

[Eric Seidel (no email)]

I get more excited about splitting out existing files into modules (like bugzilla.py or scm.py) and moving exiting sets of files down a folder (like grammar, executive, user, etc.) than moving commands out of the top level.

I think my major trouble is with the "patch" directory name, and the choice of this as a first order of business.

I think it's more interesting to try and make other scripts fit into the new python infrastructure than trying to box the new python infrastructure off into its own little corner.

[Chris Jerdonek]

(In reply to comment #8)
> Eventually we'll probably want to break down the individual files in commands/
> into smaller chunks.  possibly even one-file-per-class.
> 
> some of the command infrastructure is designed to be general however.  Like
> "Command" itself, and the whole AbstractDeclaritiveComamnd and MultiCommandTool
> and steps etc.  it's unclear to me why that goes under patch/ if patch is just
> for "webkit-patch".
> 
> I see more scripts moving to be more on more of the wekbit-tools infrastructure
> (because it was designed with re-use inmind) not the other way around.

At that point we can move such modules to common/.  Until they are actually used by more than one script though, it's not really necessary and keeps the references simpler.  It's easier to start out keeping things separate and to promote them to being shared than to go the other way around.

See my comment at the end of here, for example:

https://bugs.webkit.org/show_bug.cgi?id=36093#c3

"My initial feeling is that we should try to put modules into the inner-most
package in which they are used (thirdparty/ seems to be an exception).  This
way a module has to "prove" itself before being moved to the shared package at
the root level (i.e. common/).  This will help us limit the size of common/."

> Adam talked with breaking out all the data-storage from bugzilla-tool (now
> patcher.py I think?) into its own tool.py which dealt with holding all the
> central state that we expect "tool" to have.  If/when that ever happens than it
> becomes trivial to write additional tools and single-command tools, as we have
> long envisoned.  Tools which have nothing to do with patching, simply exist on
> top of the various infrastructure pieces we've made to make it easy to write
> webkit-aware python.

That sounds cool.  When the time comes, I think we could find an appropriate place for such code -- perhaps somewhere in common.  For now, I'm just trying to group things up by their actual dependencies, so webkitpy is a bit easier to navigate.

[Chris Jerdonek]

(In reply to comment #12)
> I don't think we know what all the pieces of webkit-patch will get re-used.  I
> think much more will be re-used than currently is.  If we ever have a second
> multi-command tool certainly many of the steps will get re-used.

We can always promote to common at the point it which it gets re-used.  It should be easy to do.

> I don't think commiters.py is used by anything else yet, but it's clearly
> common infrastructure.

Adam and I discussed moving that to common/config when the time comes:

https://bugs.webkit.org/show_bug.cgi?id=36093#c5

>scm.py is also not re-used yet, but I expect
> new-run-webkit-tests to use it eventually.

It's actually used by check-webkit-style (to check style for patches).  diff_parser.py is another one.  That's why I was proposing putting those in common/scm:

https://bugs.webkit.org/show_bug.cgi?id=36093#c10

> likewise bugzilla, buildbot,
> statusbot, irc, etc.  I think eventually command and multicommandtool and many
> of the steps.  Some of the command would be specific to webkit-patch in any
> reasonable future of course.

Yeah, we could leave behind whatever is unique to that script to keep the dependencies more understandable.

[Chris Jerdonek]

FYI, this afternoon Eric suggested that he, Adam, and I meet to discuss this in person, so we will be doing that tomorrow (Wednesday) evening.  So I will hold off on responding to remaining comments (e.g. comment 14) until then.

[Chris Jerdonek]

Manually committed:

http://trac.webkit.org/changeset/56494

(This was reviewed again by Adam in Eric's presence after discussion. :) )