Reland "Android build: moved proguarding into dex() for most cases"

This reverts commit 0dce4507.

Reason for revert: Fixed cronet issue

Original change's description:
> Revert "Android build: moved proguarding into dex() for most cases"
> 
> This reverts commit 93aa458f.
> 
> Reason for revert: Suspect for build failure https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-rel/2802
> 
> Original change's description:
> > Android build: moved proguarding into dex() for most cases
> > 
> > This is to allow R8 to proguard and dex in one call, since it has some
> > optimizations that only work when doing everything in one shot.
> > 
> > Bug: 872904
> > Change-Id: Id2c75dac4d9feecae461a0b1e279056253f1b71b
> > Reviewed-on: https://chromium-review.googlesource.com/c/1227142
> > Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
> > Reviewed-by: agrieve <agrieve@chromium.org>
> > Commit-Queue: Sam Maier <smaier@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#597257}
> 
> TBR=yfriedman@chromium.org,agrieve@chromium.org,smaier@chromium.org
> 
> Change-Id: Idf8f855094fb1bf4c4e1b47326b76166a158c1e8
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 872904
> Reviewed-on: https://chromium-review.googlesource.com/c/1265559
> Reviewed-by: Kevin McNee <mcnee@chromium.org>
> Commit-Queue: Kevin McNee <mcnee@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597274}

TBR=yfriedman@chromium.org,agrieve@chromium.org,mcnee@chromium.org,smaier@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 872904
Change-Id: I1bebed520132e81b199c639690d36171ccb46ab6
Reviewed-on: https://chromium-review.googlesource.com/c/1271792
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598766}

build/android/gyp/proguard.py

@@ -6,6 +6,7 @@
 
 import optparse
 import os
+import shutil
 import sys
 import tempfile
 
@@ -56,15 +57,23 @@ def _ParseOptions(args):
                          'included by --proguard-configs, but that should '
                          'not actually be included.')
   parser.add_option('--mapping', help='Path to proguard mapping to apply.')
+  parser.add_option('--mapping-output',
+                    help='Path for proguard to output mapping file to.')
   parser.add_option('--classpath', action='append',
                     help='Classpath for proguard.')
   parser.add_option('--enable-dangerous-optimizations', action='store_true',
                     help='Enable optimizations which are known to have issues.')
+  parser.add_option('--main-dex-rules-path', action='append',
+                    help='Paths to main dex rules for multidex'
+                         '- only works with R8.')
   parser.add_option('--verbose', '-v', action='store_true',
                     help='Print all proguard output')
 
   options, _ = parser.parse_args(args)
 
+  assert not options.main_dex_rules_path or options.r8_path, \
+      "R8 must be enabled to pass main dex rules."
+
   classpath = []
   for arg in options.classpath:
     classpath += build_utils.ParseGnList(arg)
@@ -79,16 +88,38 @@ def _ParseOptions(args):
 
   options.input_paths = build_utils.ParseGnList(options.input_paths)
 
+  if not options.mapping_output:
+    options.mapping_output = options.output_path + ".mapping"
+
   return options
 
 
-def _CreateR8Command(options, map_output_path):
+def _MoveTempDexFile(tmp_dex_dir, dex_path):
+  """Move the temp dex file out of |tmp_dex_dir|.
+
+  Args:
+    tmp_dex_dir: Path to temporary directory created with tempfile.mkdtemp().
+      The directory should have just a single file.
+    dex_path: Target path to move dex file to.
+
+  Raises:
+    Exception if there are multiple files in |tmp_dex_dir|.
+  """
+  tempfiles = os.listdir(tmp_dex_dir)
+  if len(tempfiles) > 1:
+    raise Exception('%d files created, expected 1' % len(tempfiles))
+
+  tmp_dex_path = os.path.join(tmp_dex_dir, tempfiles[0])
+  shutil.move(tmp_dex_path, dex_path)
+
+
+def _CreateR8Command(options, map_output_path, output_dir):
   # TODO: R8 needs -applymapping equivalent.
   cmd = [
     'java', '-jar', options.r8_path,
     '--no-desugaring',
-    '--classfile',
-    '--output', options.output_path,
+    '--no-data-resources',
+    '--output', output_dir,
     '--pg-map-output', map_output_path,
   ]
 
