Reland "Android: Turning on R8 instead of Proguard for public targets"

This is a reland of 09f15bd Original change's description: > Android: Turning on R8 instead of Proguard for public targets > > We are in the process of migrating to R8 to replace Proguard. This is > the first step - move all usages of Proguard in public targets to R8. > > Some refactorings were natural with the few forced changes this caused. > > TBR=smaier > > Bug: 908988, 913554 > Change-Id: I2139919598fba1643d7560dc5557d5efb9a5887c > Reviewed-on: https://chromium-review.googlesource.com/c/1357306 > Reviewed-by: Sam Maier <smaier@chromium.org> > Reviewed-by: agrieve <agrieve@chromium.org> > Commit-Queue: Sam Maier <smaier@chromium.org> > Cr-Commit-Position: refs/heads/master@{#619471} TBR=proguard.flags(smaier) Bug: 908988, 913554 Change-Id: I93690ad2a6838025221b7fab4b0bee1b06c920c9 Reviewed-on: https://chromium-review.googlesource.com/c/1394387 Commit-Queue: Sam Maier <smaier@chromium.org> Reviewed-by: Peter Wen <wnwen@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Cr-Commit-Position: refs/heads/master@{#619702}
Denger-Network · Jan 3, 2019 · 36ab751 · 36ab751
1 parent 5040590
commit 36ab751
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 100 deletions.
diff --git a/build/android/gyp/proguard.py b/build/android/gyp/proguard.py
@@ -14,19 +14,6 @@
 from util import proguard_util
 
 
-_DANGEROUS_OPTIMIZATIONS = [
-    # See crbug.com/825995 (can cause VerifyErrors)
-    "class/merging/vertical",
-    "class/unboxing/enum",
-    # See crbug.com/625992
-    "code/allocation/variable",
-    # See crbug.com/625994
-    "field/propagation/value",
-    "method/propagation/parameter",
-    "method/propagation/returnvalue",
-]
-
-
 # Example:
 # android.arch.core.internal.SafeIterableMap$Entry -> b:
 #     1:1:java.lang.Object getKey():353:353 -> getKey
@@ -56,13 +43,12 @@ def _ParseOptions(args):
                     help='GN list of paths to proguard configuration files '
                          '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(
+      '--apply-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.')
@@ -95,6 +81,10 @@ def _ParseOptions(args):
   if not options.mapping_output:
     options.mapping_output = options.output_path + ".mapping"
 
+  if options.apply_mapping:
+    options.apply_mapping = os.path.abspath(options.apply_mapping)
+
+
   return options
 
 
@@ -117,25 +107,26 @@ def _MoveTempDexFile(tmp_dex_dir, dex_path):
   shutil.move(tmp_dex_path, dex_path)
 
 
-def _CreateR8Command(options, map_output_path, output_dir):
-  # TODO: R8 needs -applymapping equivalent.
+def _CreateR8Command(options, map_output_path, output_dir, tmp_proguard_config,
+                     libraries):
   cmd = [
     'java', '-jar', options.r8_path,
     '--no-data-resources',
     '--output', output_dir,
     '--pg-map-output', map_output_path,
   ]
 
-  classpath = [
-      p for p in set(options.classpath) if p not in options.input_paths
-  ]
-
-  for lib in classpath:
+  for lib in libraries:
     cmd += ['--lib', lib]
 
   for config_file in options.proguard_configs:
     cmd += ['--pg-conf', config_file]
 
+  if options.apply_mapping:
+    tmp_proguard_config.write('-applymapping ' + options.apply_mapping)
+    tmp_proguard_config.flush()
+    cmd += ['--pg-conf', tmp_proguard_config.name]
+
   if options.min_api:
     cmd += ['--min-api', options.min_api]
 
@@ -151,33 +142,27 @@ def main(args):
   args = build_utils.ExpandFileArgs(args)
   options = _ParseOptions(args)
 
