Add safely-roll-webkit.py to streamline webkit rolls.

Add safely-roll-webkit.py to streamline webkit rolls.

It still needs some work on the commit queue side to work correctly, at the
moment it will deny the commit because there is no LGTM.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93945

[M-A Ruel, 2011-05-30 19:21:58]

Example command line:

~/src/chrome/src/tools> ./safely-roll-webkit.py 87696
Roll webkit revision to 87696
Current branch 1g_commit_queue is up to date.
Loaded authentication cookies from /home/maruel/.codereview_upload_cookies
Running presubmit upload checks ...

Presubmit checks passed.
 DEPS |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Upload server: http://codereview.chromium.org (change with -s/--server)
Loaded authentication cookies from /home/maruel/.codereview_upload_cookies
Issue created. URL: http://codereview.chromium.org/7089011
Uploading base file for DEPS
~/src/chrome/src/tools>

[M-A Ruel, 2011-05-30 19:25:41]

Note this change requires http://codereview.chromium.org/7084037/ first.

[abarth-chromium, 2011-05-30 19:28:56]

Looks cool.

[M-A Ruel, 2011-05-30 20:47:46]

Dirk, do you mind reviewing that? I wasn't sure which person was the best to
review this part but since you also do webkit stuff, you're probably the best
placed.

It's a small script to create a new git branch to roll webkit. Right now it
doesn't work because the commit queue must special case this kind of change to
not enforce lgtm but I'd still like to commit this change right now.

[Dirk Pranke, 2011-05-30 21:23:24]

The patch looks pretty good. I think it will be useful for one particular way of
doing webkit rolls, and maybe we should encourage people to follow that one way.

Two comments on how I do things differently when gardening that may or may not
apply:

1) It should usually, if not always, be the case that
webkit/tools/layout_tests/test_expectations.txt should be empty when a roll is
done (any existing lines should've been added to the upstream file in a previous
webkit change). It's okay for the lines to exist in both files simultaneously on
the canaries, but this shouldn't happen on the DEPS bots. 

So, it might make sense to add some sort of check here for that case.

2) I don't usually expect someone to review the roll prior to it being
committed; at best I might mark the change as TBR, but generally I don't expect
anyone to review this at all. Perhaps this is me being lazy or sloppy, but I
believe the way this is coded, the change won't commit until someone LGTM's it,
right?

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
File tools/safely-roll-webkit.py (right):

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:9: """
This comment isn't that helpful. I would replace it with something like:

Generate a CL to roll webkit to the specified revision number and post it to
Rietveld so that the CL will land automatically if it passes the commit-queue's
checks.

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",' %
new_rev
Hm. I'd probably do this using readlines() for line-at-a-time parsing and then
match against and update a single line. 

At the very least, I'd want to explicitly match against the beginning of the
line using ^\s+ instead of the '  ' that you have now (and also match against
the end of the line using $).

Your code could get confused (I think) by having comments elsewhere in the DEPS
file that refer to "webkit_revision: 3"  or something else like that.

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:76: subprocess2.check_output(['git', 'branch', '-D',
'webkit_roll'])
I personally don't like to delete branches until after the change has landed. I
wonder if it makes sense to provide for that, or (more generally) a way to break
this up into each step?

On the other hand, making this extremely simple and idiot-proof is probably a
good thing for now. You can always make it more flexible later.

[abarth-chromium, 2011-05-30 21:31:41]

On Mon, May 30, 2011 at 2:23 PM,  <dpranke@chromium.org> wrote:
> The patch looks pretty good. I think it will be useful for one particular
> way of
> doing webkit rolls, and maybe we should encourage people to follow that one
> way.
>
> Two comments on how I do things differently when gardening that may or may
> not
> apply:
>
> 1) It should usually, if not always, be the case that
> webkit/tools/layout_tests/test_expectations.txt should be empty when a roll
> is
> done (any existing lines should've been added to the upstream file in a
> previous
> webkit change). It's okay for the lines to exist in both files
> simultaneously on
> the canaries, but this shouldn't happen on the DEPS bots.
>
> So, it might make sense to add some sort of check here for that case.

Please don't add that check.  We're probably going to change the
semantics of the downstream test_expectations.txt file as part of
re-tooling gardening and I'd prefer if we didn't couple it with this
part of the system.

> 2) I don't usually expect someone to review the roll prior to it being
> committed; at best I might mark the change as TBR, but generally I don't
> expect
> anyone to review this at all. Perhaps this is me being lazy or sloppy, but I
> believe the way this is coded, the change won't commit until someone LGTM's
> it,
> right?
>
>
> http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
> File tools/safely-roll-webkit.py (right):
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:9: """
> This comment isn't that helpful. I would replace it with something like:
>
> Generate a CL to roll webkit to the specified revision number and post
> it to Rietveld so that the CL will land automatically if it passes the
> commit-queue's checks.
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",'
> % new_rev
> Hm. I'd probably do this using readlines() for line-at-a-time parsing
> and then match against and update a single line.
>
> At the very least, I'd want to explicitly match against the beginning of
> the line using ^\s+ instead of the '  ' that you have now (and also
> match against the end of the line using $).
>
> Your code could get confused (I think) by having comments elsewhere in
> the DEPS file that refer to "webkit_revision: 3"  or something else like
> that.
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:76: subprocess2.check_output(['git',
> 'branch', '-D', 'webkit_roll'])
> I personally don't like to delete branches until after the change has
> landed. I wonder if it makes sense to provide for that, or (more
> generally) a way to break this up into each step?
>
> On the other hand, making this extremely simple and idiot-proof is
> probably a good thing for now. You can always make it more flexible
> later.
>
> http://codereview.chromium.org/7089012/
>

