Fix javac command never caching input .md5s

build/android/gyp/javac.py

Index: build/android/gyp/javac.py
diff --git a/build/android/gyp/javac.py b/build/android/gyp/javac.py
index dafe5dff0ab9a9f767c3105641283fa30a84cdaf..1462aaa583dffad5cf6fc27ff6ae08474b55810c 100755
--- a/build/android/gyp/javac.py
+++ b/build/android/gyp/javac.py
@@ -65,16 +65,27 @@ ERRORPRONE_OPTIONS = [
   'com.google.errorprone.bugpatterns.ElementsCountedInLoop'
 ]
 
-def DoJavac(
-    bootclasspath, classpath, classes_dir, chromium_code,
-    use_errorprone_path, java_files):
-  """Runs javac.
-
-  Builds |java_files| with the provided |classpath| and puts the generated
-  .class files into |classes_dir|. If |chromium_code| is true, extra lint
-  checking will be enabled.
+def _FilterJavaFiles(paths, javac_includes):
+  if not javac_includes:
+    return paths
+  ret = []

[jbudorick, 2015/08/23 02:50:51]

return [f for f in paths if any(fnmatch.fnmatch(f, i) for i in javac_includes)]

?

[agrieve, 2015/08/26 05:52:58]

Done (and moved to build_utils.py since it was copy&pasted there as well)

+  for f in paths:
+    for include in javac_includes:
+      if fnmatch.fnmatch(f, include):
+        ret.append(f)
+        break
+  return ret
+
+def _DoJavac(bootclasspath=None, classpath=None, java_files=None,
+             java_srcjars=None, javac_includes=None, jar_path=None,
+             chromium_code=False, use_errorprone_path=None, main_class=None,
+             manifest_entry=None, jar_excluded_classes=None):
+  """Compiles .java into a .jar.
+
+  If |chromium_code| is true, extra lint checking will be enabled.
   """
-
+  java_srcjars = java_srcjars or []
+  java_files = _FilterJavaFiles(java_files, javac_includes)
   jar_inputs = []
   for path in classpath:
     if os.path.exists(path + '.TOC'):
@@ -88,7 +99,7 @@ def DoJavac(
       # javac pulling a default encoding from the user's environment.
       '-encoding', 'UTF-8',
       '-classpath', ':'.join(classpath),
-      '-d', classes_dir]
+      ]
 
   if bootclasspath:
     javac_args.extend([
@@ -106,32 +117,58 @@ def DoJavac(
     # trigger a compile warning or error.
     javac_args.extend(['-XDignore.symbol.file'])
 
+  javac_cmd = ['javac']
   if use_errorprone_path:
     javac_cmd = [use_errorprone_path] + ERRORPRONE_OPTIONS
-  else:
-    javac_cmd = ['javac']
-
-  javac_cmd = javac_cmd + javac_args + java_files
 
   def Compile():
-    build_utils.CheckOutput(
-        javac_cmd,
-        print_stdout=chromium_code,
-        stderr_filter=ColorJavacOutput)
+    with build_utils.TempDir() as temp_dir:
+      if java_srcjars:
+        java_dir = os.path.join(temp_dir, 'java')
+        os.makedirs(java_dir)
+        for srcjar in java_srcjars:
+          build_utils.ExtractAll(srcjar, path=java_dir, pattern='*.java')
+        jar_srcs = build_utils.FindInDirectory(java_dir, '*.java')
+        java_files.extend(_FilterJavaFiles(jar_srcs, javac_includes))
+
+      classes_dir = os.path.join(temp_dir, 'classes')
+      os.makedirs(classes_dir)
+      # Don't include the output directory in the initial set of args since it
+      # being in a temp dir makes it unstable (breaks md5 stamping).
+      cmd = javac_cmd + javac_args + ['-d', classes_dir] + java_files
+
+      build_utils.CheckOutput(
+          cmd,
+          print_stdout=chromium_code,
+          stderr_filter=ColorJavacOutput)
+
+      if main_class or manifest_entry:
+        if manifest_entry:
+          entries = map(lambda e: e.split(":"), manifest_entry)

[jbudorick, 2015/08/23 02:50:51]

entries = [e.split(':') for e in manifest_entry]

[agrieve, 2015/08/26 05:52:58]

Done.

+        else:
+          entries = []
+        manifest_file = os.path.join(temp_dir, 'manifest')
+        _CreateManifest(manifest_file, classpath, main_class, entries)
+      else:
+        manifest_file = None
+      jar.JarDirectory(classes_dir,
+                       jar_excluded_classes,
+                       jar_path,
+                       manifest_file=manifest_file)
 
-  record_path = os.path.join(classes_dir, 'javac.md5.stamp')

[jbudorick, 2015/08/23 02:50:50]

This is the fix, right?

It's kinda hard to tell with all of the shuffling you've done in here :/

[agrieve, 2015/08/26 05:52:58]

Yes, it's the crux of it. The script shouldn't be creating a temporary directory
and extracting srcjars if nothing has changed though, so that's the reason for
the re-jigger. Arguably could have split that out...

+  record_path = jar_path + '.javac.md5.stamp'
   md5_check.CallAndRecordIfStale(
       Compile,
       record_path=record_path,
-      input_paths=java_files + jar_inputs,
-      input_strings=javac_cmd)
+      input_paths=java_files + jar_inputs + java_srcjars,
+      input_strings=javac_cmd + javac_args)
 
 
 _MAX_MANIFEST_LINE_LEN = 72
 
 
-def CreateManifest(manifest_path, classpath, main_class=None,
-                   manifest_entries=None):
+def _CreateManifest(manifest_path, classpath, main_class=None,
+                    manifest_entries=None):
   """Creates a manifest file with the given parameters.
 
   This generates a manifest file that compiles with the spec found at
