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

fetch.py

 #!/usr/bin/env python
 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """
 Tool to perform checkouts in one easy command line!
 
 Usage:
   fetch <recipe> [--property=value [--property2=value2 ...]]
 
 This script is a wrapper around various version control and repository
 checkout commands. It requires a |recipe| name, fetches data from that
 recipe in depot_tools/recipes, and then performs all necessary inits,
 checkouts, pulls, fetches, etc.
 
 Optional arguments may be passed on the command line in key-value pairs.
 These parameters will be passed through to the recipe's main method.
 """
 
 import json
 import os
 import subprocess
 import sys
 import pipes
 
+from distutils import spawn
+
 
 SCRIPT_PATH = os.path.dirname(os.path.abspath(__file__))
 
 
 #################################################
 # Checkout class definitions.
 #################################################
 class Checkout(object):
   """Base class for implementing different types of checkouts.
 
   Attributes:
     |base|: the absolute path of the directory in which this script is run.
     |spec|: the spec for this checkout as returned by the recipe. Different
         subclasses will expect different keys in this dictionary.
     |root|: the directory into which the checkout will be performed, as returned
         by the recipe. This is a relative path from |base|.
   """
   def __init__(self, dryrun, spec, root):
     self.base = os.getcwd()
     self.dryrun = dryrun
     self.spec = spec
     self.root = root
+    self._script_paths = {}
+    self._binary_paths = {}
+    self._binary_subdirs = {}
 
   def exists(self):
     pass
 
   def init(self):
     pass
 
   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)
+  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):
 
   def __init__(self, dryrun, spec, root):
     super(GclientGitSvnCheckout, self).__init__(dryrun, spec, root)
     assert 'solutions' in self.spec
     keys = ['solutions', 'target_os', 'target_os_only']
     gclient_spec = '\n'.join('%s = %s' % (key, self.spec[key])
                              for key in self.spec if key in keys)
     self.spec['gclient_spec'] = gclient_spec
     assert 'svn_url' in self.spec
     assert 'svn_branch' in self.spec
     assert 'svn_ref' in self.spec
 
   def exists(self):
     return os.path.exists(os.path.join(os.getcwd(), self.root))
 
   def init(self):
     # Ensure we are authenticated with subversion for all submodules.
     git_svn_dirs = json.loads(self.spec.get('submodule_git_svn_spec', '{}'))
     git_svn_dirs.update({self.root: self.spec})
     for _, svn_spec in git_svn_dirs.iteritems():
       try:
         self.run_svn('ls', '--non-interactive', svn_spec['svn_url'])
       except subprocess.CalledProcessError:
         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.
     self.run_gclient('config', '--spec', self.spec['gclient_spec'])
     self.run_gclient('sync')
 
     # Configure git.
     wd = os.path.join(self.base, self.root)
     if self.dryrun:
       print 'cd %s' % wd
@@ skipping 120 matching lines @@
 
 
 def main():
   dryrun, recipe, props = handle_args(sys.argv)
   spec, root = run_recipe_fetch(recipe, props)
   return run(dryrun, spec, root)
 
 
 if __name__ == '__main__':
   sys.exit(main())