Bug 68973 – watchlist: Add webkit-patch command to run watchlist.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Levin]

See summary.

[David Levin]

Created attachment 109089 [details]
WIP

There are many loose ends to clean up (adding a changelog and some form of tests) but I wanted to make sure this is the right approach.

[WebKit Review Bot]

Attachment 109089 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/Scripts/webkitpy/tool/commands/downl..." exit_code: 1

Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:2:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:6:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:16:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:33:  expected 2 blank lines, found 1  [pep8/E302] [5]
Tools/Scripts/webkitpy/tool/commands/download.py:260:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 109089 [details]
WIP

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

>> Tools/Scripts/webkitpy/tool/commands/download.py:260
>> +        steps.ApplyWatchListToPatch
> 
> whitespace before ']'  [pep8/E202] [5]

It wants to you add a comma to the end of the line.  Maybe should just be "ApplyWatchlist" ?

> Tools/Scripts/webkitpy/tool/commands/download.py:262
> +    long_help = """ """

Better to have some long help explaining what this does.

Also, you should add a test.  Testing commands is very easy.  :)

>> Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:33
>> +class ApplyWatchListToPatch(AbstractStep):
> 
> expected 2 blank lines, found 1  [pep8/E302] [5]

The class name should also match the file name.

> Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:35
> +        log("Processing patch %s from bug %s." % (state["patch"].id(), state["patch"].bug_id()))

This shouldn't log like this.  Something else will do the logging for you.

[Eric Seidel (no email)]

Comment on attachment 109089 [details]
WIP

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

> Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:41
> +        self._tool.bugs.post_comment_to_bug(state['patch'].bug_id(), comment_text, cc_list)

I think you have to use the some sort of getter, and cant' grab at the state directly.  the patch might not be cached.

[Eric Seidel (no email)]

Comment on attachment 109089 [details]
WIP

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

> Tools/Scripts/webkitpy/tool/commands/download.py:255
> +    help_text = ""

I don't think you need to specify an empty help-text.

> Tools/Scripts/webkitpy/tool/commands/download.py:258
> +    prepare_steps = [
> +    ]

These aren't needed either.

>> Tools/Scripts/webkitpy/tool/commands/download.py:262
>> +    long_help = """ """
> 
> Better to have some long help explaining what this does.
> 
> Also, you should add a test.  Testing commands is very easy.  :)

testing commands is very easy. :)

>>> Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:33
>>> +class ApplyWatchListToPatch(AbstractStep):
>> 
>> expected 2 blank lines, found 1  [pep8/E302] [5]
> 
> The class name should also match the file name.

This should work from the current diff, which I think you get for free if you do the "patch" lookup from the state correctly.

> Tools/Scripts/webkitpy/tool/steps/processwatchlistforpatch.py:36
> +        watch_list = WatchListLoader(self._tool.filesystem).load()

Probably want this hung off the host (which is the tool).  modify host.py to have a watchlist function, which can lazy-load (and cache, via @memoized) the watchlist when accessed.  Similar to how CommitterList functions (or should function).

[Csaba Osztrogonác]

Comment on attachment 109089 [details]
WIP

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

r- due to my comment above

> Tools/Scripts/webkitpy/tool/steps/__init__.py:32
> +from webkitpy.tool.steps.applywatchlisttopatch import ApplyWatchListToPatch

There isn't applywatchlisttopatch named file, but processwatchlistforpatch.py :(
This change killed the Qt EWS. It wasn't simple to find the culprit. :)

[Csaba Osztrogonác]

new bug report filed on this issue: https://bugs.webkit.org/show_bug.cgi?id=69089

[David Levin]