@@ -101,6 +132,10 @@ def _CreateR8Command(options, map_output_path):
   for config_file in options.proguard_configs:
     cmd += ['--pg-conf', config_file]
 
+  if options.main_dex_rules_path:
+    for main_dex_rule in options.main_dex_rules_path:
+      cmd += ['--main-dex-rules', main_dex_rule]
+
   cmd += options.input_paths
   return cmd
 
@@ -114,6 +149,7 @@ def main(args):
   proguard.configs(options.proguard_configs)
   proguard.config_exclusions(options.proguard_config_exclusions)
   proguard.outjar(options.output_path)
+  proguard.mapping_output(options.mapping_output)
 
   # If a jar is part of input no need to include it as library jar.
   classpath = [
@@ -127,12 +163,17 @@ def main(args):
   # TODO(agrieve): Remove proguard usages.
   if options.r8_path:
     with tempfile.NamedTemporaryFile() as mapping_temp:
-      # R8 will output mapping file to this temp file
-      cmd = _CreateR8Command(options, mapping_temp.name)
+      if options.output_path.endswith('.dex'):
+        with build_utils.TempDir() as tmp_dex_dir:
+          cmd = _CreateR8Command(options, mapping_temp.name, tmp_dex_dir)
+          build_utils.CheckOutput(cmd)
+          _MoveTempDexFile(tmp_dex_dir, options.output_path)
+      else:
+        cmd = _CreateR8Command(options, mapping_temp.name, options.output_path)
         build_utils.CheckOutput(cmd)
 
       # Copy the mapping file back to where it should be.
-      map_path = options.output_path + ".mapping"
+      map_path = options.mapping_output
       with build_utils.AtomicOutput(map_path) as mapping:
         # Mapping files generated by R8 include comments that may break
         # some of our tooling so remove those.

build/android/gyp/util/proguard_util.py

@@ -51,6 +51,7 @@ class ProguardCmdBuilder(object):
     self._configs = None
     self._config_exclusions = None
     self._outjar = None
+    self._mapping_output = None
     self._verbose = False
     self._disabled_optimizations = []
 
@@ -58,6 +59,10 @@ class ProguardCmdBuilder(object):
     assert self._outjar is None
     self._outjar = path
 
+  def mapping_output(self, path):
+    assert self._mapping_output is None
+    self._mapping_output = path
+
   def mapping(self, path):
     assert self._mapping is None
     assert os.path.exists(path), path
@@ -124,7 +129,7 @@ class ProguardCmdBuilder(object):
       '-outjars', self._outjar,
       '-printseeds', self._outjar + '.seeds',
       '-printusage', self._outjar + '.usage',
-      '-printmapping', self._outjar + '.mapping',
+      '-printmapping', self._mapping_output,
     ]
 
     if self._verbose:
@@ -156,7 +161,7 @@ class ProguardCmdBuilder(object):
     return [
         self._outjar,
         self._outjar + '.flags',
-        self._outjar + '.mapping',
+        self._mapping_output,
         self._outjar + '.seeds',
         self._outjar + '.usage',
     ]

build/android/gyp/write_build_config.py

@@ -860,7 +860,7 @@ def main(argv):
       help='Whether proguard is enabled for this apk or bundle module.')
   parser.add_option('--proguard-configs',
       help='GN-list of proguard flag files to use in final apk.')
-  parser.add_option('--proguard-output-jar-path',
+  parser.add_option('--proguard-mapping-path',
       help='Path to jar created by ProGuard step')
   parser.add_option('--fail',
       help='GN-list of error message lines to fail with.')
@@ -1257,8 +1257,8 @@ def main(argv):
                                                deps_proguard_disabled))
     else:
       deps_info['proguard_enabled'] = bool(options.proguard_enabled)
-      if options.proguard_output_jar_path:
-        deps_info['proguard_output_jar_path'] = options.proguard_output_jar_path
+      if options.proguard_mapping_path:
+        deps_info['proguard_mapping_path'] = options.proguard_mapping_path
 
   # The java code for an instrumentation test apk is assembled differently for
   # ProGuard vs. non-ProGuard.
