Teach bot_update to support patching deps in solutions other than the patch_root

Teach bot_update to support patching deps in solutions other than the patch_root

R=hinoka@chromium.org, kjellander@chromium.org
BUG=411434
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=291858

[kjellander_chromium, 2014-09-08 11:05:47]

This looks great and should work. Do we dare to commit it without unit tests?
Have you tested it locally?

[agable, 2014-09-08 11:15:35]

On 2014/09/08 11:05:47, kjellander wrote:
> This looks great and should work. Do we dare to commit it without unit tests?
> Have you tested it locally?

No I haven't had a chance to test it locally. If you can (i.e. if you already
have the necessary command line arguments for run_recipe.py set up) please do.
Would be particularly good to test with a patch that hits both src/DEPS and
src/something/DEPS.

[kjellander_chromium, 2014-09-08 14:14:23]

I think with my suggested fixes this will work. I have tested it locally.

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1260: for solution in solutions:
Please add a blacklist to be used when patching the rest below. Something like:

# A blacklist used to exclude the DEPS patch entries when applying the other
# parts of the patch further below.
blacklist = []

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1264: target = '/'.join(relative_root,
'DEPS').lstrip('/')
This needs to be
target = '/'.join([relative_root, 'DEPS']).lstrip('/')

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1265: if patches:
Add
blacklist.append(target)
somewhere around here.

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1302: apply_svn_patch(patch_root, patches,
blacklist=['DEPS'])
blacklist=blacklist
here and below.

[kjellander_chromium, 2014-09-08 14:31:41]

lgtm!

[kjellander_chromium, 2014-09-08 14:44:01]

On 2014/09/08 14:31:41, kjellander wrote:
> lgtm!

Ryan, can you CQ this and monitor for breakages as soon you can?

[hinoka, 2014-09-08 16:43:48]

Yep, I can babysit this today.


On Mon, Sep 8, 2014 at 4:44 PM, <kjellander@chromium.org> wrote:

> On 2014/09/08 14:31:41, kjellander wrote:
>
>> lgtm!
>>
>
> Ryan, can you CQ this and monitor for breakages as soon you can?
>
> https://codereview.chromium.org/552763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Tseng, 2014-09-08 18:00:44]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2014-09-08 18:00:45]

lgtm i'll land and watch this.

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update...
scripts/slave/bot_update.py:1264: relative_root =
solution['name'][len(patch_root) + 1:]
blerg, I was trying to do this with more posixpath join/split operations rather
than string manipulations, but for the purpose of the P0 this'll do (also python
doesn't have any good way to do this out of the box).

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update...
scripts/slave/bot_update.py:1268: already_patched.append(target)
This can be done outside of the if/else block (and named "potential_deps") to
save a line.

[agable, 2014-09-08 18:23:24]

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1260: for solution in solutions:
On 2014/09/08 14:14:23, kjellander wrote:
> Please add a blacklist to be used when patching the rest below. Something
like:
> 
> # A blacklist used to exclude the DEPS patch entries when applying the other
> # parts of the patch further below.
> blacklist = []

Acknowledged.

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1264: target = '/'.join(relative_root,
'DEPS').lstrip('/')
On 2014/09/08 14:14:23, kjellander wrote:
> This needs to be
> target = '/'.join([relative_root, 'DEPS']).lstrip('/')

Done.

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1265: if patches:
On 2014/09/08 14:14:23, kjellander wrote:
> Add
> blacklist.append(target)
> somewhere around here.

Acknowledged.

https://codereview.chromium.org/552763002/diff/1/scripts/slave/bot_update.py#...
scripts/slave/bot_update.py:1302: apply_svn_patch(patch_root, patches,
blacklist=['DEPS'])
On 2014/09/08 14:14:23, kjellander wrote:
> blacklist=blacklist
> here and below.

Done.

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update...
scripts/slave/bot_update.py:1264: relative_root =
solution['name'][len(patch_root) + 1:]
On 2014/09/08 18:00:45, Ryan T. (EMEA time) wrote:
> blerg, I was trying to do this with more posixpath join/split operations
rather
> than string manipulations, but for the purpose of the P0 this'll do (also
python
> doesn't have any good way to do this out of the box).

This is the generic path way. The only additional utility path gives us is
"common prefix", but that operates on a character level, not a path component
level, which means that it isn't actually helpful here.

https://codereview.chromium.org/552763002/diff/20001/scripts/slave/bot_update...
scripts/slave/bot_update.py:1268: already_patched.append(target)
On 2014/09/08 18:00:45, Ryan T. (EMEA time) wrote:
> This can be done outside of the if/else block (and named "potential_deps") to
> save a line.

I'd only be comfortable doing that if the below was an "else", not an "elif".
I'm only adding files to "already_patched" if they've actually already been
patched, not just if we thought we maybe should have patched them.

[Ryan Tseng, 2014-09-08 18:43:30]

The CQ bit was checked by hinoka@google.com

[commit-bot: I haz the power, 2014-09-08 18:43:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/agable@chromium.org/552763002/20001

[commit-bot: I haz the power, 2014-09-08 18:45:18]

Committed patchset #2 (id:20001) as 291858

[Ryan Tseng, 2014-09-08 18:57:07]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/550103004/ by hinoka@google.com.

The reason for reverting is: Lots of bot_update failures on the waterfall.  I
don't see how this could've caused it yet, but gonna speculatively revert..