Add mode 'hashtable'.

Refactor the code to be easier to read.

Add more integration tests.

R=rogerta@chromium.org
BUG=
TEST=


Review URL: http://codereview.chromium.org/9553015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125398 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 225 insertions, 147 deletions

diff --git a/tools/isolate/isolate.py b/tools/isolate/isolate.py
index be4f076..d70296a 100755
--- a/tools/isolate/isolate.py
+++ b/tools/isolate/isolate.py
@@ -4,14 +4,21 @@
 # found in the LICENSE file.
 
 """Does one of the following depending on the --mode argument:
-  check   verify all the inputs exist, touches the file specified with
-          --result and exits.
-  run     recreates a tree with all the inputs files and run the executable
-          in it.
-  remap   stores all the inputs files in a directory without running the
-          executable.
+  check     verify all the inputs exist, touches the file specified with
+            --result and exits.
+  hashtable puts a manifest file and hard links each of the inputs into the
+            output directory.
+  remap     stores all the inputs files in a directory without running the
+            executable.
+  run       recreates a tree with all the inputs files and run the executable
+            in it.
+
+See more information at
+http://dev.chromium.org/developers/testing/isolated-testing
 """
 
+import hashlib
+import json
 import logging
 import optparse
 import os
@@ -24,6 +31,9 @@ import time
 
 import tree_creator
 
+# Needs to be coherent with the file's docstring above.
+VALID_MODES = ('check', 'hashtable', 'remap', 'run')
+
 
 def touch(filename):
   """Implements the equivalent of the 'touch' command."""
@@ -48,75 +58,104 @@ def rmtree(root):
     shutil.rmtree(root)
 
 
-def isolate(outdir, root, resultfile, mode, read_only, args):
-  """Main function to isolate a target with its dependencies."""
+def relpath(path, root):
+  """os.path.relpath() that keeps trailing slash."""
+  out = os.path.relpath(path, root)
+  if path.endswith('/'):
+    out += '/'
+  return out
+
+
+def separate_inputs_command(args, root):
+  """Strips off the command line from the inputs.
+
+  gyp provides input paths relative to cwd. Convert them to be relative to root.
+  """
   cmd = []
   if '--' in args:
-    # Strip off the command line from the inputs.
     i = args.index('--')
     cmd = args[i+1:]
     args = args[:i]
-
-  # gyp provides paths relative to cwd. Convert them to be relative to
-  # root.
   cwd = os.getcwd()
+  return [relpath(os.path.join(cwd, arg), root) for arg in args], cmd
 
-  def make_relpath(i):
-    """Makes an input file a relative path but keeps any trailing slash."""
-    out = os.path.relpath(os.path.join(cwd, i), root)
-    if i.endswith('/'):
-      out += '/'
-    return out
 
-  infiles = [make_relpath(i) for i in args]
+def isolate(outdir, resultfile, indir, infiles, mode, read_only, cmd):
+  """Main function to isolate a target with its dependencies.
 
-  if not infiles:
-    raise ValueError('Need at least one input file to map')
+  It's behavior depends on |mode|.
+  """
+  if mode == 'run':
+    return run(outdir, resultfile, indir, infiles, read_only, cmd)
+
+  if mode == 'hashtable':
+    return hashtable(outdir, resultfile, indir, infiles)
+
+  assert mode in ('check', 'remap'), mode
+  if mode == 'remap':
+    if not outdir:
+      outdir = tempfile.mkdtemp(prefix='isolate')
+    tree_creator.recreate_tree(
+        outdir, indir, infiles, tree_creator.HARDLINK)
+    if read_only:
+      tree_creator.make_writable(outdir, True)
 
-  # Other modes ignore the command.
-  if mode == 'run' and not cmd:
+  if resultfile:
+    # Signal the build tool that the test succeeded.
+    with open(resultfile, 'wb') as f:
+      for infile in infiles:
+        f.write(infile.encode('utf-8'))
+        f.write('\n')
+
+
+def run(outdir, resultfile, indir, infiles, read_only, cmd):
+  """Implements the 'run' mode."""
+  if not cmd:
     print >> sys.stderr, 'Using first input %s as executable' % infiles[0]
     cmd = [infiles[0]]
