Android: Improve type filtering, add __assetres (reland)

Add _java_resource_types, _java_library_types, and _java_leaf_types.
These types, together with "group", define all existing invoker.type
values for all java targets. Since leaf types like android_apk are not
used as dependencies for other java targets, all java targets in deps
trees can be defined as either resources, libraries, or groups.

Naming conventions have also been improved. Now java targets must match
_java_resource_patterns if the target is a resource target. Otherwise,
it must match _java_library_patterns if the target is a library target.
Group targets can match either of those, but should prefer matching
_java_library_patterns since they are treated as library targets in the
java_group template.

Thus, target filtering is now simplified. To filter for resource targets
, simply use _java_resource_patterns. To filter for library targets, use
_java_library_patterns and ignore the ones that also match
_java_resource_patterns. To filter for any java target, use
_java_target_patterns, which is a simple concatenation of the other two
lists of patterns.

The assert in write_build_config has been updated to enforce naming
schemes accordingly. This enables assumptions about library and resource
targets such as __header, __impl, and the newly added __assetres, which
is defined on library targets to be its transitive list of all resource
target deps.

Now compile_resources targets can filter their deps so that they only
depend on transitive resource deps. This allows compile_resources to
not depend on java library targets, and unblocks header generation for
apk targets. Previously an apk's __header target would depend on
compile_resources to finish, while the compile_resources target would
depend on the rest of the java library dependencies (not headers),
completely removing any benefit of header compilation for apk targets.

Original CL: https://crrev.com/c/2220585

Fixed:
- Turns out compile_resources depends on the android sdk, and that was
  either supplied directly (in the case of android_apk_or_module) or it
  was supplied by its java dependencies (in the case of junit_binary).
- This dependency was broken by replacing java library deps with their
  corresponding __assetres deps.
- Fixed by explicitly specifying android_sdk_dep just like
  build_config_dep.
- Added comments for both of these explicit *_dep variables.

Bug: 1090812
Change-Id: I57c63ff805c4e27ff6b47bc69c0a6954b0b681c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2231178
Commit-Queue: Peter Wen <wnwen@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Auto-Submit: Peter Wen <wnwen@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775105}

build/config/android/internal_rules.gni

@@ -16,37 +16,65 @@ import("//build/util/generate_wrapper.gni")
 import("//build_overrides/build.gni")
 assert(is_android)
 
