Make bot_update on win more resilient

Make bot_update on win more resilient

Sometimes Windows has trouble deleting files. This can cause problems
when lockfiles are left in .git directories.

R=agable@google.com
BUG=659178
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/86fe47d1a076563a664adaaf2ae9e4d8dbe86932

[katthomas, 2016-10-25 20:42:46]

Description was changed from

==========
Make bot_update on win more resilient

Sometimes Windows has trouble deleting files. This can cause problems
when lockfiles are left in .git directories.

R=agable@google.com
BUG=659178
==========

to

==========
Make bot_update on win more resilient

Sometimes Windows has trouble deleting files. This can cause problems
when lockfiles are left in .git directories.

R=agable@google.com
BUG=659178
==========

[katthomas, 2016-10-25 20:42:46]

katthomas@google.com changed reviewers:
+ agable@chromium.org
- agable@google.com

[ddoman1, 2016-10-25 21:55:23]

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
File recipe_modules/bot_update/resources/bot_update.py (right):

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
recipe_modules/bot_update/resources/bot_update.py:524: call('gclient.bat',
'cleanup', '--break_repo_locks')
I am afraid that it may be a better choice to copy & paste "def
_maybe_break_locks()" from gclient_scm.py to bot_update.py.

#1. CMDcleanup() has been marked as deprecated
:
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/gcl...

#2. invoking gclient within git_checkout() may fail.

From
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/rec...

" # Get a checkout of each solution, without DEPS or hooks.
  # Calling git directly because there is no way to run Gclient without
  # invoking DEPS."

[agable, 2016-10-28 16:55:34]

https://codereview.chromium.org/2454463002/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2454463002/diff/1/gclient.py#newcode1601
gclient.py:1601: This is a no-op in Git.
Update this docstring. No longer deprecated, no longer a no-op in Git.

https://codereview.chromium.org/2454463002/diff/1/gclient.py#newcode1607
gclient.py:1607: parser.add_option('--break_repo_locks', action='store_true',
Honestly I wouldn't even bother with this. This will now be the sole purpose of
the 'cleanup' command, so there's no need for the flag.

https://codereview.chromium.org/2454463002/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/2454463002/diff/1/gclient_scm.py#newcode275
gclient_scm.py:275: self._maybe_break_locks(options)
I think you can set the flag directly on the options object, rather than
actually having to include it as a flag in gclient.py.

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
File recipe_modules/bot_update/resources/bot_update.py (right):

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
recipe_modules/bot_update/resources/bot_update.py:524: call('gclient.bat',
'cleanup', '--break_repo_locks')
On 2016/10/25 at 21:55:23, ddoman1 wrote:
> I am afraid that it may be a better choice to copy & paste "def
_maybe_break_locks()" from gclient_scm.py to bot_update.py.
> 
> #1. CMDcleanup() has been marked as deprecated
> :
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/gcl...

I'm the one deprecating cleanup (which has always previously been a no-op for
Git), so I told Katie to take this approach.

> 
> #2. invoking gclient within git_checkout() may fail.
> 
> From
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/rec...
> 
> " # Get a checkout of each solution, without DEPS or hooks.
>   # Calling git directly because there is no way to run Gclient without
>   # invoking DEPS."

This is unfortunately a really good point. Nothing else in this git_checkouts
function invokes gclient, and that's probably on purpose. Dang.

[katthomas, 2016-10-28 23:40:30]

PTAL

https://codereview.chromium.org/2454463002/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2454463002/diff/1/gclient.py#newcode1607
gclient.py:1607: parser.add_option('--break_repo_locks', action='store_true',
On 2016/10/28 16:55:34, agable wrote:
> Honestly I wouldn't even bother with this. This will now be the sole purpose
of
> the 'cleanup' command, so there's no need for the flag.

Done.

https://codereview.chromium.org/2454463002/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/2454463002/diff/1/gclient_scm.py#newcode275
gclient_scm.py:275: self._maybe_break_locks(options)
On 2016/10/28 16:55:34, agable wrote:
> I think you can set the flag directly on the options object, rather than
> actually having to include it as a flag in gclient.py.

Done.

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
File recipe_modules/bot_update/resources/bot_update.py (right):

https://codereview.chromium.org/2454463002/diff/1/recipe_modules/bot_update/r...
recipe_modules/bot_update/resources/bot_update.py:524: call('gclient.bat',
'cleanup', '--break_repo_locks')
On 2016/10/28 16:55:34, agable wrote:
> On 2016/10/25 at 21:55:23, ddoman1 wrote:
> > I am afraid that it may be a better choice to copy & paste "def
> _maybe_break_locks()" from gclient_scm.py to bot_update.py.
> > 
> > #1. CMDcleanup() has been marked as deprecated
> > :
>
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/gcl...
> 
> I'm the one deprecating cleanup (which has always previously been a no-op for
> Git), so I told Katie to take this approach.
> 
> > 
> > #2. invoking gclient within git_checkout() may fail.
> > 
> > From
>
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/rec...
> > 
> > " # Get a checkout of each solution, without DEPS or hooks.
> >   # Calling git directly because there is no way to run Gclient without
> >   # invoking DEPS."
> 
> This is unfortunately a really good point. Nothing else in this git_checkouts
> function invokes gclient, and that's probably on purpose. Dang.

Done.

[agable, 2016-10-31 16:25:51]

lgtm

[katthomas, 2016-10-31 20:45:57]

The CQ bit was checked by katthomas@google.com

[commit-bot: I haz the power, 2016-10-31 20:45:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 20:49:16]

Description was changed from

==========
Make bot_update on win more resilient

Sometimes Windows has trouble deleting files. This can cause problems
when lockfiles are left in .git directories.

R=agable@google.com
BUG=659178
==========

to

==========
Make bot_update on win more resilient

Sometimes Windows has trouble deleting files. This can cause problems
when lockfiles are left in .git directories.

R=agable@google.com
BUG=659178

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/86fe47d1a07656...
==========

[commit-bot: I haz the power, 2016-10-31 20:49:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/86fe47d1a07656...