Bug 33791 – check-webkit-style: Checking patch does not work when script not run from source root

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

I get errors like the following when running check-webkit-style from a folder other than the source root -- e.g. WebKitTools/

Skipping input 'WebKitTools/Scripts/webkitpy/style/text_style.py': Can't open for reading
Skipping input 'WebKitTools/Scripts/webkitpy/style/checker.py': Can't open for reading

This is a one-line fix (adding a call to os.chdir() in check-webkit-style), but I'm going to submit another change in this patch as well.

[Chris Jerdonek]

Created attachment 46809 [details]
Proposed patch

[Serial patch: this patch assumes the patch from bug 33775 has been checked in (awaiting review). As a result, the status bots may not work.]

In this patch, in addition to the one-line fix, I'm also experimenting with a new way to manage intra-package references.  As a sibling to the style/ folder, I'm adding a style_references.py module to house external dependencies.  The reasoning behind this is more fully explained in the code comments at the beginning of style_references.py.

As a side benefit, this also allows us to remove the __path__ hack that I added a while back to __init__.py.  See, for instance--

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

[Adam Barth]

Comment on attachment 46809 [details]
Proposed patch

This looks good, especially because the style module has so few dependencies on the rest of webkitpy.  We'll have to see how this approach scales to other modules.

Why is style_references outside the style module?  I would have expected it to be located at style.references.

[Chris Jerdonek]

(In reply to comment #2)
> (From update of attachment 46809 [details])
> This looks good, especially because the style module has so few dependencies on
> the rest of webkitpy.  We'll have to see how this approach scales to other
> modules.
> 
> Why is style_references outside the style module?  I would have expected it to
> be located at style.references.

Good question. It's largely a matter of convention, but my reasons were--

(1) This way, if non-style modules (for example) need to be refactored in ways that affect the style references (e.g. moving or renaming the scm module), this can be done without having to modify the contents of the style package at all.  (One of the purposes of the references file is to insulate the style package from changes taking place in other modules/packages.)  I think this will also make viewing the source hierarchy in trac a bit more convenient:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy

The "Last Change" next to style/ will always be the last change to the logic of the style package rather than a change to the workings of one of its dependencies (which are always changing anyways).

(2) We don't need to have any "zig-zag" dependencies, e.g.--

from ..diff_parser
from ..scm

which may be conceptually simpler.  The style package just has a single reference up, and then the reference file has a bunch of references down (or across).

(3) I think the intra-package references will be more visible this way.  Someone will be able to inspect intra-package references without having to navigate into many sub-folders.  And it will be easy to tell which packages have intra-package references simply by viewing the top-level webkitpy folder and seeing which ones have an adjacent *_references.py file.  As we move forward, I think this setup will allow us to take stock of and manage our intra-package references more easily -- perhaps choosing a different pattern in the process.

It's probably not worth thinking too far ahead, but it seems like this approach might provide a natural transition to an approach where we don't have any ".." references at all (think dependency injection).

[Chris Jerdonek]

Manually committed (via "git svn dcommit") r53715:

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

There was a slight problem though in that commit-log-editor didn't kick in and include the ChangeLog as the commit message.  I'm not sure what I did wrong.  I followed the instructions on the wiki and included commit-log-editor in my core.editor config:

http://trac.webkit.org/wiki/UsingGitWithWebKit

Is this something I should correct (the commit log message for the check-in, I mean)?  Thanks.

[Chris Jerdonek]

(In reply to comment #4)
> Manually committed (via "git svn dcommit") r53715:
> 
>     http://trac.webkit.org/changeset/53715
> 
> There was a slight problem though in that commit-log-editor didn't kick in and
> include the ChangeLog as the commit message.  I'm not sure what I did wrong.  I
> followed the instructions on the wiki and included commit-log-editor in my
> core.editor config:
> 
> http://trac.webkit.org/wiki/UsingGitWithWebKit
> 
> Is this something I should correct (the commit log message for the check-in, I
> mean)?  Thanks.

Oh, I understand what I did wrong, now.  I should simply have ensured that my local commit message was correct (e.g. using commit-log-editor with commit, prior to dcommit).  This is simpler than for SVN.  :)

Still let me know if I should correct the message in the repo.  Thanks.

[Eric Seidel (no email)]

I too have never mastered svn-commit-editor.  webkit-patch has its own logic (sadly), and doesn't require setting up a commit-log-editor when using commands like "land".