(In reply to comment #6)
> (From update of attachment 109089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109089&action=review
> 
> r- due to my comment above
> 
> > Tools/Scripts/webkitpy/tool/steps/__init__.py:32
> > +from webkitpy.tool.steps.applywatchlisttopatch import ApplyWatchListToPatch
> 
> There isn't applywatchlisttopatch named file, but processwatchlistforpatch.py :(
> This change killed the Qt EWS. It wasn't simple to find the culprit. :)

Whoops. Sorry about that.

I knew the patch had issues and it was only a very early work in progress -- to get feedback on the direction being taken. I had simply put r? to make sure it got people's attention.

[David Levin]

Created attachment 109184 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 109184 [details]
Patch

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

I'm not sure I 100% follow.  I think most webkit-patch commands apply to the working directory by default.  It's interesting that you have a reversed default here, where the "normal" sounding command takes an attachment, and then there is a "local" variant.  I'm not sure if that makes sense for this command or not.  I may have to review this when less tired.  (That should not stop someone else from reviewing it first!)

> Tools/Scripts/webkitpy/tool/steps/applywatchlist.py:56
> +            log_result = _log.debug
> +        else:
> +            _log.info('No bug was updated because no id was given.')
> +            log_result = _log.info

Crazy how you're switching loggers...

[David Levin]

(In reply to comment #10)
> (From update of attachment 109184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109184&action=review
> 
> I'm not sure I 100% follow.  I think most webkit-patch commands apply to the working directory by default.  It's interesting that you have a reversed default here, where the "normal" sounding command takes an attachment, and then there is a "local" variant.  I'm not sure if that makes sense for this command or not.  I may have to review this when less tired.  (That should not stop someone else from reviewing it first!)

I think most commands have some intended local affect but a watchlist's main purpose (in my mind) is to add a note or cc to a bug.

However, I'm not wed to the current way I have things set up. I'm open to other ideas or discussing this more so we can figure out something better. (This just represents my current point of view.)

> 
> > Tools/Scripts/webkitpy/tool/steps/applywatchlist.py:56
> > +            log_result = _log.debug
> > +        else:
> > +            _log.info('No bug was updated because no id was given.')
> > +            log_result = _log.info
> 
> Crazy how you're switching loggers...

I'm not sure what you mean by that :). (I could do this using .log and switch logging level.)

The reason I do this is to let the user see the info if there is no bug being modified.

[Eric Seidel (no email)]

(In reply to comment #11)
> (In reply to comment #10)
> > Crazy how you're switching loggers...
> 
> I'm not sure what you mean by that :). (I could do this using .log and switch logging level.)
> 
> The reason I do this is to let the user see the info if there is no bug being modified.

It was more of a "you blew my mind" comment than a "you're crazy" comment. :)

[David Levin]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 109184 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109184&action=review
> > 
> > I'm not sure I 100% follow.  I think most webkit-patch commands apply to the working directory by default.  It's interesting that you have a reversed default here, where the "normal" sounding command takes an attachment, and then there is a "local" variant.  I'm not sure if that makes sense for this command or not.  I may have to review this when less tired.  (That should not stop someone else from reviewing it first!)
> 
> I think most commands have some intended local affect but a watchlist's main purpose (in my mind) is to add a note or cc to a bug.

I'm starting to see what you are saying.

Perhaps if I rename them, it will help: apply-watchlist-to-attachment and apply-watchlist

[Eric Seidel (no email)]

(In reply to comment #13)
> > I think most commands have some intended local affect but a watchlist's main purpose (in my mind) is to add a note or cc to a bug.
> 
> I'm starting to see what you are saying.
> 
> Perhaps if I rename them, it will help: apply-watchlist-to-attachment and apply-watchlist

I'm not sure I'm right.  I haven't looked at webkit-patch help --all-commands recently to see if there is a pattern.

[Eric Seidel (no email)]

Comment on attachment 109184 [details]
Patch

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

whappam!

> Tools/Scripts/webkitpy/tool/commands/applywatchlistlocal.py:35
> +    help_text = "Applies the watchlist to local changes."

I don't remember if we use periods/capitals or not in these.  I remember tryign to be consistent with other unix commands at one time, but I don't remember what I decided on. :)

[David Levin]

Committed as http://trac.webkit.org/changeset/96444.

(In reply to comment #15)
> (From update of attachment 109184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109184&action=review
> 
> whappam!
> 
> > Tools/Scripts/webkitpy/tool/commands/applywatchlistlocal.py:35
> > +    help_text = "Applies the watchlist to local changes."
> 
> I don't remember if we use periods/capitals or not in these.  I remember tryign to be consistent with other unix commands at one time, but I don't remember what I decided on. :)

Sorry. In my excitement, I forgot to check this. I'll look it over and submit another patch if what I did is inconsistent (which I suspect it may be).

[Eric Seidel (no email)]

(In reply to comment #16)
> Committed as http://trac.webkit.org/changeset/96444.
> 
> (In reply to comment #15)
> > I don't remember if we use periods/capitals or not in these.  I remember tryign to be consistent with other unix commands at one time, but I don't remember what I decided on. :)
> 
> Sorry. In my excitement, I forgot to check this. I'll look it over and submit another patch if what I did is inconsistent (which I suspect it may be).

On the list of fish to fry, that's a minnow.