[Dirk Pranke, 2011-05-30 21:48:19]

On Mon, May 30, 2011 at 2:31 PM, Adam Barth <abarth@chromium.org> wrote:
> On Mon, May 30, 2011 at 2:23 PM,  <dpranke@chromium.org> wrote:
>> The patch looks pretty good. I think it will be useful for one particular
>> way of
>> doing webkit rolls, and maybe we should encourage people to follow that one
>> way.
>>
>> Two comments on how I do things differently when gardening that may or may
>> not
>> apply:
>>
>> 1) It should usually, if not always, be the case that
>> webkit/tools/layout_tests/test_expectations.txt should be empty when a roll
>> is
>> done (any existing lines should've been added to the upstream file in a
>> previous
>> webkit change). It's okay for the lines to exist in both files
>> simultaneously on
>> the canaries, but this shouldn't happen on the DEPS bots.
>>
>> So, it might make sense to add some sort of check here for that case.
>
> Please don't add that check.  We're probably going to change the
> semantics of the downstream test_expectations.txt file as part of
> re-tooling gardening and I'd prefer if we didn't couple it with this
> part of the system.
>

Hm. A counterargument would be that, today, if you do a webkit roll
and you don't zero-out that file, you're doing it wrong (*). So, it
might make sense to actually have the tool do the right thing today,
and then just change the tool when you change the process.

But, that's just me. As noted for the other issues in the bug, this
script doesn't really match the workflow I use for doing webkit rolls,
so I'm not likely to use it at the moment, but that shouldn't stop
other people from using it.

Another general comment I'll make (unrelated to this patch in
particular, and apologies for side-tracking the code review; this is
probably deserving of a forked thread) is that the idea of using the
commit queue for webkit rolls makes me a little nervous. On the one
hand, it should be a good thing, because it'll enforce the requirement
that people have actually done try jobs.