-# These filters identify most targets that eventually use the java_library_impl
-# template.
-_java_lib_patterns = [
+# The following _java_*_types variables capture all the existing target types.
+# If a new type is introduced, please add it to one of these categories,
+# preferring the more specific resource/library types.
+_java_resource_types = [
+  "android_assets",
+  "android_resources",
+]
+
+_java_library_types = [
+  "java_library",
+  "system_java_library",
+  "android_app_bundle_module",
+]
+
+# These are leaf java target types. They cannot be passed as deps to other
+# targets. Thus their naming schemes are not enforced.
+_java_leaf_types = [
+  "android_apk",
+  "android_app_bundle",
+  "dist_aar",
+  "dist_jar",
+  "java_annotation_processor",
+  "java_binary",
+  "junit_binary",
+]
+
+# All _java_resource_types targets must conform to these patterns.
+_java_resource_patterns = [
+  "*:*_assets",
+  "*android*:assets",
+  "*:*_apk_*resources",
+  "*android*:resources",
+  "*:*_resources",
+  "*:*_grd",
+  "*:*locale_paks",
+  "*:*_java_strings",
+  "*:*strings_java",
+]
+
+# All _java_library_types targets must conform to these patterns. This includes
+# all non-leaf targets that use java_library_impl.
+_java_library_patterns = [
   "*:*_java",
   "*:*_javalib",
-  "*:*_java_*",  # e.g. java_test_support
+  "*:*_java_*",  # e.g. chrome_java_test_support
   "*:java",
   "*:junit",
   "*:junit_*",
   "*:*_junit_*",
   "*:*javatests",
+  "*:*_bundle_module",
 
   # TODO(agrieve): Rename targets below to match above patterns.
-  "*android_webview/glue*:glue",
+  "//android_webview/glue/glue:glue",
+  "//android_webview/glue:glue",
 ]
 
-# These identify targets that have .build_config files (except for android_apk,
-# java_binary, android_app_bundle since we never need to depend on these).
-_java_target_patterns = _java_lib_patterns + [
-                          "*:*_assets",
-                          "*android*:assets",
-                          "*:*_apk_*resources",
-                          "*android*:resources",
-                          "*:*_resources",
-                          "*:*_grd",
-                          "*:*locale_paks",
-                          "*_bundle_module",
-                        ]
-
-# These targets match _java_lib_patterns but do not use java_library_impl.
-_java_lib_exceptions = _java_target_patterns - _java_lib_patterns
+# These identify all non-leaf targets that have .build_config files.
+_java_target_patterns = _java_library_patterns + _java_resource_patterns
 
 _r8_path = "//third_party/r8/lib/r8.jar"
 _desugar_jdk_libs_json = "//third_party/r8/desugar_jdk_libs.json"
@@ -118,14 +146,24 @@ template("write_build_config") {
   _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") {
+  # Ensure targets match naming patterns so that __assetres, __header, __impl
+  # targets work properly. Those generated targets allow for effective deps
+  # filtering.
+  if (filter_exclude([ _type ], _java_resource_types) == []) {
+    if (filter_exclude([ _target_label ], _java_resource_patterns) != []) {
+      assert(false, "Invalid java resource target name: $_target_label")
+    }
+  } else if (filter_exclude([ _type ], _java_library_types) == []) {
+    if (filter_exclude([ _target_label ], _java_library_patterns) != [] ||
+        filter_exclude([ _target_label ], _java_resource_patterns) == []) {
+      assert(false, "Invalid java library target name: $_target_label")
+    }
+  } else if (_type == "group") {
     if (filter_exclude([ _target_label ], _java_target_patterns) != []) {
       assert(false, "Invalid java target name: $_target_label")
     }
+  } else if (filter_exclude([ _type ], _java_leaf_types) != []) {
+    assert(false, "This java type needs a category: $_type")
   }
 
   if (defined(invoker.public_target_label)) {
@@ -2058,10 +2096,14 @@ if (enable_java_templates) {
   # final R.java sources for all resource packages the binary depends on.
   #
   # Input variables:
+  #   android_sdk_dep: The sdk dep that these resources should compile against.
+  #
   #   deps: Specifies the input dependencies for this target.
   #
   #   build_config: Path to the .build_config file corresponding to the target.
   #
+  #   build_config_dep: Dep target to generate the .build_config file.
+  #
   #   android_manifest: Path to root manifest for the binary.
   #
   #   version_code: (optional)
@@ -2171,7 +2213,23 @@ if (enable_java_templates) {
                              "visibility",
                            ])
 
-    _deps = invoker.deps
+    _deps = [
+      invoker.android_sdk_dep,
+      invoker.build_config_dep,
+    ]
+    if (defined(invoker.android_manifest_dep)) {
+      _deps += [ invoker.android_manifest_dep ]
+    }
+    foreach(_dep, invoker.deps) {
+      _target_label = get_label_info(_dep, "label_no_toolchain")
+      if (filter_exclude([ _target_label ], _java_library_patterns) == [] &&
+          filter_exclude([ _target_label ], _java_resource_patterns) != []) {
+        # Depend on the java libraries' transitive __assetres target instead.
+        _deps += [ "${_target_label}__assetres" ]
+      } else {
+        _deps += [ _dep ]
+      }
+    }
 
     if (defined(invoker.arsc_output)) {
       _arsc_output = invoker.arsc_output
@@ -3025,9 +3083,10 @@ if (enable_java_templates) {
         deps = []
         foreach(_dep, invoker.deps) {
           _target_label = get_label_info(_dep, "label_no_toolchain")
-          if (filter_exclude([ _target_label ], _java_lib_patterns) == [] &&
-              filter_exclude([ _target_label ], _java_lib_exceptions) != []) {
-            # This is a java dep, so replace it.
+          if (filter_exclude([ _target_label ], _java_library_patterns) == [] &&
+              filter_exclude([ _target_label ], _java_resource_patterns) !=
+              []) {
+            # This is a java library dep, so replace it.
             deps += [ "${_target_label}__${_group_name}" ]
           } else {
             # Transitive java group targets should also include direct deps.
@@ -3326,32 +3385,29 @@ if (enable_java_templates) {
       }
     }
 
-    _public_deps = []
-    _non_java_deps = []
     _java_header_deps = []
     _java_impl_deps = []
-    _java_full_deps = []
-    _analysis_public_deps = []
-    _sdk_deps = []
+    _non_java_deps = []
     if (defined(invoker.deps)) {
       foreach(_dep, invoker.deps) {
         _target_label = get_label_info(_dep, "label_no_toolchain")
-        if (filter_exclude([ _target_label ], _java_lib_patterns) == [] &&
-            filter_exclude([ _target_label ], _java_lib_exceptions) != []) {
+        if (filter_exclude([ _target_label ], _java_library_patterns) == [] &&
+            filter_exclude([ _target_label ], _java_resource_patterns) != []) {
           # This is a java dep, so replace it with its header.
           _java_header_deps += [ "${_target_label}__header" ]
           _java_impl_deps += [ "${_target_label}__impl" ]
-          _java_full_deps += [ _dep ]
         } else {
           _non_java_deps += [ _dep ]
         }
       }
     }
 
+    _extra_java_deps = []
+
     # TODO(crbug.com/1078484): Don't use desugared .jar files for java binaries.
     if (_is_java_binary && enable_bazel_desugar &&
         (!defined(invoker.enable_desugar) || invoker.enable_desugar)) {
-      _non_java_deps += [ "//third_party/bazel/desugar:desugar_runtime_java" ]
+      _extra_java_deps += [ "//third_party/bazel/desugar:desugar_runtime_java" ]
     }
 
     if (_is_prebuilt || _has_sources) {
@@ -3364,14 +3420,12 @@ if (enable_java_templates) {
             !invoker.jacoco_never_instrument && _jacoco_instrument
       }
       if (_jacoco_instrument) {
-        _non_java_deps += [ "//third_party/jacoco:jacocoagent_java" ]
+        _extra_java_deps += [ "//third_party/jacoco:jacocoagent_java" ]
       }
     }
 
     # Don't need to depend on the apk-under-test to be packaged.
     if (defined(invoker.apk_under_test)) {
-      # No need to add to _java_full_deps since that is only used for
-      # write_build_config.
       _java_header_deps += [ "${invoker.apk_under_test}__java__header" ]
       _java_impl_deps += [ "${invoker.apk_under_test}__java__impl" ]
     }
@@ -3387,12 +3441,12 @@ if (enable_java_templates) {
       }
 
       # This is an android_system_java_prebuilt target, so no headers.
-      _sdk_deps = [ _sdk_java_dep ]
+      _extra_java_deps += [ _sdk_java_dep ]
     }
 
-    _classpath_deps = _java_header_deps + _non_java_deps + _sdk_deps +
+    _classpath_deps = _java_header_deps + _non_java_deps + _extra_java_deps +
                       [ ":$_build_config_target_name" ]
-    _full_classpath_deps = _java_impl_deps + _non_java_deps + _sdk_deps +
+    _full_classpath_deps = _java_impl_deps + _non_java_deps + _extra_java_deps +
                            [ ":$_build_config_target_name" ]
 
     # Often needed, but too hard to figure out when ahead of time.
@@ -3470,7 +3524,10 @@ if (enable_java_templates) {
       enable_jetify = defined(invoker.enable_jetify) && invoker.enable_jetify
       is_prebuilt = _is_prebuilt
       jetified_jar_path = _jetified_jar_path
-      possible_config_deps = _java_full_deps + _non_java_deps + _sdk_deps
+      possible_config_deps = _extra_java_deps
+      if (defined(invoker.deps)) {
+        possible_config_deps += invoker.deps
+      }
       if (defined(apk_under_test)) {
         possible_config_deps += [ apk_under_test ]
       }
@@ -3513,6 +3570,7 @@ if (enable_java_templates) {
       _header_target_name = "${target_name}__header"
     }
 
+    _analysis_public_deps = []
     if (_has_sources) {
       if (defined(invoker.enable_errorprone)) {
         _enable_errorprone = invoker.enable_errorprone
@@ -3600,6 +3658,7 @@ if (enable_java_templates) {
       }
     }  # _has_sources
 
+    _public_deps = []
     if (_is_prebuilt || _has_sources) {
       if (_is_prebuilt) {
         generate_interface_jar(_header_target_name) {
@@ -3750,6 +3809,11 @@ if (enable_java_templates) {
       public_deps = _public_deps
     }
 
+    java_lib_group("${target_name}__assetres") {
+      forward_variables_from(invoker, [ "deps" ])
+      group_name = "assetres"
+    }
+
     group(target_name) {
       forward_variables_from(invoker,
                              [

build/config/android/rules.gni

@@ -1123,12 +1123,9 @@ if (enable_java_templates) {
   #    }
   #  }
   template("java_group") {
-    forward_variables_from(invoker,
-                           [
-                             "testonly",
-                             "input_jars_paths",
-                           ])
+    forward_variables_from(invoker, [ "testonly" ])
     write_build_config("$target_name$build_config_target_suffix") {
+      forward_variables_from(invoker, [ "input_jars_paths" ])
       type = "group"
       build_config = "$target_gen_dir/${invoker.target_name}.build_config"
       supports_android = true
@@ -1140,6 +1137,7 @@ if (enable_java_templates) {
             [
               "header",
               "impl",
+              "assetres",
             ]) {
       java_lib_group("${target_name}__${_group_name}") {
         forward_variables_from(invoker, [ "deps" ])
@@ -1346,6 +1344,11 @@ if (enable_java_templates) {
     if (defined(invoker.deps)) {
       _deps += invoker.deps
     }
+    if (defined(invoker.alternative_android_sdk_dep)) {
+      _android_sdk_dep = invoker.alternative_android_sdk_dep
+    } else {
+      _android_sdk_dep = "//third_party/android_sdk:android_sdk_java"
+    }
 
     # a package name or a manifest is required to have resources. This is
     # added so that junit tests that do not care about the package name can
@@ -1360,7 +1363,9 @@ if (enable_java_templates) {
     _compile_resources_target = "${target_name}__compile_resources"
     compile_resources(_compile_resources_target) {
       forward_variables_from(invoker, [ "android_manifest" ])
-      deps = _deps + [ ":$_build_config_target_name" ]
+      deps = _deps
+      android_sdk_dep = _android_sdk_dep
+      build_config_dep = ":$_build_config_target_name"
       build_config = _build_config
       if (defined(_package_name)) {
         rename_manifest_package = _package_name
@@ -2491,6 +2496,7 @@ if (enable_java_templates) {
       short_resource_paths = _short_resource_paths
       strip_resource_names = _strip_resource_names
       android_manifest = _android_manifest
+      android_manifest_dep = ":$_merge_manifest_target"
       version_code = _version_code
       version_name = _version_name
       min_sdk_version = _min_sdk_version
@@ -2498,8 +2504,6 @@ if (enable_java_templates) {
       if (defined(invoker.verify_manifest) && invoker.verify_manifest &&
           !is_java_debug) {
         verify_manifest_target_name = _template_name
-        build_config_dep = ":$_build_config_target"
-        android_manifest_dep = ":$_merge_manifest_target"
         forward_variables_from(invoker,
                                [ "expected_manifest_base_expectation" ])
       }
@@ -2523,11 +2527,9 @@ if (enable_java_templates) {
       }
 
       build_config = _build_config
-      deps = _deps + [
-               ":$_merge_manifest_target",
-               ":$_build_config_target",
-               _android_sdk_dep,
-             ]
+      build_config_dep = ":$_build_config_target"
+      android_sdk_dep = _android_sdk_dep
+      deps = _deps
 
       # The static library uses the R.txt files generated by the
       # static_library_dependent_targets when generating the final R.java file.