-
-  tempdir = None
+  outdir = None
   try:
-    if not outdir and mode != 'check':
-      tempdir = tempfile.mkdtemp(prefix='isolate')
-      outdir = tempdir
-    elif outdir:
-      outdir = os.path.abspath(outdir)
-
+    outdir = tempfile.mkdtemp(prefix='isolate')
     tree_creator.recreate_tree(
-        outdir,
-        os.path.abspath(root),
-        infiles,
-        tree_creator.DRY_RUN if mode == 'check' else tree_creator.HARDLINK,
-        lambda x: re.match(r'.*\.(svn|pyc)$', x))
-
-    if mode != 'check' and read_only:
+        outdir, indir, infiles, tree_creator.HARDLINK)
+    if read_only:
       tree_creator.make_writable(outdir, True)
 
-    if mode in ('check', 'remap'):
-      result = 0
-    else:
-      # TODO(maruel): Remove me. Layering violation. Used by
-      # base/base_paths_linux.cc
-      env = os.environ.copy()
-      env['CR_SOURCE_ROOT'] = outdir.encode()
-      # Rebase the command to the right path.
-      cmd[0] = os.path.join(outdir, cmd[0])
-      logging.info('Running %s' % cmd)
-      result = subprocess.call(cmd, cwd=outdir, env=env)
-
+    # TODO(maruel): Remove me. Layering violation. Used by
+    # base/base_paths_linux.cc
+    env = os.environ.copy()
+    env['CR_SOURCE_ROOT'] = outdir.encode()
+    # Rebase the command to the right path.
+    cmd[0] = os.path.join(outdir, cmd[0])
+    logging.info('Running %s' % cmd)
+    result = subprocess.call(cmd, cwd=outdir, env=env)
     if not result and resultfile:
       # Signal the build tool that the test succeeded.
       touch(resultfile)
     return result
   finally:
-    if tempdir and mode == 'isolate':
-      if read_only:
-        tree_creator.make_writable(tempdir, False)
-      rmtree(tempdir)
+    if read_only:
+      tree_creator.make_writable(outdir, False)
+    rmtree(outdir)
+
+
+def hashtable(outdir, resultfile, indir, infiles):
+  """Implements the 'hashtable' mode."""
+  results = {}
+  for relfile in infiles:
+    infile = os.path.join(indir, relfile)
+    h = hashlib.sha1()
+    with open(infile, 'rb') as f:
+      h.update(f.read())
+    digest = h.hexdigest()
+    outfile = os.path.join(outdir, digest)
+    tree_creator.process_file(outfile, infile, tree_creator.HARDLINK)
+    results[relfile] = {'sha1': digest}
+  json.dump(
+      {
+        'files': results,
+      },
+      open(resultfile, 'wb'))
 
 
 def main():
@@ -128,16 +167,17 @@ def main():
   parser.add_option(
       '-v', '--verbose', action='count', default=0, help='Use multiple times')
   parser.add_option(
-      '--mode', choices=['remap', 'check', 'run'],
-      help='Determines the action to be taken')
+      '--mode', choices=VALID_MODES,
+      help='Determines the action to be taken: %s' % ', '.join(VALID_MODES))
   parser.add_option(
-      '--result', metavar='X',
+      '--result', metavar='FILE',
       help='File to be touched when the command succeeds')
-  parser.add_option('--root', help='Base directory to fetch files, required')
   parser.add_option(
-      '--outdir', metavar='X',
-      help='Directory used to recreate the tree. Defaults to a /tmp '
-           'subdirectory')
+      '--root', metavar='DIR', help='Base directory to fetch files, required')
+  parser.add_option(
+      '--outdir', metavar='DIR',
+      help='Directory used to recreate the tree or store the hash table. '
+           'Defaults to a /tmp subdirectory for modes run and remap.')
   parser.add_option(
       '--read-only', action='store_true',
       help='Make the temporary tree read-only')
@@ -151,16 +191,24 @@ def main():
   if not options.root:
     parser.error('--root is required.')
 
