Bug 36093 – webkitpy: Move modules in webkitpy/ into appropriate sub-packages (master bug)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This is a master bug for discussion purposes, and because these moves will probably be spread over more than one patch.

Initial thoughts:

* In the end, we probably want webkitpy/ to contain only the following as
  immediate children:
  - Package folders
  - Their associated *_references.py modules.
  - And perhaps unittests.py.
* webkit-patch specific files can go in webkitpy/patch, as was discussed here:
  https://bugs.webkit.org/show_bug.cgi?id=35499#c9
* Some of the modules used by more than just webkit-patch seem to be--
  - diff_parser
  - scm and its dependencies (changelogs, executive, user, webkit_logging)
* We may want to group the scm-related files into an scm package.
* We might want to explicitly partition off code that needs to work with Python
  2.4 (e.g. into webkitpy/python24).  The version-checking code, for example,
  needs to work with Python 2.4 so we can report a nice error message to Python
  2.4 users.  By separating it off, it will be easier to ensure that this code does
  not accidentally import code that requires Python 2.5 and break.

Initial questions:

(1) Are the webkitpy/commands and webkitpy/steps packages webkit-patch
    specific?
(2) Aside from patch, scm, and any infra-structure related modules that can
    go in webkitpy/init, do you see any other possibilities for packages that should
    go immediately under webkitpy?  For example, are there any other applications
    distinct from webkit-patch that rely on the scm-related code in webkitpy?

[Adam Barth]

Here's a candidate layout:

webkitpy/
  __init__.py
  init/
  layout_tests/
  style/
  style_references.py
  thirdparty/
  patch/
    commands/
    commands_references.py
    steps/
    steps_references.py
    bot/
      patchcollection.py
      patchcollection_unittest.py
      queueengine.py
      queueengine_unittest.py
    stepsequence.py
    comments.py
    mock_bugzillatool.py <-- wrong name
    multicommandtool.py
    multicommandtool_unittest.py
  config/
    committers.py
    committers_unittest.py
    webkitport.py
    webkitport_unittest.py
  scm/
    changelogs.py
    changelogs_unittest.py
    diff_parser.py
    diff_parser_unittest.py
    scm.py <-- should become many files
    scm_unittest.py
  net/ <-- classes that talk to servers
    bugzilla.py
    bugzilla_unittest.py
    buildbot.py
    buildbot_unittest.py
    credentials.py
    credentials_unittest.py
    networktransaction.py
    networktransaction_unittest.py
    statusserver.py
  base/
    grammar.py
    grammar_unittest.py
    executive.py
    executive_unittest.py
    outputcapture.py <-- probably should die
    user.py
    user_unittest.py
    webkit_logging.py <-- probably should die
    webkit_logging_unittest.py
  unittests.py <-- not sure

[Adam Barth]

I know the dependences probably aren't pretty right now, but that's something to aim for at least.

[Chris Jerdonek]

(In reply to comment #1)
> Here's a candidate layout:

Here's a suggested revision.  For conciseness, I've left out the *_references.py and *_unittest.py files, which should go in as siblings of their corresponding package folders and modules, respectively.

webkitpy/
  common/ <-- contains modules used by more than one root-level package
    infra/
      autoinstall.py <-- after re-landing the rewrite
      executive.py
      logtesting.py
      logutils.py
      user.py
      webkit_logging.py <-- probably should die
    scm/
      changelogs.py
      diff_parser.py
      scm.py <-- should become many files
  layout_tests/
  patch/
    bot/
      patchcollection.py
      queueengine.py
    commands/
    config/
      committers.py
      webkitport.py
    net/ <-- classes that talk to servers
      bugzilla.py
      buildbot.py
      credentials.py
      networktransaction.py
      statusserver.py
    steps/
    comments.py
    grammar.py
    mock_bugzillatool.py <-- wrong name
    multicommandtool.py
    outputcapture.py <-- probably should die
    stepsequence.py
  python24/
    versioning.py
  style/
  thirdparty/
  unittests.py <-- not sure

I tried moving config/, net/, grammar.py, and outputcapture.py into the patch/ folder because they don't seem to be used by non-patch modules.

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 Barth]

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

That sounds like a good plan.  We might want to move config and networktransaction to common now because we have immediate plans to use them in layout_tests.

[Chris Jerdonek]