@@ -199,6 +236,7 @@ def main(argv):
       'will all be appended to construct the classpath.')
   parser.add_option(
       '--javac-includes',
+      default='',
       help='A list of file patterns. If provided, only java files that match'
       'one of the patterns will be compiled.')
   parser.add_option(
@@ -216,9 +254,6 @@ def main(argv):
       '--use-errorprone-path',
       help='Use the Errorprone compiler at this path.')
 
-  parser.add_option(
-      '--classes-dir',
-      help='Directory for compiled .class files.')
   parser.add_option('--jar-path', help='Jar output path.')
   parser.add_option(
       '--main-class',
@@ -231,9 +266,7 @@ def main(argv):
   parser.add_option('--stamp', help='Path to touch on success.')
 
   options, args = parser.parse_args(argv)
-
-  if options.main_class and not options.jar_path:
-    parser.error('--main-class requires --jar-path')
+  build_utils.CheckOptions(options, parser, required=('jar_path',))
 
   bootclasspath = []
   for arg in options.bootclasspath:
@@ -253,58 +286,21 @@ def main(argv):
     java_files += build_utils.FindInDirectories(src_gendirs, '*.java')
 
   input_files = bootclasspath + classpath + java_srcjars + java_files
-  with build_utils.TempDir() as temp_dir:
-    classes_dir = os.path.join(temp_dir, 'classes')
-    os.makedirs(classes_dir)
-    if java_srcjars:
-      java_dir = os.path.join(temp_dir, 'java')
-      os.makedirs(java_dir)
-      for srcjar in java_srcjars:
-        build_utils.ExtractAll(srcjar, path=java_dir, pattern='*.java')
-      java_files += build_utils.FindInDirectory(java_dir, '*.java')
-
-    if options.javac_includes:
-      javac_includes = build_utils.ParseGypList(options.javac_includes)
-      filtered_java_files = []
-      for f in java_files:
-        for include in javac_includes:
-          if fnmatch.fnmatch(f, include):
-            filtered_java_files.append(f)
-            break
-      java_files = filtered_java_files
-
-    if len(java_files) != 0:
-      DoJavac(
-          bootclasspath,
-          classpath,
-          classes_dir,
-          options.chromium_code,
-          options.use_errorprone_path,
-          java_files)
-
-    if options.jar_path:
-      if options.main_class or options.manifest_entry:
-        if options.manifest_entry:
-          entries = map(lambda e: e.split(":"), options.manifest_entry)
-        else:
-          entries = []
-        manifest_file = os.path.join(temp_dir, 'manifest')
-        CreateManifest(manifest_file, classpath, options.main_class, entries)
-      else:
-        manifest_file = None
-      jar.JarDirectory(classes_dir,
-                       build_utils.ParseGypList(options.jar_excluded_classes),
-                       options.jar_path,
-                       manifest_file=manifest_file)
 
-    if options.classes_dir:
-      # Delete the old classes directory. This ensures that all .class files in
-      # the output are actually from the input .java files. For example, if a
-      # .java file is deleted or an inner class is removed, the classes
-      # directory should not contain the corresponding old .class file after
-      # running this action.
-      build_utils.DeleteDirectory(options.classes_dir)
-      shutil.copytree(classes_dir, options.classes_dir)
+  javac_includes = build_utils.ParseGypList(options.javac_includes)
+  jar_excluded_classes = build_utils.ParseGypList(options.jar_excluded_classes)
+
+  _DoJavac(bootclasspath=bootclasspath,
+           classpath=classpath,
+           java_files=java_files,
+           java_srcjars=java_srcjars,
+           javac_includes = javac_includes,
+           jar_path=options.jar_path,
+           chromium_code=options.chromium_code,
+           use_errorprone_path=options.use_errorprone_path,
+           main_class=options.main_class,
+           manifest_entry=options.manifest_entry,
+           jar_excluded_classes=jar_excluded_classes)
 
   if options.depfile:
     build_utils.WriteDepfile(