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@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