@@ -1287,7 +1287,7 @@ def main(argv):
                         if p not in extra_jars)
       tested_apk_config = GetDepConfig(options.tested_apk_config)
       deps_info['proguard_under_test_mapping'] = (
-          tested_apk_config['proguard_output_jar_path'] + '.mapping')
+          tested_apk_config['proguard_mapping_path'])
     elif options.proguard_enabled:
       # Not sure why you'd want to proguard the test apk when the under-test apk
       # is not proguarded, but it's easy enough to support.

build/config/android/internal_rules.gni

@@ -419,11 +419,10 @@ template("write_build_config") {
     if (defined(invoker.proguard_enabled) && invoker.proguard_enabled) {
       args += [ "--proguard-enabled" ]
     }
-    if (defined(invoker.proguard_output_jar_path)) {
-      _rebased_proguard_output_jar_path =
-          rebase_path(invoker.proguard_output_jar_path, root_build_dir)
-      args +=
-          [ "--proguard-output-jar-path=$_rebased_proguard_output_jar_path" ]
+    if (defined(invoker.proguard_mapping_path)) {
+      _rebased_proguard_mapping_path =
+          rebase_path(invoker.proguard_mapping_path, root_build_dir)
+      args += [ "--proguard-mapping-path=$_rebased_proguard_mapping_path" ]
     }
     if (defined(invoker.proguard_configs)) {
       _rebased_proguard_configs =
@@ -944,7 +943,7 @@ if (enable_java_templates) {
       # http://crbug.com/725224. Fix for bots running out of memory.
       pool = "//build/toolchain:link_pool($default_toolchain)"
 
-      _output_jar_path = invoker.output_jar_path
+      _output_path = invoker.output_path
       _proguard_jar_path = _default_proguard_jar_path
       if (defined(invoker.proguard_jar_path)) {
         _proguard_jar_path = invoker.proguard_jar_path
@@ -957,10 +956,14 @@ if (enable_java_templates) {
       if (defined(invoker.inputs)) {
         inputs += invoker.inputs
       }
+      _mapping_path = "$_output_path.mapping"
+      if (defined(invoker.proguard_mapping_path)) {
+        _mapping_path = invoker.proguard_mapping_path
+      }
       depfile = "${target_gen_dir}/${target_name}.d"
       outputs = [
-        _output_jar_path,
-        "$_output_jar_path.mapping",
+        _output_path,
+        _mapping_path,
       ]
       _rebased_build_config = rebase_path(invoker.build_config, root_build_dir)
       args = [
@@ -969,7 +972,9 @@ if (enable_java_templates) {
         "--proguard-path",
         rebase_path(_proguard_jar_path, root_build_dir),
         "--output-path",
-        rebase_path(_output_jar_path, root_build_dir),
+        rebase_path(_output_path, root_build_dir),
+        "--mapping-output",
+        rebase_path(_mapping_path, root_build_dir),
         "--classpath",
         "@FileArg($_rebased_build_config:deps_info:proguard_classpath_jars)",
         "--classpath",
@@ -1058,9 +1063,81 @@ if (enable_java_templates) {
   }
 
   template("dex") {
+    assert(defined(invoker.output))
+
+    _proguard_enabled =
+        defined(invoker.proguard_enabled) && invoker.proguard_enabled
+    _proguarding_with_r8 = _proguard_enabled && experimental_r8_path != ""
+
+    assert(!(defined(invoker.input_jars) && _proguard_enabled),
+           "input_jars can't be specified when proguarding a dex.")
+
+    if (!_proguarding_with_r8) {
+      _dexing_jars = []
+      if (defined(invoker.input_jars)) {
+        _dexing_jars += invoker.input_jars
+      }
+    }
+
+    if (_proguard_enabled) {
+      if (defined(invoker.enable_multidex)) {
+        assert(invoker.enable_multidex || !invoker.enable_multidex)
+        if (defined(invoker.extra_main_dex_proguard_config)) {
+          assert(invoker.extra_main_dex_proguard_config != [])
+        }
+        if (defined(invoker.negative_main_dex_globs)) {
+          assert(invoker.negative_main_dex_globs != [])
+        }
+      }
+      if (_proguarding_with_r8) {
+        _proguard_output_path = invoker.output
+        _proguard_target_name = target_name
+      } else {
+        _proguard_output_path = invoker.output + ".proguard.jar"
+        _proguard_target_name = "${target_name}__proguard"
+        _dexing_jars += [ _proguard_output_path ]
+      }
+
+      proguard(_proguard_target_name) {
+        forward_variables_from(invoker,
+                               [
+                                 "proguard_jar_path",
+                                 "deps",
+                                 "build_config",
+                                 "proguard_mapping_path",
+                                 "testonly",
+                               ])
+        inputs = []
+        if (defined(invoker.inputs)) {
+          inputs += invoker.inputs
+        }
+        if (defined(invoker.proguard_configs)) {
+          inputs += invoker.proguard_configs
+        }
+
+        _rebased_build_config = rebase_path(build_config, root_build_dir)
+        args = [
+          "--proguard-configs=@FileArg($_rebased_build_config:deps_info:proguard_all_configs)",
+          "--input-paths=@FileArg($_rebased_build_config:deps_info:java_runtime_classpath)",
+        ]
+        if (defined(invoker.proguard_config_exclusions)) {
+          _rebased_proguard_config_exclusions =
+              rebase_path(invoker.proguard_config_exclusions, root_build_dir)
+          args += [
+            "--proguard-config-exclusions=$_rebased_proguard_config_exclusions",
+          ]
+        }
+        if (defined(invoker.proguard_args)) {
+          args += invoker.proguard_args
+        }
+
+        output_path = _proguard_output_path
+      }
+    }
+
+    if (!_proguarding_with_r8) {
       _enable_multidex =
           defined(invoker.enable_multidex) && invoker.enable_multidex
-
       if (_enable_multidex) {
         _main_dex_list_path = invoker.output + ".main_dex_list"
         _main_dex_list_target_name = "${target_name}__main_dex_list"
@@ -1117,13 +1194,19 @@ if (enable_java_templates) {
             inputs += [ invoker.extra_main_dex_proguard_config ]
             args += [
               "--main-dex-rules-path",
-            rebase_path(invoker.extra_main_dex_proguard_config, root_build_dir),
+              rebase_path(invoker.extra_main_dex_proguard_config,
+                          root_build_dir),
             ]
           }
 
+          if (_proguard_enabled) {
+            deps += [ ":${_proguard_target_name}" ]
+          }
+
           if (defined(invoker.negative_main_dex_globs)) {
-          args +=
-              [ "--negative-main-dex-globs=${invoker.negative_main_dex_globs}" ]
+            args += [
+              "--negative-main-dex-globs=${invoker.negative_main_dex_globs}",
+            ]
           }
 
           if (defined(invoker.input_jars_file_arg)) {
@@ -1131,14 +1214,13 @@ if (enable_java_templates) {
             args += [ "--inputs=${invoker.input_jars_file_arg}" ]
           }
 
-        if (defined(invoker.input_jars)) {
-          inputs += invoker.input_jars
-          args += rebase_path(invoker.input_jars, root_build_dir)
+          inputs += _dexing_jars
+          if (_dexing_jars != []) {
+            args += rebase_path(_dexing_jars, root_build_dir)
           }
         }
       }
 
-    assert(defined(invoker.output))
       action_with_pydeps(target_name) {
         forward_variables_from(invoker,
                                [
@@ -1152,10 +1234,6 @@ if (enable_java_templates) {
           invoker.output,
         ]
 
-      if (defined(invoker.use_pool) && invoker.use_pool) {
-        pool = "//build/toolchain:link_pool($default_toolchain)"
-      }
-
         _rebased_output = rebase_path(invoker.output, root_build_dir)
 
         args = [
@@ -1165,21 +1243,8 @@ if (enable_java_templates) {
           _rebased_output,
         ]
 
-      if (defined(invoker.dexlayout_profile)) {
-        args += [
-          "--dexlayout-profile",
-          rebase_path(invoker.dexlayout_profile, root_build_dir),
-          "--dexlayout-path",
-          rebase_path(_dexlayout_path, root_build_dir),
-          "--profman-path",
-          rebase_path(_profman_path, root_build_dir),
-        ]
-        inputs += [
-          _dexlayout_path,
-          _profman_path,
-          invoker.dexlayout_profile,
-        ]
-        inputs += _default_art_libs
+        if (_proguard_enabled) {
+          deps += [ ":${_proguard_target_name}" ]
         }
 
         if (_enable_multidex) {
@@ -1197,9 +1262,26 @@ if (enable_java_templates) {
           args += [ "--inputs=${invoker.input_jars_file_arg}" ]
         }
 
-      if (defined(invoker.input_jars)) {
-        inputs += invoker.input_jars
-        args += rebase_path(invoker.input_jars, root_build_dir)
+        inputs += _dexing_jars
+        if (_dexing_jars != []) {
+          args += rebase_path(_dexing_jars, root_build_dir)
+        }
+
+        if (defined(invoker.dexlayout_profile)) {
+          args += [
+            "--dexlayout-profile",
+            rebase_path(invoker.dexlayout_profile, root_build_dir),
+            "--dexlayout-path",
+            rebase_path(_dexlayout_path, root_build_dir),
+            "--profman-path",
+            rebase_path(_profman_path, root_build_dir),
+          ]
+          inputs += [
+            _dexlayout_path,
+            _profman_path,
+            invoker.dexlayout_profile,
+          ]
+          inputs += _default_art_libs
         }
 
         if (!is_java_debug) {
@@ -1221,6 +1303,7 @@ if (enable_java_templates) {
         ]
       }
     }
+  }
 
   template("emma_instr") {
     action_with_pydeps(target_name) {
@@ -2786,7 +2869,7 @@ if (enable_java_templates) {
   #    output APK.
   #  incremental_install_json_path: If incremental_allowed, path to the output
   #    incremental install json configuration file.
-  #  proguard_output_jar_path: Path to .jar file produced from ProGuard step.
+  #  proguard_mapping_path: Path to .mapping file produced from ProGuard step.
   #  shared_libraries_runtime_deps_file: Optional. Path to a file listing the
   #    native shared libraries required at runtime by the APK.
   #  secondary_abi_shared_libraries_runtime_deps_file:
@@ -2982,7 +3065,7 @@ if (enable_java_templates) {
                                "main_class",
                                "proguard_configs",
                                "proguard_enabled",
-                               "proguard_output_jar_path",
+                               "proguard_mapping_path",
                                "secondary_abi_loadable_modules",
                                "type",
                              ])

chrome/android/BUILD.gn

@@ -1501,8 +1501,10 @@ template("monochrome_public_apk_or_module_tmpl") {
     }
 
     version_name = chrome_version_name
+    if (experimental_r8_path == "") {
       enable_multidex = true
     }
+  }
 }
 
 monochrome_public_apk_or_module_tmpl("monochrome_public_apk") {
@@ -1849,7 +1851,9 @@ android_app_bundle("monochrome_public_bundle") {
   base_module_target = ":monochrome_public_base_module"
   if (!is_java_debug) {
     proguard_enabled = true
+    if (experimental_r8_path == "") {
       enable_multidex = true
+    }
     proguard_android_sdk_dep = webview_framework_dep
   }
   if (modularize_ar) {

chrome/android/webapk/libs/runtime_library/BUILD.gn

@@ -49,19 +49,16 @@ android_library("runtime_library_for_tests_java") {
   ]
 }
 
-dist_jar("webapk_runtime_library") {
-  requires_android = true
+proguarded_dist_dex("webapk_runtime_library") {
   deps = [
     ":runtime_library_for_assets_java",
   ]
-  proguard_enabled = true
   proguard_configs = [
     "runtime_library.proguard.flags",
     "//base/android/proguard/chromium_code.flags",
     "//base/android/proguard/chromium_apk.flags",
   ]
-  output = "$target_gen_dir/$target_name.jar"
-  dex_path = "$target_gen_dir/$runtime_library_dex_asset_name"
+  output = "$target_gen_dir/$runtime_library_dex_asset_name"
 }
 
 android_assets("runtime_library_assets") {