Add Trichrome support to bundle library allotment

Previously, the bundle allotment step didn't account for libraries
packaged into the Trichrome library and, thus, duplicated many libraries
of the Trichrome library into the Trichrome Chrome bundle. This CL fixes
this by teaching the library allotment step about the Trichrome
dependency tree so that it can allot the libraries accordingly. In fact,
with this CL, the library allotment step can now handle any arbitrary
dependency tree. The Trichrome dependency tree looks as follows:

         lib
          |
        base
       /  |  \
  dev_ui  vr  etc.

Note that webview is not yet part of the computation. See
crbug.com/1021565 for more details.

Bug: 1020717, 1021565
Change-Id: I651bcfd1bd15e36b4028b3f535112d3233be2dab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898132
Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712775}

build/android/gyp/allot_native_libraries.py

@@ -7,7 +7,37 @@
 """Allots libraries to modules to be packaged into.
 
 All libraries that are depended on by a single module will be allotted to this
-module. All other libraries will be allotted to base.
+module. All other libraries will be allotted to the closest ancestor.
+
+Example:
+  Given the module dependency structure
+
+        c
+       / \
+      b   d
+     /     \
+    a       e
+
+  and libraries assignment
+
+    a: ['lib1.so']
+    e: ['lib2.so', 'lib1.so']
+
+  will make the allotment decision
+
+    c: ['lib1.so']
+    e: ['lib2.so']
+
+  The above example is invoked via:
+
+    ./allot_native_libraries \
+      --libraries 'a,["1.so"]' \
+      --libraries 'e,["2.so", "1.so"]' \
+      --dep c:b \
+      --dep b:a \
+      --dep c:d \
+      --dep d:e \
+      --output <output JSON>
 """
 
 import argparse
@@ -24,6 +54,74 @@ def _ModuleLibrariesPair(arg):
   return (arg[:pos], arg[pos + 1:])
 
 
+def _DepPair(arg):
+  parent, child = arg.split(':')
+  return (parent, child)
+
+
+def _PathFromRoot(module_tree, module):
+  """Computes path from root to a module.
+
+  Parameters:
+    module_tree: Dictionary mapping each module to its parent.
+    module: Module to which to compute the path.
+
+  Returns:
+    Path from root the the module.
+  """
+  path = [module]
+  while module_tree.get(module):
+    module = module_tree[module]
+    path = [module] + path
+  return path
+
+
+def _ClosestCommonAncestor(module_tree, modules):
+  """Computes the common ancestor of a set of modules.
+
+  Parameters:
+    module_tree: Dictionary mapping each module to its parent.
+    modules: Set of modules for which to find the closest common ancestor.
+
+  Returns:
+    The closest common ancestor.
+  """
+  paths = [_PathFromRoot(module_tree, m) for m in modules]
+  assert len(paths) > 0
+  ancestor = None
+  for level in zip(*paths):
+    if len(set(level)) != 1:
+      return ancestor
+    ancestor = level[0]
+  return ancestor
+
+
+def _AllotLibraries(module_tree, libraries_map):
+  """Allot all libraries to a module.
+
+  Parameters:
+    module_tree: Dictionary mapping each module to its parent. Modules can map
+      to None, which is considered the root of the tree.
+    libraries_map: Dictionary mapping each library to a set of modules, which
+      depend on the library.
+
+  Returns:
+    A dictionary mapping mapping each module name to a set of libraries allotted
+    to the module such that libraries with multiple dependees are allotted to
+    the closest ancestor.
+
+  Raises:
+    Exception if some libraries can only be allotted to the None root.
+  """
+  allotment_map = collections.defaultdict(set)
+  for library, modules in libraries_map.items():
+    ancestor = _ClosestCommonAncestor(module_tree, modules)
+    if not ancestor:
+      raise Exception('Cannot allot libraries for given dependency tree')
+    allotment_map[ancestor].add(library)
+  return allotment_map
+
+
 def main(args):
   parser = argparse.ArgumentParser()
   parser.add_argument(
@@ -32,37 +130,52 @@ def main(args):
       type=_ModuleLibrariesPair,
       required=True,
       help='A pair of module name and GN list of libraries a module depends '
-      'on. Can be specified multiple times. Must be set at least once for '
-      'base.')
+      'on. Can be specified multiple times.')
   parser.add_argument(
       '--output',
       required=True,
       help='A JSON file with a key for each module mapping to a list of '
       'libraries, which should be packaged into this module.')
+  parser.add_argument(
+      '--dep',
+      action='append',
+      type=_DepPair,
+      dest='deps',
+      default=[],
+      help='A pair of parent module name and child module name '
+      '(format: "<parent>:<child>"). Can be specified multiple times.')
   options = parser.parse_args(build_utils.ExpandFileArgs(args))
   options.libraries = [(m, build_utils.ParseGnList(l))
                        for m, l in options.libraries]
 