On the other, especially given the current state of the tooling around
the chromium commit queue, there is no telling when a change marked
for the commit queue will actually land (and there's no guarantee that
the try jobs that passed actually reflect the current state of the
tree, although hopefully most of the time it'll be pretty close).

We've seen both in chromium-land and webkit-land that the commit queue
can make it easier for the committer to upload something and then walk
away instead of making sure the patch lands cleanly. That would be
particularly bad for webkit rolls, so hopefully it won't happen in
practice, but again, lack of any insight as to when a patch might make
it through the queue won't help.

Please don't take this as me being against commit queues in general or
our queues in particular. I do think the good outweighs the bad and we
can generally fix the bad over time, it's just good to be aware of
what the bad is.

-- Dirk

(*) I consider not zeroing out the file as "wrong" because other
people and tools are used to only looking at the upstream version of
the file, and so it's very easy to be confused by suppressions that
only exist in the downstream file (I get questions about this fairly
frequently). I view the downstream file as a hack that should only be
used in rare cases; unfortunately, the tests (and code) are too flaky
today for this to usually be true. Of course, if we change the process
so that we start using the downstream file differently, this comment
will no longer apply.


>> 2) I don't usually expect someone to review the roll prior to it being
>> committed; at best I might mark the change as TBR, but generally I don't
>> expect
>> anyone to review this at all. Perhaps this is me being lazy or sloppy, but I
>> believe the way this is coded, the change won't commit until someone LGTM's
>> it,
>> right?
>>
>>
>> http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
>> File tools/safely-roll-webkit.py (right):
>>
>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>> tools/safely-roll-webkit.py:9: """
>> This comment isn't that helpful. I would replace it with something like:
>>
>> Generate a CL to roll webkit to the specified revision number and post
>> it to Rietveld so that the CL will land automatically if it passes the
>> commit-queue's checks.
>>
>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>> tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",'
>> % new_rev
>> Hm. I'd probably do this using readlines() for line-at-a-time parsing
>> and then match against and update a single line.
>>
>> At the very least, I'd want to explicitly match against the beginning of
>> the line using ^\s+ instead of the '  ' that you have now (and also
>> match against the end of the line using $).
>>
>> Your code could get confused (I think) by having comments elsewhere in
>> the DEPS file that refer to "webkit_revision: 3"  or something else like
>> that.
>>
>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>> tools/safely-roll-webkit.py:76: subprocess2.check_output(['git',
>> 'branch', '-D', 'webkit_roll'])
>> I personally don't like to delete branches until after the change has
>> landed. I wonder if it makes sense to provide for that, or (more
>> generally) a way to break this up into each step?
>>
>> On the other hand, making this extremely simple and idiot-proof is
>> probably a good thing for now. You can always make it more flexible
>> later.
>>
>> http://codereview.chromium.org/7089012/
>>
>

[abarth-chromium, 2011-05-30 22:19:56]

On Mon, May 30, 2011 at 2:47 PM, Dirk Pranke <dpranke@chromium.org> wrote:
> On Mon, May 30, 2011 at 2:31 PM, Adam Barth <abarth@chromium.org> wrote:
>> On Mon, May 30, 2011 at 2:23 PM,  <dpranke@chromium.org> wrote:
>>> The patch looks pretty good. I think it will be useful for one particular
>>> way of
>>> doing webkit rolls, and maybe we should encourage people to follow that one
>>> way.
>>>
>>> Two comments on how I do things differently when gardening that may or may
>>> not
>>> apply:
>>>
>>> 1) It should usually, if not always, be the case that
>>> webkit/tools/layout_tests/test_expectations.txt should be empty when a roll
>>> is
>>> done (any existing lines should've been added to the upstream file in a
>>> previous
>>> webkit change). It's okay for the lines to exist in both files
>>> simultaneously on
>>> the canaries, but this shouldn't happen on the DEPS bots.
>>>
>>> So, it might make sense to add some sort of check here for that case.
>>
>> Please don't add that check.  We're probably going to change the
>> semantics of the downstream test_expectations.txt file as part of
>> re-tooling gardening and I'd prefer if we didn't couple it with this
>> part of the system.
>>
>
> Hm. A counterargument would be that, today, if you do a webkit roll
> and you don't zero-out that file, you're doing it wrong (*). So, it
> might make sense to actually have the tool do the right thing today,
> and then just change the tool when you change the process.
>
> But, that's just me. As noted for the other issues in the bug, this
> script doesn't really match the workflow I use for doing webkit rolls,
> so I'm not likely to use it at the moment, but that shouldn't stop
> other people from using it.
>
> Another general comment I'll make (unrelated to this patch in
> particular, and apologies for side-tracking the code review; this is
> probably deserving of a forked thread) is that the idea of using the
> commit queue for webkit rolls makes me a little nervous. On the one
> hand, it should be a good thing, because it'll enforce the requirement
> that people have actually done try jobs.
>
> On the other, especially given the current state of the tooling around
> the chromium commit queue, there is no telling when a change marked
> for the commit queue will actually land (and there's no guarantee that
> the try jobs that passed actually reflect the current state of the
> tree, although hopefully most of the time it'll be pretty close).
>
> We've seen both in chromium-land and webkit-land that the commit queue
> can make it easier for the committer to upload something and then walk
> away instead of making sure the patch lands cleanly. That would be
> particularly bad for webkit rolls, so hopefully it won't happen in
> practice, but again, lack of any insight as to when a patch might make
> it through the queue won't help.
>
> Please don't take this as me being against commit queues in general or
> our queues in particular. I do think the good outweighs the bad and we
> can generally fix the bad over time, it's just good to be aware of
> what the bad is.

I'm not too worried about that problem given that we have basically
round-the-clock staffing for WebKit gardening and rolling back DEPS
changes is easy if all else fails.

Adam


> (*) I consider not zeroing out the file as "wrong" because other
> people and tools are used to only looking at the upstream version of
> the file, and so it's very easy to be confused by suppressions that
> only exist in the downstream file (I get questions about this fairly
> frequently). I view the downstream file as a hack that should only be
> used in rare cases; unfortunately, the tests (and code) are too flaky
> today for this to usually be true. Of course, if we change the process
> so that we start using the downstream file differently, this comment
> will no longer apply.
>
>
>>> 2) I don't usually expect someone to review the roll prior to it being
>>> committed; at best I might mark the change as TBR, but generally I don't
>>> expect
>>> anyone to review this at all. Perhaps this is me being lazy or sloppy, but I
>>> believe the way this is coded, the change won't commit until someone LGTM's
>>> it,
>>> right?
>>>
>>>
>>> http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
>>> File tools/safely-roll-webkit.py (right):
>>>
>>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>>> tools/safely-roll-webkit.py:9: """
>>> This comment isn't that helpful. I would replace it with something like:
>>>
>>> Generate a CL to roll webkit to the specified revision number and post
>>> it to Rietveld so that the CL will land automatically if it passes the
>>> commit-queue's checks.
>>>
>>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>>> tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",'
>>> % new_rev
>>> Hm. I'd probably do this using readlines() for line-at-a-time parsing
>>> and then match against and update a single line.
>>>
>>> At the very least, I'd want to explicitly match against the beginning of
>>> the line using ^\s+ instead of the '  ' that you have now (and also
>>> match against the end of the line using $).
>>>
>>> Your code could get confused (I think) by having comments elsewhere in
>>> the DEPS file that refer to "webkit_revision: 3"  or something else like
>>> that.
>>>
>>>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
>>> tools/safely-roll-webkit.py:76: subprocess2.check_output(['git',
>>> 'branch', '-D', 'webkit_roll'])
>>> I personally don't like to delete branches until after the change has
>>> landed. I wonder if it makes sense to provide for that, or (more
>>> generally) a way to break this up into each step?
>>>
>>> On the other hand, making this extremely simple and idiot-proof is
>>> probably a good thing for now. You can always make it more flexible
>>> later.
>>>
>>> http://codereview.chromium.org/7089012/
>>>
>>
>

