Ensure that we pick up 'git.bat' and 'svn.bat' on Windows.

fetch.py

Index: fetch.py
diff --git a/fetch.py b/fetch.py
index 80dcce9a2f13a0e7aeefaf8871298a7737154af3..8b7ff013295c067f2e4b1128b5c0c234709b2837 100755
--- a/fetch.py
+++ b/fetch.py
@@ -24,6 +24,8 @@ import subprocess
 import sys
 import pipes
 
+from distutils import spawn
+
 
 SCRIPT_PATH = os.path.dirname(os.path.abspath(__file__))
 
@@ -46,6 +48,9 @@ class Checkout(object):
     self.dryrun = dryrun
     self.spec = spec
     self.root = root
+    self._script_paths = {}
+    self._binary_paths = {}
+    self._binary_subdirs = {}
 
   def exists(self):
     pass
@@ -56,31 +61,69 @@ class Checkout(object):
   def sync(self):
     pass
 
+  def _find_executable(self, binary_name):
+    if sys.platform == 'win32':

[szager1, 2013/04/11 21:30:01]

This is unnecessary; spawn.find_executable will look for the '.exe' suffix
automatically.

[Dirk Pranke, 2013/04/11 21:52:08]

Huh, so it does. That's great, as it simplifies things and removes some
disappointment. I had thought it did but it was failing when I was testing
things last night, but I think it turns out those things either weren't actually
in my path or weren't where I thought they were.

+      exe_name = binary_name + '.exe'
+    else:
+      exe_name = binary_name
+    path = spawn.find_executable(exe_name)
+    if path:
+      return path
+    return spawn.find_executable(

[szager1, 2013/04/11 21:30:01]

What's the purpose of this call?  As far as I can tell, it only checks for the
existence of the file, and returns the path if it exists, without checking
whether it's executable.

Also, you need to handle the case where binary_name is not in
self._binary_subdirs (otherwise you'll get a KeyError).

Maybe:

subdir = self._binary_subdirs.get(binary_name, '')
full_path = os.path.join(SCRIPT_PATH, subdir, exe_name)
if os.access(full_path, os.X_OK):
  return full_path
return None

Really, this whole method could be:

path = os.environ['PATH']
if binary_name in self._binary_subdirs[binary_name]:
  subdir = os.path.join(SCRIPT_PATH, self._binary_subdirs[binary_name])
  path = os.pathsep.join(path, subdir)
return spawn.find_executable(binary_name)

[Dirk Pranke, 2013/04/11 21:52:08]

I thought that spawn.find_executable() does actually check that something is
executable? If not, I agree it's no different than os.path.exists() + returning
the string.
No, I don't want to handle that case. I consider that a programming error, as
this routine should only be called by routines like run_git and run_svn, for a
specific set of binaries dealt with via mixins. It is not a general purpose
routine. I can add in asserts() and/or comments to make this clearer.
  
I don't know quite what you were getting at with this pseudocode, but you're not
actually doing anything with 'path' here?

[szager1, 2013/04/11 22:18:43]

Whoops, should be:

spawn.find_executable(binary_name, path)

+        os.path.join(SCRIPT_PATH,
+                     self._binary_subdirs[binary_name],
+                     exe_name))
+
+  def run_script(self, script, *args, **kwargs):
+    path = self._script_paths.get(script)
+    if not path:
+      path = os.path.join(SCRIPT_PATH, script + '.py')

[szager1, 2013/04/11 21:30:01]

path = os.path.join(SCRIPT_PATH, script)
if not script.endswith('.py):
  path += '.py'

[Dirk Pranke, 2013/04/11 21:52:08]

Again, this is too general for my intentions.

+      if not os.path.exists(path):
+        print "Error: could not find '%s.py' in depot_tools" % script
+        return 1
+      self._script_paths[script] = path
+    print 'Running: %s %s' % (script,
+                              ' '.join(pipes.quote(arg) for arg in args))
+    if self.dryrun:
+      return 0
+    return subprocess.check_call((sys.executable, path) + args, **kwargs)
+
+  def run_binary(self, binary_name, *args, **kwargs):
+    path = self._binary_paths.get(binary_name)
+    if not path:
+      path = self._find_executable(binary_name)
+      if not path:
+        print "Error: could not find '%s' in PATH" % binary_name
+        return 1
+      self._binary_paths[binary_name] = path
+    print 'Running: %s %s' % (binary_name,
+                              ' '.join(pipes.quote(arg) for arg in args))
+    if self.dryrun:
+      return 0
+    return subprocess.check_call((path,) + args, **kwargs)
 
-class GclientCheckout(Checkout):
 
-  def run_gclient(self, *cmd, **kwargs):
-    print 'Running: gclient %s' % ' '.join(pipes.quote(x) for x in cmd)
-    if not self.dryrun:
-      return subprocess.check_call(
-          (sys.executable, os.path.join(SCRIPT_PATH, 'gclient.py')) + cmd,
-          **kwargs)
+class GclientCheckout(Checkout):
+  def run_gclient(self, *args, **kwargs):
+    return self.run_script('gclient', *args, **kwargs)
 
 
 class GitCheckout(Checkout):
+  def __init__(self, *args, **kwargs):
+    super(GitCheckout, self).__init__(*args, **kwargs)
+    self._binary_subdirs['git'] = os.path.join('git-1.8.0_bin', 'bin')

[szager1, 2013/04/11 21:30:01]

if sys.platform == 'win32':
  self._binary_subdirs['git'] = ...

[Dirk Pranke, 2013/04/11 21:52:08]

I'm not sure if you would still want me to make these changes given what I've
written above. I always want binary_subdirs to be populated.

[szager1, 2013/04/11 22:18:43]

Done.

 
-  def run_git(self, *cmd, **kwargs):
-    print 'Running: git %s' % ' '.join(pipes.quote(x) for x in cmd)
-    if not self.dryrun:
-      return subprocess.check_call(('git',) + cmd, **kwargs)
+  def run_git(self, *args, **kwargs):
+    return self.run_binary('git', *args, **kwargs)
 
 
 class SvnCheckout(Checkout):
+  def __init__(self, *args, **kwargs):
+    super(SvnCheckout, self).__init__(*args, **kwargs)
+    self._binary_subdirs['svn'] = 'svn_bin'

[szager1, 2013/04/11 21:30:01]

if sys.platform == 'win32':
  ...

 
-  def run_svn(self, *cmd, **kwargs):
-    print 'Running: svn %s' % ' '.join(pipes.quote(x) for x in cmd)
-    if not self.dryrun:
-      return subprocess.check_call(('svn',) + cmd, **kwargs)
+  def run_svn(self, *args, **kwargs):
+    return self.run_binary('svn', *args, **kwargs)
 
 
 class GclientGitSvnCheckout(GclientCheckout, GitCheckout, SvnCheckout):
@@ -110,7 +153,9 @@ class GclientGitSvnCheckout(GclientCheckout, GitCheckout, SvnCheckout):
         print 'Please run `svn ls %s`' % svn_spec['svn_url']
         return 1
 
-    # TODO(dpranke): Work around issues w/ delta compression on big repos.
+    # TODO(dpranke): This works around issues w/ delta compression on
+    # big repos. We should either not be setting this globally, or
+    # not need it at all, or both.
     self.run_git('config', '--global', 'core.deltaBaseCacheLimit', '1G')
 
     # Configure and do the gclient checkout.