-  # Parse input to map and count how many modules depend on each library.
-  libraries_map = {}
-  libraries_counter = collections.Counter()  # Modules count for each library.
+  # Parse input creating libraries and dependency tree.
+  libraries_map = collections.defaultdict(set)  # Maps each library to its
+  #                                               dependee modules.
+  module_tree = {}  # Maps each module name to its parent.
   for module, libraries in options.libraries:
-    if module not in libraries_map:
-      libraries_map[module] = set()
-    libraries_map[module].update(libraries)
-    libraries_counter.update(libraries)
+    module_tree[module] = None
+    for library in libraries:
+      libraries_map[library].add(module)
+  for parent, child in options.deps:
+    if module_tree.get(child):
+      raise Exception('%s cannot have multiple parents' % child)
+    module_tree[child] = parent
+    module_tree[parent] = module_tree.get(parent)
 
-  assert 'base' in libraries_map
+  # Allot all libraries to a module such that libraries with multiple dependees
+  # are allotted to the closest ancestor.
+  allotment_map = _AllotLibraries(module_tree, libraries_map)
 
-  # Allot libraries depended on by multiple modules to base.
-  multidep_libraries = {l for l, c in libraries_counter.items() if c > 1}
-  libraries_map = {m: l - multidep_libraries for m, l in libraries_map.items()}
-  libraries_map['base'] |= multidep_libraries
+  # The build system expects there to be a set of libraries even for the modules
+  # that don't have any libraries allotted.
+  for module in module_tree:
+    # Creates missing sets because of defaultdict.
+    allotment_map[module] = allotment_map[module]
 
   with open(options.output, 'w') as f:
     # Write native libraries config and ensure the output is deterministic.
     json.dump({m: sorted(l)
-               for m, l in libraries_map.items()},
+               for m, l in allotment_map.items()},
               f,
               sort_keys=True,
               indent=2)

build/config/android/internal_rules.gni

@@ -3769,11 +3769,13 @@ template("dexsplitter") {
 # Allots native libraries depended on by feature modules to the module the
 # libraries should be packaged into. The packaging module may be different from
 # the dependee module in case a library is depended on by multiple modules. In
-# that case the library will be allotted to base.
+# that case the library will be allotted to the closest ancestor given a module
+# dependency tree (see |parent| below).
 #
 # Variables:
 #   modules: List of scopes with the following format:
 #     name: The module's name.
+#     parent: The module's parent's name.
 #     build_config: Path to the module's build config.
 #     build_config_target: Target creating |build_config|.
 #   native_libraries_filearg_keys: Keys to be used in
@@ -3805,6 +3807,12 @@ template("allot_native_libraries") {
           "${_module.name},@FileArg($_rebased_build_config:$_key)",
         ]
       }
+      if (defined(_module.parent)) {
+        args += [
+          "--dep",
+          "${_module.parent}:${_module.name}",
+        ]
+      }
     }
   }
 }

build/config/android/rules.gni

