I've been experimenting with a wrapper for bugzilla-tool that runs in a loop and tries to land patches [1]. Ideally, there would be enough information in Bugzilla so I could kick off this script and make tea while it does its thing. However, deciding whether a bug with an r+ patch is ready to be landed requires human intervention. I went through the queue tonight and manually marked the bugs as commit-ready by adding [commit+] to their titles. This worked well, except that it made the bug titles ugly. I think a better solution is to have flag analogous to r+ which is commit+. The flag means "I certify that this patch is ready to be landed according to WebKit process." Unlike r+, this flag would be settable by any committer because it is the moral equivalent of typing "svn commit". In rare cases, such as unreviewed build fixes, a patch could have commit+ without an r+. In future, future land when we have a working commit queue, the actual "svn commit" command might be run by a bot instead of a human. Adam [1] You can see my progress here: https://bugs.webkit.org/show_bug.cgi?id=27918
I went through the queue tonight and manually marked the bugs as commit-ready by adding [commit+] to their titles. This worked well, except that it made the bug titles ugly. I think a better solution is to have flag analogous to r+ which is commit+.
Adam, as I suggedted previously, bugzilla supports KEYWORDs, so that would be a matter of adding a special support for bugs where patches are ready to go in. 'checkin-needed' keyword would work , i believe. What do you think ? these are the currently supported keywords: https://bugs.webkit.org/describekeywords.cgi -- --Antonio Gomes
On Sat, Aug 1, 2009 at 11:43 AM, tonikitoo (Antonio Gomes) < tonikitoo@gmail.com> wrote:
I went through the queue tonight and manually marked the bugs as commit-ready by adding [commit+] to their titles. This worked well, except that it made the bug titles ugly. I think a better solution is to have flag analogous to r+ which is commit+.
Adam, as I suggedted previously, bugzilla supports KEYWORDs, so that would be a matter of adding a special support for bugs where patches are ready to go in.
Can keywords be applied to individual patches? This needs to work for bugs with multiple patches on them. Ojan
On Sat, Aug 1, 2009 at 8:43 AM, tonikitoo (Antonio Gomes)<tonikitoo@gmail.com> wrote:
Adam, as I suggedted previously, bugzilla supports KEYWORDs, so that would be a matter of adding a special support for bugs where patches are ready to go in.
'checkin-needed' keyword would work , i believe. What do you think ?
In addition to the point Ojan made about applying to individual patches, a flag also has the advantage that it implicates who approved the patch to be committed. Sure, the information is available for keywords if you dig deep enough, but the system has more accountability if the name is right there. Adam
On Saturday, August 1, 2009 10:16:39 AM, Adam Barth wrote:
On Sat, Aug 1, 2009 at 8:43 AM, tonikitoo (Antonio Gomes)wrote:
Adam, as I suggedted previously, bugzilla supports KEYWORDs, so that would be a matter of adding a special support for bugs where patches are ready to go in.
'checkin-needed' keyword would work , i believe. What do you think ?
In addition to the point Ojan made about applying to individual patches, a flag also has the advantage that it implicates who approved the patch to be committed. Sure, the information is available for keywords if you dig deep enough, but the system has more accountability if the name is right there.
Bugzilla has the ability to create additional 4-state flags at both the attachment level and at the bug level. (Note that bugs.webkit.org does not have bug-level flags enabled.) For example, we could create a "commit" attachment flag which would have four states (just like the "review" flag): <none>, commit?, commit+, commit-. I'm not sure if this helps or hurts, though it would have made abarth's desired workflow much nicer. Mozilla also uses a "superreview" flag and various "approval" flags as seen on the attachments on this bug: <https://bugzilla.mozilla.org/show_bug.cgi?id=342677> Dave
On Sat, Aug 1, 2009 at 11:04 AM, David Kilzer<ddkilzer@webkit.org> wrote:
Bugzilla has the ability to create additional 4-state flags at both the attachment level and at the bug level. (Note that bugs.webkit.org does not have bug-level flags enabled.)
For example, we could create a "commit" attachment flag which would have four states (just like the "review" flag): <none>, commit?, commit+, commit-.
I'm not sure if this helps or hurts, though it would have made abarth's desired workflow much nicer.
Yeah, that sounds great. Should we try that out for a while and see if it's useful? Adam
On Sat, Aug 1, 2009 at 2:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Sat, Aug 1, 2009 at 11:04 AM, David Kilzer<ddkilzer@webkit.org> wrote:
Bugzilla has the ability to create additional 4-state flags at both the attachment level and at the bug level. (Note that bugs.webkit.org does not have bug-level flags enabled.)
For example, we could create a "commit" attachment flag which would have four states (just like the "review" flag): <none>, commit?, commit+, commit-.
I'm not sure if this helps or hurts, though it would have made abarth's desired workflow much nicer.
Yeah, that sounds great. Should we try that out for a while and see if it's useful?
This seems fine to me, except it adds yet another layer of complexity to the review process. I think getting patches landed more quickly is probably worth it though. This is essentially a slightly more complicated (but easier to implement?) version of Maciej's proposal from a few weeks ago to get rid of r* and have the following four review states: REQUESTED DENIED APPROVED WITH MODIFICATIONS APPROVED One benefit over the above is that, I don't think we need to restrict commit+ to people with svn commit bit, as long as a patch is r+'ed or r+'ed with modifications. Ojan
On Sat, Aug 1, 2009 at 11:45 AM, Ojan Vafai <ojan@chromium.org> wrote:
On Sat, Aug 1, 2009 at 2:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Sat, Aug 1, 2009 at 11:04 AM, David Kilzer<ddkilzer@webkit.org> wrote:
Bugzilla has the ability to create additional 4-state flags at both the attachment level and at the bug level. (Note that bugs.webkit.org does not have bug-level flags enabled.)
For example, we could create a "commit" attachment flag which would have four states (just like the "review" flag): <none>, commit?, commit+, commit-.
I'm not sure if this helps or hurts, though it would have made abarth's desired workflow much nicer.
Yeah, that sounds great. Should we try that out for a while and see if it's useful?
This seems fine to me, except it adds yet another layer of complexity to the review process. I think getting patches landed more quickly is probably worth it though. This is essentially a slightly more complicated (but easier to implement?) version of Maciej's proposal from a few weeks ago to get rid of r* and have the following four review states:
REQUESTED DENIED APPROVED WITH MODIFICATIONS APPROVED
It seems to me that the only difference is APPROVED becomes READY TO LAND (or LANDABLE or something like that). Btw, I see one downside to a commit queue: When you manually commit something, you're supposed to watch the build bots for breakage. If the submit queue is running all the tests on all the platforms then it doesn't really matter, but if not there'll need to be automated rollbacks when things break on the waterfall build bots. This could easily become complicated if the commit queue can submit multiple patches before the waterfall bots show green for the previous patch. I guess the point that I'm trying to make is that tests should either be run on every buildbot configuration, there needs to be some pretty robust rollback code, or the queue needs to let everything cycle green before committing the next patch. The last suggestion seems like a good start. Note also that, if the system rolls back a patch and the bots don't go back to their previous state, the submit bot needs to stop so someone can take a look manually (so things don't get further hosed). One benefit over the above is that, I don't think we need to restrict
commit+ to people with svn commit bit, as long as a patch is r+'ed or r+'ed with modifications.
Ojan
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Sat, Aug 1, 2009 at 12:53 PM, Jeremy Orlow<jorlow@chromium.org> wrote:
Btw, I see one downside to a commit queue: When you manually commit something, you're supposed to watch the build bots for breakage. If the submit queue is running all the tests on all the platforms then it doesn't really matter, but if not there'll need to be automated rollbacks when things break on the waterfall build bots. This could easily become complicated if the commit queue can submit multiple patches before the waterfall bots show green for the previous patch.
We're a ways from having a fully automated commit queue. When I run the commit-queue script, I also watch the bots and clean up manually as needed. (So far this hasn't been necessary, but I'm sure it will be at some point.) Adam
On Saturday, August 1, 2009 11:45:43 AM, Ojan Vafai wrote:
On Sat, Aug 1, 2009 at 2:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Sat, Aug 1, 2009 at 11:04 AM, David Kilzer<ddkilzer@webkit.org> wrote:
Bugzilla has the ability to create additional 4-state flags at both the attachment level and at the bug level. (Note that bugs.webkit.org does not have bug-level flags enabled.)
For example, we could create a "commit" attachment flag which would have four states (just like the "review" flag): <none>, commit?, commit+, commit-.
I'm not sure if this helps or hurts, though it would have made abarth's desired workflow much nicer.
Yeah, that sounds great. Should we try that out for a while and see if it's useful?
This seems fine to me, except it adds yet another layer of complexity to the review process. I think getting patches landed more quickly is probably worth it though. This is essentially a slightly more complicated (but easier to implement?) version of Maciej's proposal from a few weeks ago to get rid of r* and have the following four review states:
REQUESTED DENIED APPROVED WITH MODIFICATIONS APPROVED
One benefit over the above is that, I don't think we need to restrict commit+ to people with svn commit bit, as long as a patch is r+'ed or r+'ed with modifications.
This would require a non-trivial amount of modification to Bugzilla, and it would make future upgrades much more difficult. Basically, Adam's asking for a flag to make it easier for bugzilla-tool to know when to land patches without human intervention. Currently, a committer must look at each patch's review comments to make this determination. Either we should change the review process to only set the review+ flag if the patch is ready to go with zero modifications, or we should use the commit+ flag to signify that. I could go either way on this. I don't like the idea of setting review- flags for trivial fixes that could be landed with minor modifications. On the other hand, being able to commit patches directly from bugs.webkit.org with bugzilla-tool is really handy. Dave
On Sat, Aug 1, 2009 at 1:13 PM, David Kilzer<ddkilzer@webkit.org> wrote:
Either we should change the review process to only set the review+ flag if the patch is ready to go with zero modifications, or we should use the commit+ flag to signify that.
I could go either way on this. I don't like the idea of setting review- flags for trivial fixes that could be landed with minor modifications. On the other hand, being able to commit patches directly from bugs.webkit.org with bugzilla-tool is really handy.
The other factor is that some committers might not want the commit-queue script to land their changes. In those cases, their patches would get an r+ but never get a commit+. (Personally, I don't mind if other people want to land may changes, but I can see how it might disrupt other's workflow.) Adam
Adam Barth wrote:
On Sat, Aug 1, 2009 at 1:13 PM, David Kilzer<ddkilzer@webkit.org> wrote:
Either we should change the review process to only set the review+ flag if the patch is ready to go with zero modifications, or we should use the commit+ flag to signify that.
I could go either way on this. I don't like the idea of setting review- flags for trivial fixes that could be landed with minor modifications. On the other hand, being able to commit patches directly from bugs.webkit.org with bugzilla-tool is really handy.
The other factor is that some committers might not want the commit-queue script to land their changes. In those cases, their patches would get an r+ but never get a commit+. (Personally, I don't mind if other people want to land may changes, but I can see how it might disrupt other's workflow.)
Based on this, I suggest calling the flag "auto-commit", to make it clear that you don't need to set it if you want to commit by hand. Joe
On Aug 1, 2009, at 8:41 PM, Adam Barth wrote:
On Sat, Aug 1, 2009 at 1:13 PM, David Kilzer<ddkilzer@webkit.org> wrote:
Either we should change the review process to only set the review+ flag if the patch is ready to go with zero modifications, or we should use the commit+ flag to signify that.
I could go either way on this. I don't like the idea of setting review- flags for trivial fixes that could be landed with minor modifications. On the other hand, being able to commit patches directly from bugs.webkit.org with bugzilla-tool is really handy.
The other factor is that some committers might not want the commit-queue script to land their changes. In those cases, their patches would get an r+ but never get a commit+. (Personally, I don't mind if other people want to land may changes, but I can see how it might disrupt other's workflow.
I think the flag state should be "commit?" for requesting a commit, and "commit+" for a committed patch, to match how we use the review flag. That way, either the patch submitter or anyone else who notices the patch needs committing and wasn't submitted by a committer can flag it, and the state when the patch is landed can be clear. Regards, Maciej
OK, per the discussion, I will add a commit-queue=? flag for Adam's testing. If we like it, we can keep it. If not, we can kill it. -eric On Mon, Aug 3, 2009 at 9:43 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Aug 1, 2009, at 8:41 PM, Adam Barth wrote:
On Sat, Aug 1, 2009 at 1:13 PM, David Kilzer<ddkilzer@webkit.org> wrote:
Either we should change the review process to only set the review+ flag if the patch is ready to go with zero modifications, or we should use the commit+ flag to signify that.
I could go either way on this. I don't like the idea of setting review- flags for trivial fixes that could be landed with minor modifications. On the other hand, being able to commit patches directly from bugs.webkit.org with bugzilla-tool is really handy.
The other factor is that some committers might not want the commit-queue script to land their changes. In those cases, their patches would get an r+ but never get a commit+. (Personally, I don't mind if other people want to land may changes, but I can see how it might disrupt other's workflow.
I think the flag state should be "commit?" for requesting a commit, and "commit+" for a committed patch, to match how we use the review flag. That way, either the patch submitter or anyone else who notices the patch needs committing and wasn't submitted by a committer can flag it, and the state when the patch is landed can be clear.
Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
participants (8)
-
Adam Barth
-
David Kilzer
-
Eric Seidel
-
Jeremy Orlow
-
Joe Mason
-
Maciej Stachowiak
-
Ojan Vafai
-
tonikitoo (Antonio Gomes)