Fix javac --incremental by using jmake for dependency analysis

BUG=536817

Review URL: https://codereview.chromium.org/1373723003

Cr-Commit-Position: refs/heads/master@{#352065}

5 files changed, 106 insertions, 23 deletions

diff --git a/build/android/gyp/javac.py b/build/android/gyp/javac.py
index eb17acc..65b36a5 100755
--- a/build/android/gyp/javac.py
+++ b/build/android/gyp/javac.py
@@ -12,6 +12,7 @@ import sys
 import textwrap
 
 from util import build_utils
+from util import md5_check
 
 import jar
 
@@ -122,10 +123,47 @@ def _ExtractClassFiles(jar_path, dest_dir, java_files):
   def extract_predicate(path):
     if not path.endswith('.class'):
       return False
-    path_without_suffix = re.sub(r'(?:\$[^/]+)?\.class$', '', path)
-    return not any(path_without_suffix in p for p in java_files)
+    path_without_suffix = re.sub(r'(?:\$|\.)[^/]+class$', '', path)
+    partial_java_path = path_without_suffix + '.java'
+    return not any(p.endswith(partial_java_path) for p in java_files)
 
   build_utils.ExtractAll(jar_path, path=dest_dir, predicate=extract_predicate)
+  for path in build_utils.FindInDirectory(dest_dir, '*.class'):
+    shutil.copystat(jar_path, path)
+
+
+def _ConvertToJMakeArgs(javac_cmd, pdb_path):
+  new_args = ['bin/jmake', '-pdb', pdb_path]
+  if javac_cmd[0] != 'javac':
+    new_args.extend(('-jcexec', new_args[0]))
+  if md5_check.PRINT_EXPLANATIONS:
+    new_args.append('-Xtiming')
+
+  do_not_prefix = ('-classpath', '-bootclasspath')
+  skip_next = False
+  for arg in javac_cmd[1:]:
+    if not skip_next and arg not in do_not_prefix:
+      arg = '-C' + arg
+    new_args.append(arg)
+    skip_next = arg in do_not_prefix
+
+  return new_args
+
+
+def _FilterJMakeOutput(stdout):
+  if md5_check.PRINT_EXPLANATIONS:
+    return stdout
+  return re.sub(r'\b(Jmake version|Writing project database).*?\n', '', stdout)
+
+
+def _FixTempPathsInIncrementalMetadata(pdb_path, temp_dir):
+  # The .pdb records absolute paths. Fix up paths within /tmp (srcjars).
+  if os.path.exists(pdb_path):
+    # Although its a binary file, search/replace still seems to work fine.
+    with open(pdb_path) as fileobj:
+      pdb_data = fileobj.read()
+    with open(pdb_path, 'w') as fileobj:
+      fileobj.write(re.sub(r'/tmp/[^/]*', temp_dir, pdb_data))
 
 
 def _OnStaleMd5(changes, options, javac_cmd, java_files, classpath_inputs,
@@ -140,35 +178,58 @@ def _OnStaleMd5(changes, options, javac_cmd, java_files, classpath_inputs,
     os.makedirs(classes_dir)
 
     changed_paths = None
+    # jmake can handle deleted files, but it's a rare case and it would
+    # complicate this script's logic.
     if options.incremental and changes.AddedOrModifiedOnly():
       changed_paths = set(changes.IterChangedPaths())
       # Do a full compile if classpath has changed.
+      # jmake doesn't seem to do this on its own... Might be that ijars mess up
+      # its change-detection logic.
       if any(p in changed_paths for p in classpath_inputs):
         changed_paths = None
-      else:
-        java_files = [p for p in java_files if p in changed_paths]
-        srcjars = [p for p in srcjars if p in changed_paths]
+
+    if options.incremental:
+      # jmake is a compiler wrapper that figures out the minimal set of .java
+      # files that need to be rebuilt given a set of .java files that have
+      # changed.
+      # jmake determines what files are stale based on timestamps between .java
+      # and .class files. Since we use .jars, .srcjars, and md5 checks,
+      # timestamp info isn't accurate for this purpose. Rather than use jmake's
+      # programatic interface (like we eventually should), we ensure that all
+      # .class files are newer than their .java files, and convey to jmake which
+      # sources are stale by having their .class files be missing entirely
+      # (by not extracting them).
+      pdb_path = options.jar_path + '.pdb'
+      javac_cmd = _ConvertToJMakeArgs(javac_cmd, pdb_path)
+      if srcjars:
+        _FixTempPathsInIncrementalMetadata(pdb_path, temp_dir)
 
     if srcjars:
       java_dir = os.path.join(temp_dir, 'java')
       os.makedirs(java_dir)
       for srcjar in options.java_srcjars:
-        extract_predicate = None
         if changed_paths:
-          changed_subpaths = set(changes.IterChangedSubpaths(srcjar))
-          extract_predicate = lambda p: p in changed_subpaths
-        build_utils.ExtractAll(srcjar, path=java_dir, pattern='*.java',
-                               predicate=extract_predicate)
+          changed_paths.update(changes.IterChangedSubpaths(srcjar))
+        build_utils.ExtractAll(srcjar, path=java_dir, pattern='*.java')
       jar_srcs = build_utils.FindInDirectory(java_dir, '*.java')
-      java_files.extend(_FilterJavaFiles(jar_srcs, options.javac_includes))
+      jar_srcs = _FilterJavaFiles(jar_srcs, options.javac_includes)
+      java_files.extend(jar_srcs)
+      if changed_paths:
+        # Set the mtime of all sources to 0 since we use the absense of .class
+        # files to tell jmake which files are stale.
+        for path in jar_srcs:
+          os.utime(path, (0, 0))
 
     if java_files:
       if changed_paths:
-        # When no files have been removed and the output jar already
-        # exists, reuse .class files from the existing jar.
-        _ExtractClassFiles(options.jar_path, classes_dir, java_files)
-        _ExtractClassFiles(excluded_jar_path, classes_dir, java_files)
-        # Add the extracted files to the classpath.
+        changed_java_files = [p for p in java_files if p in changed_paths]
+        if os.path.exists(options.jar_path):
+          _ExtractClassFiles(options.jar_path, classes_dir, changed_java_files)
+        if os.path.exists(excluded_jar_path):
+          _ExtractClassFiles(excluded_jar_path, classes_dir, changed_java_files)
+        # Add the extracted files to the classpath. This is required because
+        # when compiling only a subset of files, classes that haven't changed
+        # need to be findable.
         classpath_idx = javac_cmd.index('-classpath')
         javac_cmd[classpath_idx + 1] += ':' + classes_dir
 
@@ -179,6 +240,7 @@ def _OnStaleMd5(changes, options, javac_cmd, java_files, classpath_inputs,
       build_utils.CheckOutput(
           cmd,
           print_stdout=options.chromium_code,
+          stdout_filter=_FilterJMakeOutput,
           stderr_filter=ColorJavacOutput)
 
     if options.main_class or options.manifest_entry:
@@ -360,6 +422,8 @@ def main(argv):
       options.jar_path,
       options.jar_path.replace('.jar', '.excluded.jar'),
   ]
+  if options.incremental:
+    output_paths.append(options.jar_path + '.pdb')
 
   # An escape hatch to be able to check if incremental compiles are causing
   # problems.
diff --git a/build/android/gyp/util/md5_check.py b/build/android/gyp/util/md5_check.py
index 3020944..2ddc3ac 100644
--- a/build/android/gyp/util/md5_check.py
+++ b/build/android/gyp/util/md5_check.py
@@ -12,7 +12,7 @@ import zipfile
 
 
 # When set and a difference is detected, a diff of what changed is printed.
-_PRINT_MD5_DIFFS = int(os.environ.get('PRINT_MD5_DIFFS', 0))
+PRINT_EXPLANATIONS = int(os.environ.get('PRINT_BUILD_EXPLANATIONS', 0))
 
 # An escape hatch that causes all targets to be rebuilt.
 _FORCE_REBUILD = int(os.environ.get('FORCE_REBUILD', 0))
@@ -77,7 +77,7 @@ def CallAndRecordIfStale(
   if not changes.HasChanges():
     return
 
-  if _PRINT_MD5_DIFFS:
+  if PRINT_EXPLANATIONS:
     print '=' * 80
     print 'Target is stale: %s' % record_path
     print changes.DescribeDifference()
diff --git a/build/config/android/config.gni b/build/config/android/config.gni
index 7abf51a..7ca1b70 100644
--- a/build/config/android/config.gni
+++ b/build/config/android/config.gni
@@ -71,10 +71,16 @@ if (is_android) {
     disable_incremental_isolated_processes = false
 
     # Speed up incremental compiles by compiling only changed files.
-    incremental_javac = false
+    enable_incremental_javac = false
 
     # Speed up dexing using dx --incremental.
-    incremental_dx = true
+    enable_incremental_dx = is_debug
+
+    # Neither of these should ever be used for release builds since they are
+    # somewhat experimental and dx --incremental is known to not produce
+    # byte-for-byte identical output.
+    assert(!(enable_incremental_dx && !is_debug))
+    assert(!(enable_incremental_javac && !is_debug))
 
     # Adds intrumentation to each function. Writes a file with the order that
     # functions are called at startup.
diff --git a/build/config/android/internal_rules.gni b/build/config/android/internal_rules.gni
index 0a8f64d..f9cdaf2 100644
--- a/build/config/android/internal_rules.gni
+++ b/build/config/android/internal_rules.gni
@@ -199,7 +199,7 @@ template("dex") {
       rebased_output,
     ]
 
-    if (incremental_dx) {
+    if (enable_incremental_dx) {
       args += [ "--incremental" ]
     }
 
@@ -1048,6 +1048,11 @@ template("compile_java") {
     _enable_errorprone = invoker.enable_errorprone
   }
 
+  _enable_incremental_javac = enable_incremental_javac
+  if (defined(invoker.enable_incremental_javac)) {
+    _enable_incremental_javac = invoker.enable_incremental_javac
+  }
+
   _manifest_entries = []
   if (defined(invoker.manifest_entries)) {
     _manifest_entries = invoker.manifest_entries
@@ -1107,8 +1112,10 @@ template("compile_java") {
       "--java-srcjars=@FileArg($_rebased_build_config:javac:srcjars)",
       "--jar-excluded-classes=$_jar_excluded_patterns",
     ]
-    if (incremental_javac) {
+    if (_enable_incremental_javac) {
       args += [ "--incremental" ]
+      deps += [ "//third_party/jmake" ]
+      outputs += [ "${_intermediate_jar_path}.pdb" ]
     }
     if (_supports_android) {
       deps += [ "//build/android:android_ijar" ]
@@ -1276,6 +1283,7 @@ template("java_library_impl") {
                            [
                              "dist_jar_path",
                              "enable_errorprone",
+                             "enable_incremental_javac",
                              "jar_excluded_patterns",
                              "manifest_entries",
                              "proguard_config",
diff --git a/build/config/android/rules.gni b/build/config/android/rules.gni
index dee6116..16796b6 100644
--- a/build/config/android/rules.gni
+++ b/build/config/android/rules.gni
@@ -817,6 +817,7 @@ template("java_strings_grd_prebuilt") {
 #     android_library target, for example.
 #   chromium_code: If true, extra analysis warning/errors will be enabled.
 #   enable_errorprone: If true, enables the errorprone compiler.
+#   enable_incremental_javac: Overrides the global enable_incremental_javac.
 #   main_class: When specified, a wrapper script is created within
 #     $target_out_dir/bin to launch the binary with the given class as the
 #     entrypoint.
@@ -905,6 +906,7 @@ template("junit_binary") {
 #
 #   chromium_code: If true, extra analysis warning/errors will be enabled.
 #   enable_errorprone: If true, enables the errorprone compiler.
+#   enable_incremental_javac: Overrides the global enable_incremental_javac.
 #
 #   jar_excluded_patterns: List of patterns of .class files to exclude from the
 #     final jar.
@@ -996,6 +998,7 @@ template("java_prebuilt") {
 #
 #   chromium_code: If true, extra analysis warning/errors will be enabled.
 #   enable_errorprone: If true, enables the errorprone compiler.
+#   enable_incremental_javac: Overrides the global enable_incremental_javac.
 #
 #   jar_excluded_patterns: List of patterns of .class files to exclude from the
 #     final jar.
@@ -1799,7 +1802,9 @@ template("unittest_apk") {
     if (defined(invoker.deps)) {
       deps += invoker.deps
     }
-    data_deps = [ "//tools/android/md5sum" ]
+    data_deps = [
+      "//tools/android/md5sum",
+    ]
     if (host_os == "linux") {
       data_deps += [ "//tools/android/forwarder2" ]
     }