[webkit-dev] style-queue entering beta

Sat Nov 28 10:21:00 PST 2009

Sounds like a good idea in general. Some comments below:

On Nov 28, 2009, at 2:21 AM, Adam Barth wrote:

> One of the bots that Eric and I have been working on is about to come
> online.  This bot is a "style bot" that runs check-webkit-style on
> patches that have been nominated for review.  I'd like to ask your
> patience while we work out the kinks.
>
> You can skip the rest of this email if you're not interested in the
> nitty gritty.
>
> == Summary ==
>
> The primary goal of the style-queue is to reduce review latency by (1)
> giving immediate feedback to contributors and (2) making human
> reviewer more efficient by relieving them of mechanical tasks (like
> asking for tabs to be replaced with spaces).
>
> The style-queue scans the set of patches that are pending-review for
> new patches.  When it finds a new patch, the bot runs
> check-webkit-style on the patch.  If check-webkit-style finds style
> errors, the bot comments on the bug.
>
> == Social Contract ==
>
> The style-queue is purely informative.  You're free to ignore it's
> comments.  Ideally, the bot should be silent unless it has something
> interesting to say.  If we decide that style-queue is too chatty, we
> can change check-webkit-style to report fewer errors.
>
> One corollary of this social contract is that the style-queue doesn't
> inform you when your patch passes the style check (because most
> patches should pass, right?).  We can re-visit this design choice
> later if it turns out we really want to know when the check succeeds.

To actually relieve reviewers of the burden of manual style review, it  
seems like the bot would have to record when a patch passes the style  
check. Otherwise, how can you tell whether a patch has passed the  
check, or if the bot just hasn't run on it yet? Would anyone object to  
seeing the information that a patch has passed an automated style  
check in a comment?

>
> == Response to Previous Feedback ==
>
> When I floated this idea on webkit-dev previously (in the "bot-filled
> future" thread), I got a bunch of useful feedback.  Here's how we
> incorporated that feedback into the current design:
>
> 1) "Adding an extra flags is going to cause confusion."  The
> style-queue does not add any flags to Bugzilla.  Instead of storing
> it's state in Bugzilla flags (like commit-queue does), we built an
> AppEngine web service to hold the queue's persistent state.  Instead
> of indicating style errors with a negative flag, the bot adds a
> comment to the bug.

It does seem that flags are noisy in an unappealing way, but it would  
be much better (IMO) to store this information in the bugzilla  
database instead of externally. Is there any way we can do that? Could  
we make a flag that is hidden in the default UI, or use specially  
formatted comments that the bot knows how to recognize?

>
> 2) "What machines are going to be doing these tests, and on which
> platforms?"  The style-queue runs on a machine in my apartment that
> runs Mac OS X.  The style-queue is platform independent, so this is
> less of an issue for style-queue than it is for a build-queue.
>
> 3) "Which patches would this test?"  The style-queue tests any patch
> marked "review?".  This design avoids the need for additional flags or
> indications that a patch should be run through the queue.
>
> 4) "Running tests on an arbitrary patches [opens a security hole]."
> Instead of running check-webkit-style from it's own working copy, the
> style-queue runs check-webkit-style from a separate working copy.
> While it's true you could haxor my machine by committing an evil copy
> of check-webkit-style, I'm already running that risk whenever I type
> "svn up".
>
> == Frequently Asked Questions ==
>
> Q1: If the style-queue doesn't complain, does that mean my patch has
> correct style?
>
> A1: Unfortunately, no.  First of all, check-webkit-style has false
> negatives.  Hopefully, the script will improve over time, but it will
> never be perfect.  Second, the style-queue is only able to check
> patches that successfully download and apply to top-of-tree.  If your
> patch does not apply to top-of-tree (e.g., because it depends on
> another patch), then style-queue won't say anything.
>
> Q2: How can I see whether style-queue processed my patch?
>
> A2: There isn't a great way to do this yet.  The data is stored in the
> webkit-commit-queue.appspot.com web service, but we haven't built a
> front-end for it yet.  If you have a strong stomach, you can look at
> the endpoint the bot itself uses
>
> http://webkit-commit-queue.appspot.com/patch-status/style-queue/12345
>
> where 12345 is the ID of your attachment.  If you'd like to have a
> better status display, feel free to hack on
> WebKitTools/QueueStatusServer and send me or Eric patches to review.
> In the long term, there's no reason we can't have an awesome UI.
>
> Q3: This bot is going to run amock!  How can I keep track of all it's
> evil deeds?
>
> A3: If you want to see what the bots are doing, you can subscribe to
> the following email list:
>
> http://groups.google.com/group/webkit-bot-watchers
>
> Whenever the bots touch a bug, they're supposed to add this address to
> the CC list.  This functionality is very new, so we might not have
> every case covered yet.  If you see the bot misbehaving, let me or
> Eric know.  We'll correct the issue ASAP.
>
> Q4: Checking style is fine and dandy, but I was hoping for a build- 
> queue!
>
> A4: My next project is to get a build-queue working for Qt.  The
> build-queue will share a lot of infrastructure with the style-queue,
> but making the style-queue was easier because check-webkit-style is a
> lot faster than build-webkit.  :)
>
> Q5: When are you flipping the switch?
>
> A5: Either today or tomorrow, depending on when I find a stretch of
> time to babysit the queue through whatever initial troubles it has.
> My working copy of the style-queue is about five patches ahead of
> trunk.  It would be nice to have those patches committed before
> flipping the switch, but it's not necessary.
>
> Thanks for reading to the end, and I hope you find the style-queue  
> useful.
>
> Adam
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev