Fixed bug in dependency recursion in isolate_driver.py.

tools/isolate_driver.py

 #!/usr/bin/env python
 # Copyright 2014 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.
 
 """Adaptor script called through build/isolate.gypi.
 
 Creates a wrapping .isolate which 'includes' the original one, that can be
 consumed by tools/swarming_client/isolate.py. Path variables are determined
 based on the current working directory. The relative_cwd in the .isolated file
@@ skipping 95 matching lines @@
 
 
 def using_blacklist(item):
   """Returns True if an item should be analyzed.
 
   Ignores many rules that are assumed to not depend on a dynamic library. If
   the assumption doesn't hold true anymore for a file format, remove it from
   this list. This is simply an optimization.
   """
   IGNORED = (
-    '.a', '.cc', '.css', '.def', '.h', '.html', '.js', '.json', '.manifest',
-    '.o', '.obj', '.pak', '.png', '.pdb', '.strings', '.txt',
+    '.a', '.cc', '.css', '.def', '.frag', '.h', '.html', '.js', '.json',
+    '.manifest', '.o', '.obj', '.pak', '.png', '.pdb', '.strings', '.test',
+    '.txt', '.vert',
   )
   # ninja files use native path format.
   ext = os.path.splitext(item)[1]
   if ext in IGNORED:
     return False
   # Special case Windows, keep .dll.lib but discard .lib.
   if item.endswith('.dll.lib'):
     return True
   if ext == '.lib':
     return False
   return item not in ('', '|', '||')
 
 
 def raw_build_to_deps(item):
   """Converts a raw ninja build statement into the list of interesting
   dependencies.
   """
   # TODO(maruel): Use a whitelist instead? .stamp, .so.TOC, .dylib.TOC,
   # .dll.lib, .exe and empty.
   # The first item is the build rule, e.g. 'link', 'cxx', 'phony', etc.
   return filter(using_blacklist, item.split(' ')[1:])
 
 
-def recurse(target, build_steps, rules_seen):
+def recurse(target, build_steps, rules_seen, dependencies_added):

[Vadim Sh., 2014/05/30 22:15:25]

Please make this function _not_ to return anything, and explicitly document that
it mutates |dependencies_added| in place. Also it makes sense to rename to
'collect_deps' or something like that, to indicate it gathers deps in passed
set.

Functions that return results and have side effects on arguments and
confusing... 

[Ken Russell (switch to Gerrit), 2014/05/30 22:43:49]

Thanks for the good suggestion. Done.

   """Recursively returns all the interesting dependencies for root_item."""
-  out = []
   if rules_seen is None:
     rules_seen = set()
+  if dependencies_added is None:
+    dependencies_added = set()
   if target in rules_seen:
     # TODO(maruel): Figure out how it happens.
     logging.warning('Circular dependency for %s!', target)
-    return []
+    return dependencies_added
   rules_seen.add(target)
   try:
     dependencies = raw_build_to_deps(build_steps[target])
   except KeyError:
     logging.info('Failed to find a build step to generate: %s', target)
-    return []
+    return dependencies_added
   logging.debug('recurse(%s) -> %s', target, dependencies)
   for dependency in dependencies:
-    out.append(dependency)
-    dependency_raw_dependencies = build_steps.get(dependency)
-    if dependency_raw_dependencies:
-      for i in raw_build_to_deps(dependency_raw_dependencies):
-        out.extend(recurse(i, build_steps, rules_seen))
-    else:
-      logging.info('Failed to find a build step to generate: %s', dependency)
-  return out
+    dependencies_added.add(dependency)
+    recurse(dependency, build_steps, rules_seen, dependencies_added)
+  return dependencies_added
 
 
 def post_process_deps(build_dir, dependencies):
   """Processes the dependency list with OS specific rules."""
   def filter_item(i):
     if i.endswith('.so.TOC'):
       # Remove only the suffix .TOC, not the .so!
       return i[:-4]
     if i.endswith('.dylib.TOC'):
       # Remove only the suffix .TOC, not the .dylib!
@@ skipping 36 matching lines @@
 
   # Relative path between the new and old .isolate file.
   isolate_relpath = os.path.relpath(
       '.', temp_isolate_dir).replace(os.path.sep, '/')
 
   # It's a big assumption here that the name of the isolate file matches the
   # primary target '_run'. Fix accordingly if this doesn't hold true, e.g.
   # complain to maruel@.
   target = isolate[:-len('.isolate')] + '_run'
   build_steps = load_ninja(build_dir)
-  binary_deps = post_process_deps(build_dir, recurse(target, build_steps, None))
+  binary_deps = post_process_deps(build_dir, recurse(

[Vadim Sh., 2014/05/30 22:15:25]

and then:
binary_deps = set()
collect_deps(target, build_steps, None, binary_deps)
binary_deps = post_process_deps(build_dir, binary_deps)

[Ken Russell (switch to Gerrit), 2014/05/30 22:43:49]

Done. I switched the order of the dependencies_added and rules_seen arguments so
None could continue to be the last argument.

+    target, build_steps, None, None))
   logging.debug(
       'Binary dependencies:%s', ''.join('\n  ' + i for i in binary_deps))
 
   # Now do actual wrapping .isolate.
   isolate_dict = {
     'includes': [
       posixpath.join(isolate_relpath, isolate),
     ],
     'variables': {
       # Will look like ['<(PRODUCT_DIR)/lib/flibuser_prefs.so'].
@@ skipping 39 matching lines @@
 
   swarming_client = os.path.join(SRC_DIR, 'tools', 'swarming_client')
   sys.stdout.flush()
   result = subprocess.call(
       [sys.executable, os.path.join(swarming_client, 'isolate.py')] + args)
   return result
 
 
 if __name__ == '__main__':
   sys.exit(main())