+  infiles, cmd = separate_inputs_command(args, options.root)
+  if not infiles:
+    parser.error('Need at least one input file to map')
+  # Preprocess the input files.
   try:
+    infiles, root = tree_creator.preprocess_inputs(
+          options.root, infiles, lambda x: re.match(r'.*\.(svn|pyc)$', x))
     return isolate(
         options.outdir,
-        options.root,
         options.result,
+        root,
+        infiles,
         options.mode,
         options.read_only,
-        args)
-  except ValueError, e:
-    parser.error(str(e))
+        cmd)
+  except tree_creator.MappingError, e:
+    print >> sys.stderr, str(e)
+    return 1
 
 
 if __name__ == '__main__':
diff --git a/tools/isolate/isolate_test.py b/tools/isolate/isolate_test.py
index eb44d53..aa1dd69 100755
--- a/tools/isolate/isolate_test.py
+++ b/tools/isolate/isolate_test.py
@@ -3,7 +3,9 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import hashlib
 import os
+import re
 import shutil
 import subprocess
 import sys
@@ -15,6 +17,10 @@ ROOT_DIR = os.path.dirname(os.path.abspath(__file__))
 
 class Isolate(unittest.TestCase):
   def setUp(self):
+    # The reason is that isolate_test.py --ok is run in a temporary directory
+    # without access to isolate.py
+    import isolate
+    self.isolate = isolate
     self.tempdir = tempfile.mkdtemp()
     self.result = os.path.join(self.tempdir, 'result')
 
@@ -32,6 +38,22 @@ class Isolate(unittest.TestCase):
         stdout=subprocess.PIPE,
         stderr=subprocess.STDOUT)
 