-  proguard = proguard_util.ProguardCmdBuilder(options.proguard_path)
-  proguard.injars(options.input_paths)
-  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 = [
-      p for p in set(options.classpath) if p not in options.input_paths
-  ]
-  proguard.libraryjars(classpath)
-  proguard.verbose(options.verbose)
-  if not options.enable_dangerous_optimizations:
-    proguard.disable_optimizations(_DANGEROUS_OPTIMIZATIONS)
+  libraries = []
+  for p in options.classpath:
+    # If a jar is part of input no need to include it as library jar.
+    if p not in libraries and p not in options.input_paths:
+      libraries.append(p)
 
   # TODO(agrieve): Remove proguard usages.
   if options.r8_path:
     with tempfile.NamedTemporaryFile() as mapping_temp:
-      if options.output_path.endswith('.dex'):
-        with build_utils.TempDir() as tmp_dex_dir:
-          cmd = _CreateR8Command(options, mapping_temp.name, tmp_dex_dir)
+      with tempfile.NamedTemporaryFile() as tmp_proguard_config:
+        if options.output_path.endswith('.dex'):
+          with build_utils.TempDir() as tmp_dex_dir:
+            cmd = _CreateR8Command(options, mapping_temp.name, tmp_dex_dir,
+                                   tmp_proguard_config, libraries)
+            build_utils.CheckOutput(cmd)
+            _MoveTempDexFile(tmp_dex_dir, options.output_path)
+        else:
+          cmd = _CreateR8Command(options, mapping_temp.name,
+                                 options.output_path, tmp_proguard_config,
+                                 libraries)
           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.mapping_output
@@ -187,20 +172,35 @@ def main(args):
         mapping_temp.seek(0)
         mapping.writelines(l for l in mapping_temp if not l.startswith("#"))
 
-    build_utils.WriteDepfile(options.depfile, options.output_path,
-                             inputs=proguard.GetDepfileDeps(),
-                             add_pydeps=False)
+    other_inputs = []
+    if options.apply_mapping:
+      other_inputs += options.apply_mapping
+
+    build_utils.WriteDepfile(
+        options.depfile,
+        options.output_path,
+        inputs=options.proguard_configs + options.input_paths + libraries +
+        other_inputs,
+        add_pydeps=False)
   else:
+    proguard = proguard_util.ProguardCmdBuilder(options.proguard_path)
+    proguard.injars(options.input_paths)
+    proguard.configs(options.proguard_configs)
+    proguard.config_exclusions(options.proguard_config_exclusions)
+    proguard.outjar(options.output_path)
+    proguard.mapping_output(options.mapping_output)
+    proguard.libraryjars(libraries)
+    proguard.verbose(options.verbose)
     # Do not consider the temp file as an input since its name is random.
     input_paths = proguard.GetInputs()
 
     with tempfile.NamedTemporaryFile() as f:
-      if options.mapping:
-        input_paths.append(options.mapping)
+      if options.apply_mapping:
+        input_paths.append(options.apply_mapping)
         # Maintain only class name mappings in the .mapping file in order to
         # work around what appears to be a ProGuard bug in -applymapping:
         #     method 'int close()' is not being kept as 'a', but remapped to 'c'
-        _RemoveMethodMappings(options.mapping, f)
+        _RemoveMethodMappings(options.apply_mapping, f)
         proguard.mapping(f.name)
 
       input_strings = proguard.build()

diff --git a/build/config/android/config.gni b/build/config/android/config.gni
@@ -221,9 +221,10 @@ if (is_android || is_chromeos) {
     android_sdk_tools_bundle_aapt2 =
         "//third_party/android_build_tools/aapt2/aapt2"
 
-    # Use r8 for Java optimization rather than ProGuard.
-    # This will evenutally be the default. https://crbug.com/872904
-    experimental_use_r8 = false
+    # Use R8 for Java optimization rather than ProGuard for all targets. R8 is
+    # already used as the default for public targets. This will evenutally be
+    # the default. https://crbug.com/908988
+    use_r8 = false
   }
 
   # We need a second declare_args block to make sure we are using the overridden