[M-A Ruel, 2011-05-31 01:21:07]

I removed the check for only DEPS being modified since it doesn't make sense.

I wanted to push this script to give an idea of the workflow and once you guys
settle for something, I'll add the commit queue side to not require any review
*and* send try jobs to *_layout try bots, which will mean they need to get in
shape too.

So I don't have any opinion on the script as is, it was only to give an idea
about how to automate that part to be able to use the commit queue for webkit
roll. In any case, it's not useful yet since it requires a review so we can put
this review on hold too if you prefer, as long as you think it's a good way to
go.

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
File tools/safely-roll-webkit.py (right):

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:9: """
On 2011/05/30 21:23:24, Dirk Pranke wrote:
> This comment isn't that helpful. I would replace it with something like:
> 
> Generate a CL to roll webkit to the specified revision number and post it to
> Rietveld so that the CL will land automatically if it passes the
commit-queue's
> checks.
> 

done

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",' %
new_rev
On 2011/05/30 21:23:24, Dirk Pranke wrote:
> Hm. I'd probably do this using readlines() for line-at-a-time parsing and then
> match against and update a single line. 
> 
> At the very least, I'd want to explicitly match against the beginning of the
> line using ^\s+ instead of the '  ' that you have now (and also match against
> the end of the line using $).
> 
> Your code could get confused (I think) by having comments elsewhere in the
DEPS
> file that refer to "webkit_revision: 3"  or something else like that.

I prefer to keep the dumb logic here and change it if needed. I added count=1 so
only one replacement is done. I changed the code below to print the diff so it
will pause if the diff is too long.

http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
tools/safely-roll-webkit.py:76: subprocess2.check_output(['git', 'branch', '-D',
'webkit_roll'])
On 2011/05/30 21:23:24, Dirk Pranke wrote:
> I personally don't like to delete branches until after the change has landed.
I
> wonder if it makes sense to provide for that, or (more generally) a way to
break
> this up into each step?
> 
> On the other hand, making this extremely simple and idiot-proof is probably a
> good thing for now. You can always make it more flexible later.

By using check_call(git checkout -b) before the try block, it will return 1 if
the branch already existed or if uncommitted files would be touched so this is
quite safe. Also, the branch is directly done against upstream and not the
current branch.

[abarth-chromium, 2011-05-31 05:34:08]

Yeah, I agree that we'll probably want to iterate on this once more of
the workflow is sketched out.  Thanks!

Adam


On Mon, May 30, 2011 at 6:21 PM,  <maruel@chromium.org> wrote:
> I removed the check for only DEPS being modified since it doesn't make
> sense.
>
> I wanted to push this script to give an idea of the workflow and once you
> guys
> settle for something, I'll add the commit queue side to not require any
> review
> *and* send try jobs to *_layout try bots, which will mean they need to get
> in
> shape too.
>
> So I don't have any opinion on the script as is, it was only to give an idea
> about how to automate that part to be able to use the commit queue for
> webkit
> roll. In any case, it's not useful yet since it requires a review so we can
> put
> this review on hold too if you prefer, as long as you think it's a good way
> to
> go.
>
>
> http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py
> File tools/safely-roll-webkit.py (right):
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:9: """
> On 2011/05/30 21:23:24, Dirk Pranke wrote:
>>
>> This comment isn't that helpful. I would replace it with something
>
> like:
>
>> Generate a CL to roll webkit to the specified revision number and post
>
> it to
>>
>> Rietveld so that the CL will land automatically if it passes the
>
> commit-queue's
>>
>> checks.
>
>
> done
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:34: new_line = '  "webkit_revision": "%d",'
> % new_rev
> On 2011/05/30 21:23:24, Dirk Pranke wrote:
>>
>> Hm. I'd probably do this using readlines() for line-at-a-time parsing
>
> and then
>>
>> match against and update a single line.
>
>> At the very least, I'd want to explicitly match against the beginning
>
> of the
>>
>> line using ^\s+ instead of the '  ' that you have now (and also match
>
> against
>>
>> the end of the line using $).
>
>> Your code could get confused (I think) by having comments elsewhere in
>
> the DEPS
>>
>> file that refer to "webkit_revision: 3"  or something else like that.
>
> I prefer to keep the dumb logic here and change it if needed. I added
> count=1 so only one replacement is done. I changed the code below to
> print the diff so it will pause if the diff is too long.
>
>
http://codereview.chromium.org/7089012/diff/3001/tools/safely-roll-webkit.py#...
> tools/safely-roll-webkit.py:76: subprocess2.check_output(['git',
> 'branch', '-D', 'webkit_roll'])
> On 2011/05/30 21:23:24, Dirk Pranke wrote:
>>
>> I personally don't like to delete branches until after the change has
>
> landed. I
>>
>> wonder if it makes sense to provide for that, or (more generally) a
>
> way to break
>>
>> this up into each step?
>
>> On the other hand, making this extremely simple and idiot-proof is
>
> probably a
>>
>> good thing for now. You can always make it more flexible later.
>
> By using check_call(git checkout -b) before the try block, it will
> return 1 if the branch already existed or if uncommitted files would be
> touched so this is quite safe. Also, the branch is directly done against
> upstream and not the current branch.
>
> http://codereview.chromium.org/7089012/
>

[M-A Ruel, 2011-07-25 17:34:45]

Reviving this patch now that TBR works with the commit queue. I've switched the
description to set TBR= correctly so the presubmit check realizes it shouldn't
bother. I've also written an integration test in the commit queue to verify this
workflow.

[abarth-chromium, 2011-07-25 17:38:26]

I'm super excited about this script.  Once this lands, I'll set up a test
instance.

[Dirk Pranke, 2011-07-25 19:11:22]

LGTM.

[commit-bot: I haz the power, 2011-07-25 21:12:43]

Change committed as 93945