+  def test_help_modes(self):
+    # Check coherency in the help and implemented modes.
+    p = subprocess.Popen(
+        [sys.executable, os.path.join(ROOT_DIR, 'isolate.py'), '--help'],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.STDOUT)
+    out = p.communicate()[0].splitlines()
+    self.assertEquals(0, p.returncode)
+    out = out[out.index('') + 1:]
+    out = out[:out.index('')]
+    modes = [re.match(r'^  (\w+) .+', l) for l in out]
+    modes = tuple(m.group(1) for m in modes if m)
+    self.assertEquals(self.isolate.VALID_MODES, modes)
+    for mode in modes:
+      self.assertTrue(hasattr(self, 'test_%s' % mode), mode)
+
   def test_check(self):
     cmd = [
         '--mode', 'check',
@@ -52,6 +74,34 @@ class Isolate(unittest.TestCase):
       pass
     self.assertFalse(os.path.isfile(self.result))
 
+  def test_hashtable(self):
+    cmd = [
+        '--mode', 'hashtable',
+        '--outdir', self.tempdir,
+        'isolate_test.py',
+    ]
+    self._execute(cmd)
+    # Calculate our hash.
+    h = hashlib.sha1()
+    h.update(open(__file__, 'rb').read())
+    digest = h.hexdigest()
+    self.assertEquals(
+      '{"files": {"isolate_test.py": {"sha1": "%s"}}}' % digest,
+      open(self.result, 'rb').read())
+    self.assertEquals(
+        sorted([digest, 'result']), sorted(os.listdir(self.tempdir)))
+
+  def test_remap(self):
+    cmd = [
+        '--mode', 'remap',
+        '--outdir', self.tempdir,
+        'isolate_test.py',
+    ]
+    self._execute(cmd)
+    self.assertEquals('isolate_test.py\n', open(self.result, 'rb').read())
+    self.assertEquals(
+        ['isolate_test.py', 'result'], sorted(os.listdir(self.tempdir)))
+
   def test_run(self):
     cmd = [
         '--mode', 'run',
@@ -84,6 +134,7 @@ def main():
     return 0
   if sys.argv[1] == '--fail':
     return 1
+
   unittest.main()
 
 
diff --git a/tools/isolate/pylintrc b/tools/isolate/pylintrc
index 4e4b3e17..0ccc2b4 100644
--- a/tools/isolate/pylintrc
+++ b/tools/isolate/pylintrc
@@ -122,7 +122,7 @@ ignore-mixin-members=yes
 
 # List of classes names for which member attributes should not be checked
 # (useful for classes with attributes dynamically set).
-ignored-classes=SQLObject
+ignored-classes=SQLObject,hashlib
 
 # When zope mode is activated, add a predefined set of Zope acquired attributes
 # to generated-members.
diff --git a/tools/isolate/tree_creator.py b/tools/isolate/tree_creator.py
index 445daa0..9a7af1d 100644
--- a/tools/isolate/tree_creator.py
+++ b/tools/isolate/tree_creator.py
@@ -12,7 +12,7 @@ import sys
 
 
 # Types of action accepted by recreate_tree().
-DRY_RUN, HARDLINK, SYMLINK, COPY = range(5)[1:]
+HARDLINK, SYMLINK, COPY = range(4)[1:]
 
 
 class MappingError(OSError):
@@ -30,30 +30,50 @@ def os_link(source, link_name):
     os.link(source, link_name)
 
 
-def _process_item(outdir, indir, relfile, action, blacklist):
-  """Processes an input file.
+def preprocess_inputs(indir, infiles, blacklist):
+  """Reads the infiles and expands the directories and applies the blacklist.
 
-  Returns True if processed.
+  Returns the normalized indir and infiles. Converts infiles with a trailing
+  slash as the list of its files.
   """
-  logging.debug(
-      '_process_item(%s, %s, %s, %s, %s)' % (
-        outdir, indir, relfile, action, blacklist))
-  if blacklist and blacklist(relfile):
-    return False
-  infile = os.path.normpath(os.path.join(indir, relfile))
-  if not os.path.isfile(infile):
-    raise MappingError('%s doesn\'t exist' % infile)
-
-  if action == DRY_RUN:
-    logging.info('Verified input: %s' % infile)
-    return True
-
-  outfile = os.path.normpath(os.path.join(outdir, relfile))
-  logging.debug('Mapping %s to %s' % (infile, outfile))
-  outsubdir = os.path.dirname(outfile)
-  if not os.path.isdir(outsubdir):
-    os.makedirs(outsubdir)
+  logging.debug('preprocess_inputs(%s, %s, %s)' % (indir, infiles, blacklist))
+  # Both need to be a local path.
+  indir = os.path.normpath(indir)
+  if not os.path.isdir(indir):
+    raise MappingError('%s is not a directory' % indir)
+
+  # Do not call abspath until it was verified the directory exists.
+  indir = os.path.abspath(indir)
+  outfiles = []
+  for relfile in infiles:
+    if os.path.isabs(relfile):
+      raise MappingError('Can\'t map absolute path %s' % relfile)
+    infile = os.path.normpath(os.path.join(indir, relfile))
+    if not infile.startswith(indir):
+      raise MappingError('Can\'t map file %s outside %s' % (infile, indir))
 
+    if relfile.endswith('/'):
+      if not os.path.isdir(infile):
+        raise MappingError(
+            'Input directory %s must have a trailing slash' % infile)
+      for dirpath, dirnames, filenames in os.walk(infile):
+        # Convert the absolute path to subdir + relative subdirectory.
+        relpath = dirpath[len(infile)+1:]
+        outfiles.extend(os.path.join(relpath, f) for f in filenames)
+        for index, dirname in enumerate(dirnames):
+          # Do not process blacklisted directories.
+          if blacklist(os.path.join(relpath, dirname)):
+            del dirnames[index]
+    else:
+      if not os.path.isfile(infile):
+        raise MappingError('Input file %s doesn\'t exist' % infile)
+      outfiles.append(relfile)
+  return outfiles, indir
+
+
+def process_file(outfile, infile, action):
+  """Links a file. The type of link depends on |action|."""
+  logging.debug('Mapping %s to %s' % (infile, outfile))
   if os.path.isfile(outfile):
     raise MappingError('%s already exist' % outfile)
 
@@ -72,82 +92,41 @@ def _process_item(outdir, indir, relfile, action, blacklist):
       shutil.copy(infile, outfile)
   else:
     raise ValueError('Unknown mapping action %s' % action)
-  return True
-
-
-def _recurse_dir(outdir, indir, subdir, action, blacklist):
-  """Processes a directory and all its decendents."""
-  logging.debug(
-      '_recurse_dir(%s, %s, %s, %s, %s)' % (
-        outdir, indir, subdir, action, blacklist))
-  root = os.path.join(indir, subdir)
-  for dirpath, dirnames, filenames in os.walk(root):
-    # Convert the absolute path to subdir + relative subdirectory.
-    relpath = dirpath[len(indir)+1:]
-
-    for filename in filenames:
-      relfile = os.path.join(relpath, filename)
-      _process_item(outdir, indir, relfile, action, blacklist)
-
-    for index, dirname in enumerate(dirnames):
-      # Do not process blacklisted directories.
-      if blacklist(os.path.normpath(os.path.join(relpath, dirname))):
-        del dirnames[index]
 
 
-def recreate_tree(outdir, indir, infiles, action, blacklist):
+def recreate_tree(outdir, indir, infiles, action):
   """Creates a new tree with only the input files in it.
 
   Arguments:
     outdir:    Temporary directory to create the files in.
     indir:     Root directory the infiles are based in.
-    infiles:   List of files to map from |indir| to |outdir|.
+    infiles:   List of files to map from |indir| to |outdir|. Must have been
+               sanitized with preprocess_inputs().
     action:    See assert below.
-    blacklist: Files to unconditionally ignore.
   """
   logging.debug(
-      'recreate_tree(%s, %s, %s, %s, %s)' % (
-        outdir, indir, infiles, action, blacklist))
+      'recreate_tree(%s, %s, %s, %s)' % (outdir, indir, infiles, action))
   logging.info('Mapping from %s to %s' % (indir, outdir))
 
-  assert action in (DRY_RUN, HARDLINK, SYMLINK, COPY)
-  # Both need to be a local path.
-  indir = os.path.normpath(indir)
-  if not os.path.isdir(indir):
-    raise MappingError('%s is not a directory' % indir)
-
-  # Do not call abspath until it was verified the directory exists.
-  indir = os.path.abspath(indir)
-
-  if action != DRY_RUN:
-    outdir = os.path.normpath(outdir)
-    if not os.path.isdir(outdir):
-      logging.info ('Creating %s' % outdir)
-      os.makedirs(outdir)
-    # Do not call abspath until the directory exists.
-    outdir = os.path.abspath(outdir)
+  assert action in (HARDLINK, SYMLINK, COPY)
+  outdir = os.path.normpath(outdir)
+  if not os.path.isdir(outdir):
+    logging.info ('Creating %s' % outdir)
+    os.makedirs(outdir)
+  # Do not call abspath until the directory exists.
+  outdir = os.path.abspath(outdir)
 
   for relfile in infiles:
-    if os.path.isabs(relfile):
-      raise MappingError('Can\'t map absolute path %s' % relfile)
-    infile = os.path.normpath(os.path.join(indir, relfile))
-    if not infile.startswith(indir):
-      raise MappingError('Can\'t map file %s outside %s' % (infile, indir))
-
-    isdir = os.path.isdir(infile)
-    if isdir and not relfile.endswith('/'):
-      raise MappingError(
-          'Input directory %s must have a trailing slash' % infile)
-    if not isdir and relfile.endswith('/'):
-      raise MappingError(
-          'Input file %s must not have a trailing slash' % infile)
-    if isdir:
-      _recurse_dir(outdir, indir, relfile, action, blacklist)
-    else:
-      _process_item(outdir, indir, relfile, action, blacklist)
+    infile = os.path.join(indir, relfile)
+    outfile = os.path.join(outdir, relfile)
+    outsubdir = os.path.dirname(outfile)
+    if not os.path.isdir(outsubdir):
+      os.makedirs(outsubdir)
+    process_file(outfile, infile, action)
 
 
 def _set_write_bit(path, read_only):
+  """Sets or resets the executable bit on a file or directory."""
   mode = os.stat(path).st_mode
   if read_only:
     mode = mode & 0500