Android: Improve error messages from bytecode_processor

1. Add error banner
2. Show the failing target
3. Show which targets to add rather than which .jar files to add
4. Show all errors rather than the first from each class.

Example error:
============================= Dependency Checks Failed =============================
Target: //base:base_java
Direct classpath is incomplete. To fix, add deps on:
 * com_android_support_support_compat_java
     * android.support.v4.view.ViewCompat (needed by org.chromium.base.ApiCompatibilityUtils and 1 more)

Bug: None
Change-Id: I26c6ea94d4df4167737519ad181131f8efecc00b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208813
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771049}

build/android/bytecode/java/org/chromium/bytecode/ByteCodeProcessor.java

@@ -18,7 +18,9 @@ import java.net.URLClassLoader;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
@@ -40,6 +42,7 @@ class ByteCodeProcessor {
     private static ClassLoader sFullClassPathClassLoader;
     private static Set<String> sFullClassPathJarPaths;
     private static Set<String> sMissingClassesAllowlist;
+    private static Map<String, String> sJarToGnTarget;
     private static ClassPathValidator sValidator;
 
     private static Void processEntry(ZipEntry entry, byte[] data) {
@@ -55,7 +58,7 @@ class ByteCodeProcessor {
         return null;
     }
 
-    private static void process(String inputJarPath)
+    private static void process(String gnTarget, String inputJarPath)
             throws ExecutionException, InterruptedException {
         ExecutorService executorService =
                 Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
@@ -76,7 +79,7 @@ class ByteCodeProcessor {
         }
 
         if (sValidator.hasErrors()) {
-            sValidator.printAll();
+            sValidator.printAll(gnTarget, sJarToGnTarget);
             System.exit(1);
         }
     }
@@ -122,6 +125,7 @@ class ByteCodeProcessor {
                                                   ExecutionException, InterruptedException {
         // Invoke this script using //build/android/gyp/bytecode_processor.py
         int currIndex = 0;
+        String gnTarget = args[currIndex++];
         String inputJarPath = args[currIndex++];
         sVerbose = args[currIndex++].equals("--verbose");
         sIsPrebuilt = args[currIndex++].equals("--is-prebuilt");
@@ -138,19 +142,26 @@ class ByteCodeProcessor {
         currIndex = parseListArgument(args, currIndex, directClassPathJarPaths);
         sDirectClassPathClassLoader = loadJars(directClassPathJarPaths);
 
+        ArrayList<String> fullClassPathJarPaths = new ArrayList<>();
+        currIndex = parseListArgument(args, currIndex, fullClassPathJarPaths);
+        ArrayList<String> gnTargets = new ArrayList<>();
+        parseListArgument(args, currIndex, gnTargets);
+        sJarToGnTarget = new HashMap<>();
+        assert fullClassPathJarPaths.size() == gnTargets.size();
+        for (int i = 0; i < fullClassPathJarPaths.size(); ++i) {
+            sJarToGnTarget.put(fullClassPathJarPaths.get(i), gnTargets.get(i));
+        }
+
         // Load all jars that are on the classpath for the input jar for analyzing class
         // hierarchy.
         sFullClassPathJarPaths = new HashSet<>();
-        sFullClassPathJarPaths.clear();
         sFullClassPathJarPaths.add(inputJarPath);
         sFullClassPathJarPaths.addAll(sdkJarPaths);
-        sFullClassPathJarPaths.addAll(
-                Arrays.asList(Arrays.copyOfRange(args, currIndex, args.length)));
-
+        sFullClassPathJarPaths.addAll(fullClassPathJarPaths);
         sFullClassPathClassLoader = loadJars(sFullClassPathJarPaths);
         sFullClassPathJarPaths.removeAll(directClassPathJarPaths);
 
         sValidator = new ClassPathValidator();
-        process(inputJarPath);
+        process(gnTarget, inputJarPath);
     }
 }

build/android/bytecode/java/org/chromium/bytecode/ClassPathValidator.java

@@ -13,6 +13,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.function.Consumer;
 
 /**
  * Checks classpaths (given as ClassLoaders) by reading the constant pool of the class file and
@@ -86,10 +87,10 @@ public class ClassPathValidator {
      *
      * @param classReader .class file interface for reading the constant pool.
      * @param classLoader classpath you wish to validate.
-     * @throws ClassNotLoadedException thrown if it can't load a certain class.
+     * @param errorConsumer Called for each missing class.
      */
-    private static void validateClassPath(ClassReader classReader, ClassLoader classLoader)
-            throws ClassNotLoadedException {
+    private static void validateClassPath(ClassReader classReader, ClassLoader classLoader,
+            Consumer<ClassNotLoadedException> errorConsumer) {
         char[] charBuffer = new char[classReader.getMaxStringLength()];
         // According to the Java spec, the constant pool is indexed from 1 to constant_pool_count -
         // 1. See https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
@@ -98,7 +99,11 @@ public class ClassPathValidator {
             // Class entries correspond to 7 in the constant pool
             // https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
             if (offset > 0 && classReader.readByte(offset - 1) == 7) {
-                validateClass(classLoader, classReader.readUTF8(offset, charBuffer));
+                try {
+                    validateClass(classLoader, classReader.readUTF8(offset, charBuffer));
+                } catch (ClassNotLoadedException e) {
+                    errorConsumer.accept(e);
+                }
             }
         }
     }
@@ -106,21 +111,17 @@ public class ClassPathValidator {
     public void validateFullClassPath(ClassReader classReader, ClassLoader fullClassLoader,
             Set<String> missingClassAllowlist) {
         // Prebuilts only need transitive dependencies checked, not direct dependencies.
-        try {
-            validateClassPath(classReader, fullClassLoader);
-        } catch (ClassNotLoadedException e) {
+        validateClassPath(classReader, fullClassLoader, (e) -> {
             if (!missingClassAllowlist.contains(e.getClassName())) {
                 addMissingError(classReader.getClassName(), e.getClassName());
             }
-        }
+        });
     }
 
     public void validateDirectClassPath(ClassReader classReader, ClassLoader directClassLoader,
             ClassLoader fullClassLoader, Collection<String> jarsOnlyInFullClassPath,
             Set<String> missingClassAllowlist, boolean verbose) {
-        try {
-            validateClassPath(classReader, directClassLoader);
-        } catch (ClassNotLoadedException e) {
+        validateClassPath(classReader, directClassLoader, (e) -> {
             try {
                 validateClass(fullClassLoader, e.getClassName());
             } catch (ClassNotLoadedException d) {
@@ -146,7 +147,7 @@ public class ClassPathValidator {
                 } catch (ClassNotLoadedException f) {
                 }
             }
-        }
+        });
     }
 
     private void addMissingError(String srcClass, String missingClass) {
@@ -175,9 +176,9 @@ public class ClassPathValidator {
     }
 
     private static void printValidationError(
-            PrintStream out, String jarName, Map<String, Set<String>> missingClasses) {
+            PrintStream out, String gnTarget, Map<String, Set<String>> missingClasses) {
         out.print(" * ");
-        out.println(jarName);
+        out.println(gnTarget);
         int i = 0;
         // The list of missing classes is non-exhaustive because each class that fails to validate
         // reports only the first missing class.
@@ -201,10 +202,11 @@ public class ClassPathValidator {
         }
     }
 
-    public void printAll() {
+    public void printAll(String gnTarget, Map<String, String> jarToGnTarget) {
         String streamer = "=============================";
         System.err.println();
         System.err.println(streamer + " Dependency Checks Failed " + streamer);
+        System.err.println("Target: " + gnTarget);
         if (!mMissingClasses.isEmpty()) {
             int i = 0;
             for (Map.Entry<String, String> entry : mMissingClasses.entrySet()) {
@@ -220,10 +222,10 @@ public class ClassPathValidator {
             System.err.println();
         }
         if (!mDirectErrors.isEmpty()) {
-            System.err.println("Direct classpath is incomplete. To fix, add deps on the "
-                    + "GN target(s) that provide:");
+            System.err.println("Direct classpath is incomplete. To fix, add deps on:");
             for (Map.Entry<String, Map<String, Set<String>>> entry : mDirectErrors.entrySet()) {
-                printValidationError(System.err, entry.getKey(), entry.getValue());
+                printValidationError(
+                        System.err, jarToGnTarget.get(entry.getKey()), entry.getValue());
             }
             System.err.println();
         }

build/android/gyp/bytecode_processor.py

@@ -23,10 +23,12 @@ def main(argv):
   parser = argparse.ArgumentParser()
   parser.add_argument('--script', required=True,
                       help='Path to the java binary wrapper script.')
+  parser.add_argument('--gn-target', required=True)
   parser.add_argument('--input-jar', required=True)
   parser.add_argument('--direct-classpath-jars')
   parser.add_argument('--sdk-classpath-jars')
-  parser.add_argument('--full-classpath-jars', action='append')
+  parser.add_argument('--full-classpath-jars')
+  parser.add_argument('--full-classpath-gn-targets')
   parser.add_argument('--stamp')
   parser.add_argument('-v', '--verbose', action='store_true')
   parser.add_argument('--missing-classes-allowlist')
@@ -37,19 +39,24 @@ def main(argv):
   args.direct_classpath_jars = build_utils.ParseGnList(
       args.direct_classpath_jars)
   args.full_classpath_jars = build_utils.ParseGnList(args.full_classpath_jars)
+  args.full_classpath_gn_targets = build_utils.ParseGnList(
+      args.full_classpath_gn_targets)
   args.missing_classes_allowlist = build_utils.ParseGnList(
       args.missing_classes_allowlist)
 
   verbose = '--verbose' if args.verbose else '--not-verbose'
 
-  cmd = [args.script, args.input_jar, verbose, args.is_prebuilt]
+  cmd = [args.script, args.gn_target, args.input_jar, verbose, args.is_prebuilt]
   cmd += [str(len(args.missing_classes_allowlist))]
   cmd += args.missing_classes_allowlist
   cmd += [str(len(args.sdk_classpath_jars))]
   cmd += args.sdk_classpath_jars
   cmd += [str(len(args.direct_classpath_jars))]
   cmd += args.direct_classpath_jars
+  cmd += [str(len(args.full_classpath_jars))]
   cmd += args.full_classpath_jars
+  cmd += [str(len(args.full_classpath_gn_targets))]
+  cmd += args.full_classpath_gn_targets
   subprocess.check_call(cmd)
 
   if args.stamp:

build/android/gyp/write_build_config.py

@@ -837,6 +837,15 @@ def _CreateJavaLocaleListFromAssets(assets, locale_paks):
   return '{%s}' % ','.join(['"%s"' % l for l in sorted(locales)])
 
 
+def _AddJarMapping(jar_to_target, configs):
+  for config in configs:
+    jar = config.get('unprocessed_jar_path')
+    if jar:
+      jar_to_target[jar] = config['gn_target']
+    for jar in config.get('extra_classpath_jars', []):
+      jar_to_target[jar] = config['gn_target']
+
+
 def main(argv):
   parser = optparse.OptionParser()
   build_utils.AddDepfileOption(parser)
@@ -844,6 +853,7 @@ def main(argv):
   parser.add_option(
       '--type',
       help='Type of this target (e.g. android_library).')
+  parser.add_option('--gn-target', help='GN label for this target')
   parser.add_option(
       '--deps-configs',
       help='GN-list of dependent build_config files.')
@@ -1151,6 +1161,7 @@ def main(argv):
           'name': os.path.basename(options.build_config),
           'path': options.build_config,
           'type': options.type,
+          'gn_target': options.gn_target,
           'deps_configs': deps.direct_deps_config_paths,
           'chromium_code': not options.non_chromium_code,
       },
@@ -1402,7 +1413,7 @@ def main(argv):
     # Adding base module to classpath to compile against its R.java file
     if base_module_build_config:
       javac_full_classpath.append(
-          base_module_build_config['deps_info']['jar_path'])
+          base_module_build_config['deps_info']['unprocessed_jar_path'])
       javac_full_interface_classpath.append(
           base_module_build_config['deps_info']['interface_jar_path'])
       jetified_full_jar_classpath.append(
@@ -1855,6 +1866,25 @@ def main(argv):
     RemoveObjDups(config, base, 'final_dex', 'all_dex_files')
     RemoveObjDups(config, base, 'extra_android_manifests')
 
+  if is_java_target:
+    jar_to_target = {}
+    _AddJarMapping(jar_to_target, [deps_info])
+    _AddJarMapping(jar_to_target, deps.all_deps_configs)
+    if base_module_build_config:
+      _AddJarMapping(jar_to_target, [base_module_build_config['deps_info']])
+    if options.tested_apk_config:
+      _AddJarMapping(jar_to_target, [tested_apk_config])
+      for jar, target in itertools.izip(
+          tested_apk_config['javac_full_classpath'],
+          tested_apk_config['javac_full_classpath_targets']):
+        jar_to_target[jar] = target
+
+    # Used by bytecode_processor to give better error message when missing
+    # deps are found.
+    config['deps_info']['javac_full_classpath_targets'] = [
+        jar_to_target[x] for x in deps_info['javac_full_classpath']
+    ]
+
   build_utils.WriteJson(config, options.build_config, only_if_changed=True)
 
   if options.depfile:

build/config/android/internal_rules.gni

@@ -120,21 +120,25 @@ build_config_target_suffix = "__build_config_crbug_908819"
 # build/android/gyp/util/build_utils.py:ExpandFileArgs
 template("write_build_config") {
   _type = invoker.type
+  _parent_invoker = invoker.invoker
+  _target_label =
+      get_label_info(":${_parent_invoker.target_name}", "label_no_toolchain")
 
   # Don't need to enforce naming scheme for these targets since we never
   # consider them in dependency chains.
   if (_type != "android_apk" && _type != "java_binary" && _type != "dist_jar" &&
       _type != "java_annotation_processor" && _type != "dist_aar" &&
       _type != "android_app_bundle") {
-    _parent_invoker = invoker.invoker
-    _target_label =
-        get_label_info(":${_parent_invoker.target_name}", "label_no_toolchain")
     if (filter_exclude([ _target_label ], _java_target_patterns) != [] &&
         filter_exclude([ _target_label ], _java_target_exceptions) != []) {
       assert(false, "Invalid java target name: $_target_label")
     }
   }
 
+  if (defined(invoker.public_target_label)) {
+    _target_label = invoker.public_target_label
+  }
+
   action_with_pydeps(target_name) {
     forward_variables_from(invoker,
                            [
@@ -156,12 +160,12 @@ template("write_build_config") {
     _deps_configs = []
     if (defined(invoker.possible_config_deps)) {
       foreach(_possible_dep, invoker.possible_config_deps) {
-        _target_label = get_label_info(_possible_dep, "label_no_toolchain")
-        if (filter_exclude([ _target_label ], _java_target_patterns) == [] &&
-            filter_exclude([ _target_label ], _java_target_exceptions) != []) {
+        _dep_label = get_label_info(_possible_dep, "label_no_toolchain")
+        if (filter_exclude([ _dep_label ], _java_target_patterns) == [] &&
+            filter_exclude([ _dep_label ], _java_target_exceptions) != []) {
           # Put the bug number in the target name so that false-positives
           # have a hint in the error message about non-existent dependencies.
-          deps += [ "$_target_label$build_config_target_suffix" ]
+          deps += [ "$_dep_label$build_config_target_suffix" ]
           _dep_gen_dir = get_label_info(_possible_dep, "target_gen_dir")
           _dep_name = get_label_info(_possible_dep, "name")
           _deps_configs += [ "$_dep_gen_dir/$_dep_name.build_config" ]
@@ -177,6 +181,8 @@ template("write_build_config") {
       "--deps-configs=$_rebased_deps_configs",
       "--build-config",
       rebase_path(invoker.build_config, root_build_dir),
+      "--gn-target",
+      _target_label,
     ]
 
     if (defined(invoker.chromium_code) && !invoker.chromium_code) {
@@ -221,10 +227,10 @@ template("write_build_config") {
         invoker.annotation_processor_deps != []) {
       _processor_configs = []
       foreach(_processor_dep, invoker.annotation_processor_deps) {
-        _target_label = get_label_info(_processor_dep, "label_no_toolchain")
+        _dep_label = get_label_info(_processor_dep, "label_no_toolchain")
         _dep_gen_dir = get_label_info(_processor_dep, "target_gen_dir")
         _dep_name = get_label_info(_processor_dep, "name")
-        deps += [ "$_target_label$build_config_target_suffix" ]
+        deps += [ "$_dep_label$build_config_target_suffix" ]
         _processor_configs += [ "$_dep_gen_dir/$_dep_name.build_config" ]
       }
       _rebased_processor_configs =
@@ -472,12 +478,12 @@ template("write_build_config") {
     }
     if (defined(invoker.static_library_dependent_targets)) {
       _dependent_configs = []
-      foreach(_target, invoker.static_library_dependent_targets) {
-        _target_name = _target.name
-        _target_label = get_label_info(_target_name, "label_no_toolchain")
-        deps += [ "$_target_label$build_config_target_suffix" ]
-        _dep_gen_dir = get_label_info(_target_name, "target_gen_dir")
-        _dep_name = get_label_info(_target_name, "name")
+      foreach(_dep, invoker.static_library_dependent_targets) {
+        _dep_name = _dep.name
+        _dep_label = get_label_info(_dep_name, "label_no_toolchain")
+        deps += [ "$_dep_label$build_config_target_suffix" ]
+        _dep_gen_dir = get_label_info(_dep_name, "target_gen_dir")
+        _dep_name = get_label_info(_dep_name, "name")
         _config =
             rebase_path("$_dep_gen_dir/$_dep_name.build_config", root_build_dir)
         _dependent_configs += [ _config ]
@@ -495,11 +501,11 @@ template("write_build_config") {
       ]
     }
     if (defined(invoker.base_module_target)) {
-      _target_label =
+      _base_label =
           get_label_info(invoker.base_module_target, "label_no_toolchain")
-      _dep_gen_dir = get_label_info(_target_label, "target_gen_dir")
-      _dep_name = get_label_info(_target_label, "name")
-      deps += [ "$_target_label$build_config_target_suffix" ]
+      _dep_gen_dir = get_label_info(_base_label, "target_gen_dir")
+      _dep_name = get_label_info(_base_label, "name")
+      deps += [ "$_base_label$build_config_target_suffix" ]
       args += [
         "--base-module-build-config",
         rebase_path("$_dep_gen_dir/$_dep_name.build_config", root_build_dir),
@@ -524,7 +530,8 @@ template("write_build_config") {
       # }
       _msg = [
         "Tried to build an Android target in a non-default toolchain.",
-        "target: " + get_label_info(":$target_name", "label_with_toolchain"),
+        "target: $_target_label",
+        "current_toolchain: $current_toolchain",
         "default_toolchain: $default_toolchain",
       ]
       args += [ "--fail=$_msg" ]
@@ -1878,14 +1885,14 @@ if (enable_java_templates) {
       args = [
         "--script",
         rebase_path(_bytecode_checker_script, root_build_dir),
+        "--gn-target=${invoker.target_label}",
         "--input-jar",
         rebase_path(invoker.input_jar, root_build_dir),
         "--stamp",
         rebase_path(outputs[0], root_build_dir),
-        "--direct-classpath-jars",
-        "@FileArg($_rebased_build_config:javac:classpath)",
-        "--full-classpath-jars",
-        "@FileArg($_rebased_build_config:deps_info:javac_full_classpath)",
+        "--direct-classpath-jars=@FileArg($_rebased_build_config:javac:classpath)",
+        "--full-classpath-jars=@FileArg($_rebased_build_config:deps_info:javac_full_classpath)",
+        "--full-classpath-gn-targets=@FileArg($_rebased_build_config:deps_info:javac_full_classpath_targets)",
       ]
       if (invoker.is_prebuilt) {
         args += [ "--is-prebuilt" ]
@@ -3469,6 +3476,7 @@ if (enable_java_templates) {
                                "gradle_treat_as_prebuilt",
                                "input_jars_paths",
                                "main_class",
+                               "public_target_label",
                                "proguard_configs",
                                "proguard_enabled",
                                "proguard_mapping_path",
@@ -3770,6 +3778,8 @@ if (enable_java_templates) {
               deps += [ ":$_compile_java_target" ]
             }
             requires_android = _requires_android
+            target_label =
+                get_label_info(":${invoker.target_name}", "label_no_toolchain")
             input_jar = _unprocessed_jar_path
             build_config = _build_config
             is_prebuilt = _is_prebuilt

build/config/android/rules.gni

@@ -4313,6 +4313,7 @@ if (enable_java_templates) {
         jar_path = "$_output_path/${_tuple[1]}"
         _base_output_name = get_path_info(jar_path, "name")
         output_name = "${invoker.target_name}-$_base_output_name"
+        public_target_label = invoker.target_name
       }
     }
 
@@ -4347,6 +4348,7 @@ if (enable_java_templates) {
           }
           proguard_configs += [ "$_output_path/proguard.txt" ]
         }
+        public_target_label = invoker.target_name
       }
     }