(In reply to comment #4)
> > 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/.
> 
> That sounds like a good plan.  We might want to move config and
> networktransaction to common now because we have immediate plans to use them in
> layout_tests.

I assume that would just be config/webkitport.py?  Sounds good, though we might want to wait on those until the versioning stuff is decided.  It seems like some people may still want to preserve 2.4 for layout_tests, and networktransaction and webkitport both currently require 2.5 (for example, networktransaction imports mechanize, which in turn needs autoinstall).

By the way, how do you feel about renaming webkitport.py to ports.py since it will be in a webkitpy config/ directory and contain configurations for multiple ports?  And similarly, networktransaction.py -> transaction.py since it will be in a net/ directory?  (Just the modules and not the class names within.)

[Chris Jerdonek]

(In reply to comment #5)
> And similarly, networktransaction.py -> transaction.py since it will be
> in a net/ directory?  (Just the modules and not the class names within.)

Or perhaps common/infra/net.py (or network.py or networktransaction.py) would be a better location for now.

[Adam Barth]

webkitport => ports is a good idea.  I'm not sure about networktransaction.  I never really liked the name, but I like the class.

[Adam Barth]

There's also a notion of ports in layout_test.  Those need to get merged at some point.  There's a patch in review now for layout_test that needs networktransaction (the one about uploading the JSON to AppEngine).

[Chris Jerdonek]

(In reply to comment #8)
> There's a patch in review now for layout_test that needs
> networktransaction (the one about uploading the JSON to AppEngine).

So others know, the patch referred to here is associated with this report:

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

[Chris Jerdonek]

Updating the plan for the new hierarchy.

(Again leaving out *_references.py and *_unittest.py files.)

webkitpy/
  common/ <-- contains modules used by more than one root-level package
    config/
      ports.py (was webkitport.py)
    infra/
      autoinstall.py <-- after re-landing the rewrite
      executive.py
      logtesting.py
      logutils.py
      net.py (was networktransaction.py)
      user.py
      webkit_logging.py <-- probably should die
    scm/
      changelogs.py
      diff_parser.py
      scm.py <-- should become many files
  layout_tests/
  patch/
    bot/
      patchcollection.py
      queueengine.py
    commands/
    config/
      committers.py
    irc/
    net/ <-- classes that talk to servers
      bugzilla.py
      buildbot.py
      credentials.py
      statusserver.py
    steps/
    comments.py
    grammar.py
    mock_bugzillatool.py <-- wrong name
    multicommandtool.py
    outputcapture.py <-- probably should die
    stepsequence.py
  python24/
    versioning.py
  style/
  test/
    main.py (move code from test-webkitpy)
    unittests.py
  thirdparty/

[Adam Barth]

webkitpy/
  common/ <-- contains modules used by more than one root-level package
    config/
      ports.py (was webkitport.py)
      committers.py
    infra/
      autoinstall.py <-- after re-landing the rewrite
      executive.py
      logtesting.py
      logutils.py
      user.py
      webkit_logging.py <-- probably should die
    scm/
      changelogs.py
      diff_parser.py
      scm.py <-- should become many files
    net/ <-- classes that talk to servers
      irc/
      bugzilla.py
      buildbot.py
      credentials.py
      networktransaction.py
      statusserver.py
  layout_tests/
  patch/
    bot/
      patchcollection.py
      queueengine.py
    commands/
    steps/
    comments.py
    grammar.py  <-- WTF?
    mock_bugzillatool.py <-- wrong name
    multicommandtool.py
    outputcapture.py <-- probably should die
    stepsequence.py
  python24/
    versioning.py
  style/
  test/
    main.py (move code from test-webkitpy)
    unittests.py
  thirdparty/

[Chris Jerdonek]

Manually committed:

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

(Moves webkitpy/steps/ to webkitpy/tools/steps.)

[Chris Jerdonek]

Manually committed:

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

(Moves webkitport.py and committers.py into common/config/.)

[Adam Barth]

webkitpy.common.net in http://trac.webkit.org/changeset/56508

[Chris Jerdonek]

Moved init/versioning.py into python24/versioning.py:

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

[Adam Barth]

webkitpy.common.checkout in http://trac.webkit.org/changeset/56510

[Adam Barth]

Committed r56512: <http://trac.webkit.org/changeset/56512>

[Adam Barth]

stepsequence in http://trac.webkit.org/changeset/56512

[Chris Jerdonek]

Moved init/ to common/system/:

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

[Adam Barth]

Committed r56516: <http://trac.webkit.org/changeset/56516>

[Adam Barth]

grammar.py in http://trac.webkit.org/changeset/56516

[Adam Barth]

Committed r56517: <http://trac.webkit.org/changeset/56517>

[Adam Barth]

Committed r56518: <http://trac.webkit.org/changeset/56518>

[Adam Barth]

Committed r56519: <http://trac.webkit.org/changeset/56519>

[Adam Barth]

Committed r56520: <http://trac.webkit.org/changeset/56520>

[Chris Jerdonek]

Divided the unittest files up:

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

[Adam Barth]

Committed r56522: <http://trac.webkit.org/changeset/56522>

[Chris Jerdonek]

Moved webkit_logging.py to common/system/deprecated_logging.py:

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