Remove git lockfile flakiness on win (bot_update)

recipe_modules/bot_update/resources/bot_update.py

Index: recipe_modules/bot_update/resources/bot_update.py
diff --git a/recipe_modules/bot_update/resources/bot_update.py b/recipe_modules/bot_update/resources/bot_update.py
index ca91dbdf98d15876634feeb65406ba9007608523..9cd4087a552fe0860093545b1376320ea787c1af 100755
--- a/recipe_modules/bot_update/resources/bot_update.py
+++ b/recipe_modules/bot_update/resources/bot_update.py
@@ -210,7 +210,6 @@ def call(*args, **kwargs):  # pragma: no cover
                          (' '.join(args), code, cwd, attempt),
                          code, outval)
 
-
 def git(*args, **kwargs):  # pragma: no cover
   """Wrapper around call specifically for Git commands."""
   if args and args[0] == 'cache':
@@ -222,9 +221,32 @@ def git(*args, **kwargs):  # pragma: no cover
     # have to do it explicitly. This is better than passing shell=True.
     if sys.platform.startswith('win'):
       git_executable += '.bat'
+      # Windows sometimes has trouble deleting files. This can make git commands
+      # that rely on locks fail.
+      remove_git_lockfiles(sys.platform, os.getcwd())

[Paweł Hajdan Jr., 2016/10/07 13:47:43]

Well this still runs before _each_ git command rather than exactly _once_ at the
beginning. One idea might be to add support for breaking locks to gclient, which
knows where all of the sub-repos should be.

I'd suggest something similar to git_cache.py's CMDunlock.

This probably doesn't have to be Windows-specific, right?

[katthomas, 2016/10/07 18:49:16]

Right, I was thinking that any git command could cause this problem (windows
doesn't successfully delete lock file, causing subsequent git command to fail).
We could try doing it just once at the beginning, and then get more aggressive
if that doesn't work?

The issue was only happening on Windows. We discussed that it's not great
practice to manually delete lockfiles, so I think it makes sense to be more
specific here. 

On 2016/10/07 13:47:43, Paweł Hajdan Jr. wrote:

[Paweł Hajdan Jr., 2016/10/10 14:58:04]

That'd sound good to me.
IMO there's nothing wrong with it as long as we do that _once_ and at the
beginning. Locks could have been left in an inconsistent state, and it's
reasonable to bring the system to a consistent state at the beginning of
bot_update step. All following operations should keep the system in a sane
state, unless abruptly interrupted.

I'm fine with starting Windows-only.

[agable, 2016/10/10 23:01:34]

Yep, I'm in agreement with Pawel here: breaking locks for every Git invocation
is both a) slow, and b) not maintaining the invariant of "bot_update should know
what it is doing". Breaking the locks is the solution for "something went wrong
outside of what we control". If bot_update is actually tripping over itself in
the middle of a single invocation, then we definitely need to solve the problem,
but breaking locks won't be the right solution for that.

[katthomas, 2016/10/11 17:29:18]

Makes sense, done.

     cmd = (git_executable,) + args
   return call(*cmd, **kwargs)
 
+def remove_git_lockfiles(platform, cwd, retries=3):
+  if not platform.startswith('win'):

[agable, 2016/10/10 23:01:34]

nit: It's a matter of opinion whether it is better to do this check at the top
of this function or in the caller. The former keeps us safe if someone else
decides to reuse this function; the latter reduces the complexity of the
function parameters and lets someone doing a straight-line readthrough of the
calling function keep going without having to read this function too.

Not expressing a strong opinion either way, just making sure both are being
considered.

[katthomas, 2016/10/11 17:29:18]

Thanks!

+    return
+  git_dir = cwd + '/.git'

[agable, 2016/10/10 23:01:34]

Can you guarantee that all callers of this function will call it from the root
of a repository? I'm pretty sure that in other places we're more careful about
calculating the path to the .git dir. That, or rename 'cwd' to 'git_root_path'
or something like that, so it is clear to callers that the path must be a repo
root.

[katthomas, 2016/10/11 17:29:18]

Done.

+  try:
+    lockfiles = [git_dir + '/' + f for f in os.listdir(git_dir)
+                 if f.endswith('.lock')]
+  except Exception as e:
+    print 'Could not remove git lockfiles: %s' % str(e)
+    return
+  if lockfiles:
+    for filename in lockfiles:
+      print '===Attempting to remove lockfile %s===' % filename

[agable, 2016/10/10 23:01:34]

super duper tiny nit: I'd print these big header/footer pairs only once for the
entire lock-breaking operation. A single line per file per attempt seems like
plenty of output to me.

[katthomas, 2016/10/11 17:29:18]

Done.

+      for _ in xrange(retries):
+        exitcode = call('cmd.exe', '/c', 'del',
+                        '/f', '/q', os.path.normcase(filename))
+        if exitcode == 0:
+          return
+        time.sleep(3)
+      print '===Failed to remove lockfile %s====' % filename
 
 def get_gclient_spec(solutions, target_os, target_os_only, git_cache_dir):
   return GCLIENT_TEMPLATE % {