diff --git a/build/config/android/internal_rules.gni b/build/config/android/internal_rules.gni
@@ -951,13 +951,7 @@ if (enable_java_templates) {
       pool = "//build/toolchain:link_pool($default_toolchain)"
 
       _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
-      }
-
       inputs = [
-        _proguard_jar_path,
         invoker.build_config,
       ]
       if (defined(invoker.inputs)) {
@@ -976,8 +970,6 @@ if (enable_java_templates) {
       args = [
         "--depfile",
         rebase_path(depfile, root_build_dir),
-        "--proguard-path",
-        rebase_path(_proguard_jar_path, root_build_dir),
         "--output-path",
         rebase_path(_output_path, root_build_dir),
         "--mapping-output",
@@ -987,20 +979,25 @@ if (enable_java_templates) {
         "--classpath",
         "@FileArg($_rebased_build_config:android:sdk_jars)",
       ]
-      if (experimental_use_r8) {
+
+      if (!defined(invoker.proguard_jar_path) || use_r8) {
         args += [
           "--r8-path",
           rebase_path(_r8_path, root_build_dir),
         ]
+        inputs += [ _r8_path ]
+      } else {
+        _proguard_jar_path = invoker.proguard_jar_path
+        args += [
+          "--proguard-path",
+          rebase_path(_proguard_jar_path, root_build_dir),
+        ]
+        inputs += [ _proguard_jar_path ]
       }
+
       if (defined(invoker.args)) {
         args += invoker.args
       }
-      if (defined(invoker.proguard_jar_path)) {
-        # We assume that if we are using a different ProGuard, this new version
-        # can handle the 'dangerous' optimizaions.
-        args += [ "--enable-dangerous-optimizations" ]
-      }
     }
   }
 
@@ -1074,11 +1071,18 @@ if (enable_java_templates) {
 
     _proguard_enabled =
         defined(invoker.proguard_enabled) && invoker.proguard_enabled
-    _proguarding_with_r8 = _proguard_enabled && experimental_use_r8
+    _proguarding_with_r8 =
+        _proguard_enabled && !defined(invoker.proguard_jar_path)
+    _enable_multidex =
+        defined(invoker.enable_multidex) && invoker.enable_multidex
 
     assert(!(defined(invoker.input_jars) && _proguard_enabled),
            "input_jars can't be specified when proguarding a dex.")
 
+    if (_enable_multidex) {
+      _main_dex_rules = "//build/android/main_dex_classes.flags"
+    }
+
     if (!_proguarding_with_r8) {
       _dexing_jars = []
       if (defined(invoker.input_jars)) {
@@ -1088,10 +1092,6 @@ if (enable_java_templates) {
 
     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 != [])
         }
@@ -1145,13 +1145,27 @@ if (enable_java_templates) {
           ]
         }
 
+        if (_enable_multidex && _proguarding_with_r8) {
+          if (defined(invoker.extra_main_dex_proguard_config)) {
+            args += [
+              "--main-dex-rules-path",
+              rebase_path(invoker.extra_main_dex_proguard_config,
+                          root_build_dir),
+            ]
+            inputs += [ invoker.extra_main_dex_proguard_config ]
+          }
+          args += [
+            "--main-dex-rules-path",
+            rebase_path(_main_dex_rules, root_build_dir),
+          ]
+          inputs += [ _main_dex_rules ]
+        }
+
         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"
@@ -1168,8 +1182,6 @@ if (enable_java_templates) {
           # http://crbug.com/725224. Fix for bots running out of memory.
           pool = "//build/toolchain:link_pool($default_toolchain)"
 
-          main_dex_rules = "//build/android/main_dex_classes.flags"
-
           if (defined(invoker.proguard_jar_path)) {
             _proguard_jar_path = invoker.proguard_jar_path
           } else {
@@ -1179,7 +1191,7 @@ if (enable_java_templates) {
           _shrinked_android = "$android_sdk_build_tools/lib/shrinkedAndroid.jar"
           _dx = "$android_sdk_build_tools/lib/dx.jar"
           inputs = [
-            main_dex_rules,
+            _main_dex_rules,
             _dx,
             _proguard_jar_path,
             _shrinked_android,
@@ -1199,7 +1211,7 @@ if (enable_java_templates) {
             "--main-dex-list-path",
             rebase_path(_main_dex_list_path, root_build_dir),
             "--main-dex-rules-path",
-            rebase_path(main_dex_rules, root_build_dir),
+            rebase_path(_main_dex_rules, root_build_dir),
             "--proguard-path",
             rebase_path(_proguard_jar_path, root_build_dir),
           ]
@@ -3369,7 +3381,7 @@ if (enable_java_templates) {
 
         # Skip desugaring for debug builds using r8 to increase incremental
         # build speed. Release builds still need desugar.jar for binary size.
-        _using_r8 = !defined(invoker.proguard_jar_path) || experimental_use_r8
+        _using_r8 = !defined(invoker.proguard_jar_path) || use_r8
         _desugar = !(_using_r8 && is_java_debug) && _supports_android
         process_java_prebuilt(_process_prebuilt_target_name) {
           forward_variables_from(invoker,

diff --git a/build/config/android/rules.gni b/build/config/android/rules.gni
@@ -2578,10 +2578,6 @@ if (enable_java_templates) {
           forward_variables_from(invoker, [ "proguard_jar_path" ])
           deps += _deps + [ ":$_compile_resources_target" ]
           proguard_configs = [ _jar_path ]
-          if (defined(invoker.apk_under_test)) {
-            proguard_args = [ "--mapping=@FileArg($_rebased_build_config:deps_info:proguard_under_test_mapping)" ]
-            deps += [ "${invoker.apk_under_test}__final_dex" ]
-          }
           proguard_mapping_path = _proguard_mapping_path
         } else {
           if (_enable_multidex) {

diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
@@ -1457,9 +1457,6 @@ template("chrome_public_apk_or_module_tmpl") {
     load_library_from_apk = _is_modern && chromium_linker_supported
 
     version_name = chrome_version_name
-    if (!experimental_use_r8) {
-      enable_multidex = true
-    }
   }
 }
 
@@ -1558,9 +1555,6 @@ template("monochrome_public_apk_or_module_tmpl") {
         (!defined(use_trichrome_library) || !use_trichrome_library)
 
     version_name = chrome_version_name
-    if (!experimental_use_r8) {
-      enable_multidex = true
-    }
   }
 }
 
@@ -1823,9 +1817,6 @@ android_app_bundle("chrome_public_bundle") {
   base_module_target = ":chrome_public_base_module"
   if (!is_java_debug) {
     proguard_enabled = true
-    if (!experimental_use_r8) {
-      enable_multidex = true
-    }
   }
 
   # Signing is very slow, only do that for official builds.
@@ -1839,9 +1830,6 @@ android_app_bundle("chrome_modern_public_bundle") {
   base_module_target = ":chrome_modern_public_base_module"
   if (!is_java_debug) {
     proguard_enabled = true
-    if (!experimental_use_r8) {
-      enable_multidex = true
-    }
   }
   enable_language_splits = enable_chrome_language_splits
   if (modularize_vr) {
@@ -1859,9 +1847,6 @@ android_app_bundle("monochrome_public_bundle") {
   base_module_target = ":monochrome_public_base_module"
   if (!is_java_debug) {
     proguard_enabled = true
-    if (!experimental_use_r8) {
-      enable_multidex = true
-    }
     proguard_android_sdk_dep = webview_framework_dep
   }
   enable_language_splits = enable_chrome_language_splits