@@ -4349,7 +4349,15 @@ if (enable_java_templates) {
   #
   #    static_library_provider: Specifies a single target that this target will
   #      use as a static library APK. When proguard is enabled, the
-  #      static_library_provider target will perform the synchronized dex step.
+  #      static_library_provider target will perform the synchronized dex step
+  #      unless |static_library_proguard_disabled| is set.
+  #      Additionally, when allotting libraries to be packaged into modules, the
+  #      libraries packaged into the static library will be accounted for to
+  #      avoid library duplication. Effectively, the static library will be
+  #      treated as the parent of the base module.
+  #
+  #    static_library_proguard_disabled: If true will not use the
+  #      |static_library_provider| to perform synchronized proguarding.
   #
   #    verify_proguard_flags: Enables verification of expected merged proguard
   #      flags based on a golden file.
@@ -4370,6 +4378,16 @@ if (enable_java_templates) {
   #
   template("android_app_bundle") {
     _target_name = target_name
+    _uses_static_library = defined(invoker.static_library_provider)
+    _proguard_enabled =
+        defined(invoker.proguard_enabled) && invoker.proguard_enabled
+    if (_proguard_enabled) {
+      _static_library_proguard_disabled =
+          defined(invoker.static_library_proguard_disabled) &&
+          invoker.static_library_proguard_disabled
+    } else {
+      assert(!defined(invoker.static_library_proguard_disabled))
+    }
 
     _bundle_base_path = "$root_build_dir/apks"
     if (defined(invoker.bundle_base_path)) {
@@ -4399,11 +4417,12 @@ if (enable_java_templates) {
         module_target = invoker.base_module_target
         build_config = _base_module_build_config
         build_config_target = _base_module_build_config_target
+        if (_uses_static_library) {
+          parent = "lib"
+        }
       },
     ]
 
-    _proguard_enabled =
-        defined(invoker.proguard_enabled) && invoker.proguard_enabled
     _enable_multidex =
         !defined(invoker.enable_multidex) || invoker.enable_multidex
 
@@ -4415,8 +4434,7 @@ if (enable_java_templates) {
     not_needed([ "_enable_multidex" ])
 
     if (_proguard_enabled) {
-      _uses_static_library = defined(invoker.static_library_provider)
-      if (_uses_static_library) {
+      if (!_static_library_proguard_disabled && _uses_static_library) {
         _sync_dex_target_dep = "${invoker.static_library_provider}__dexsplitter"
       } else {
         _sync_dex_target = "${_target_name}__sync_dex"
@@ -4449,6 +4467,7 @@ if (enable_java_templates) {
             "$_module_target_gen_dir/${_module_target_name}.build_config"
         _module.build_config_target =
             "$_module_target$build_config_target_suffix"
+        _module.parent = "base"
         _modules += [ _module ]
       }
     }
@@ -4463,9 +4482,26 @@ if (enable_java_templates) {
       _module_build_configs += [ _module.build_config ]
     }
 
+    if (_uses_static_library) {
+      _lib_proxy_module = {
+        name = "lib"
+      }
+      _static_library_target_name =
+          get_label_info(invoker.static_library_provider, "name")
+      _static_library_gen_dir =
+          get_label_info(invoker.static_library_provider, "target_gen_dir")
+      _lib_proxy_module.build_config =
+          "$_static_library_gen_dir/$_static_library_target_name.build_config"
+      _lib_proxy_module.build_config_target =
+          "${invoker.static_library_provider}$build_config_target_suffix"
+    }
+
     # Allot native libraries to modules they should be packaged into. This is
     # necessary since all libraries that are depended on by multiple modules
-    # have to go into base.
+    # have to go into base or the static shared library if it exists.
+    # TODO(crbug.com/1021565): It would be nice if this lived outside the
+    # android_app_bundle template and the static shared library would pull in
+    # the libs as allotted by this step.
     _native_libraries_config =
         "$target_gen_dir/$_target_name.native_libraries_config"
     allot_native_libraries("${_target_name}__allot_native_libraries") {
@@ -4475,6 +4511,9 @@ if (enable_java_templates) {
         "native:extra_shared_libraries",
       ]
       output = _native_libraries_config
+      if (_uses_static_library) {
+        modules += [ _lib_proxy_module ]
+      }
     }
     if (defined(android_app_secondary_abi)) {
       _secondary_abi_native_libraries_config =
@@ -4484,6 +4523,9 @@ if (enable_java_templates) {
         modules = _modules
         native_libraries_filearg_keys = [ "native:secondary_abi_libraries" ]
         output = _secondary_abi_native_libraries_config
+        if (_uses_static_library) {
+          modules += [ _lib_proxy_module ]
+        }
       }
     }
 
@@ -4529,7 +4571,7 @@ if (enable_java_templates) {
     if (_proguard_enabled) {
       # If this Bundle uses a static library, the static library APK will
       # create the synchronized dex file path.
-      if (_uses_static_library) {
+      if (!_static_library_proguard_disabled && _uses_static_library) {
         if (defined(invoker.min_sdk_version)) {
           not_needed(invoker, [ "min_sdk_version" ])
         }

chrome/android/BUILD.gn

@@ -2434,6 +2434,11 @@ template("monochrome_or_trichrome_public_bundle_tmpl") {
   }
 
   chrome_bundle(target_name) {
+    forward_variables_from(invoker,
+                           [
+                             "static_library_proguard_disabled",
+                             "static_library_provider",
+                           ])
     base_module_target = ":$_base_module_target_name"
     bundle_name = _bundle_name
     include_32_bit_webview = defined(invoker.include_32_bit_webview) &&
@@ -2497,6 +2502,10 @@ if (public_android_sdk) {
   monochrome_or_trichrome_public_bundle_tmpl("trichrome_chrome_bundle") {
     bundle_suffix = ""
     use_trichrome_library = true
+    static_library_provider = ":trichrome_library_for_bundle_apk"
+    if (!is_java_debug) {
+      static_library_proguard_disabled = !trichrome_synchronized_proguard
+    }
   }
 
   if (android_64bit_target_cpu) {

chrome/android/chrome_bundle_tmpl.gni

@@ -80,6 +80,7 @@ template("chrome_bundle") {
                              "proguard_android_sdk_dep",
                              "proguard_jar_path",
                              "sign_bundle",
+                             "static_library_proguard_disabled",
                              "static_library_provider",
                              "verify_proguard_flags",
                            ])