Summary: | check-webkit-style: Checking patch does not work when script not run from source root | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cjerdonek, eric, hamaji, levin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | 33775 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Chris Jerdonek
2010-01-18 03:21:38 PST
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 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.
(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). 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. (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. 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". |