Revert "Android: Updates to warnings-as-errors when building"

This reverts commit 339e6581.

Reason for revert: *Probably* broke Android Cast builders.

Original change's description:
> Android: Updates to warnings-as-errors when building
> 
> * Delete java_warnings_as_errors
> * Use treat_warnings_as_errors for android rules
>   * warnings-as-errors is now default true for debug builds
> * Add --warnings-as-errors to proguard.py, dex.py, bytecode_processor.py
>   so that these warnings can all be turned off locally.
> * Made build_utils.CheckOutput fail by default when stderr or stdout is
>   printed. This should prevent build logs from creaping in.
> * Added "you should use treat_warnings_as_errors=false" into error
>   message when builds fail due to warnings.
> 
> Bug: 1029357
> Change-Id: Ia69e545a110554d76e190cb2da8b20fe10d4d6e8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2232809
> Reviewed-by: Dirk Pranke <dpranke@google.com>
> Reviewed-by: Peter Wen <wnwen@chromium.org>
> Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#790225}

TBR=wnwen@chromium.org,dpranke@google.com,agrieve@chromium.org

Change-Id: Id123fab1cdcbbd99c137ea5750719fe68b05440c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1029357
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308411Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790287}

build/android/gyp/bytecode_processor.py

@@ -32,9 +32,6 @@ def main(argv):
   parser.add_argument('--stamp')
   parser.add_argument('-v', '--verbose', action='store_true')
   parser.add_argument('--missing-classes-allowlist')
-  parser.add_argument('--warnings-as-errors',
-                      action='store_true',
-                      help='Treat all warnings as errors.')
   _AddSwitch(parser, '--is-prebuilt')
   args = parser.parse_args(argv)
 
@@ -60,10 +57,7 @@ def main(argv):
   cmd += args.full_classpath_jars
   cmd += [str(len(args.full_classpath_gn_targets))]
   cmd += args.full_classpath_gn_targets
-  build_utils.CheckOutput(cmd,
-                          print_stdout=True,
-                          fail_func=None,
-                          fail_on_output=args.warnings_as_errors)
+  subprocess.check_call(cmd)
 
   if args.stamp:
     build_utils.Touch(args.stamp)

build/android/gyp/compile_java.py

@@ -30,7 +30,7 @@ _JAVAC_EXTRACTOR = os.path.join(build_utils.DIR_SOURCE_ROOT, 'third_party',
                                 'framework', 'javac_extractor.jar')
 
 # Full list of checks: https://errorprone.info/bugpatterns
-ERRORPRONE_WARNINGS_TO_DISABLE = [
+ERRORPRONE_WARNINGS_TO_TURN_OFF = [
     # These should really be turned on.
     'ParameterNotNullable',
     'CollectionUndefinedEquality',
@@ -175,8 +175,8 @@ ERRORPRONE_WARNINGS_TO_DISABLE = [
 
 # Full list of checks: https://errorprone.info/bugpatterns
 # Only those marked as "experimental" need to be listed here in order to be
-# enabled.
-ERRORPRONE_WARNINGS_TO_ENABLE = [
+# enabled. We build with -Werror, so all default checks cause builds to fail.
+ERRORPRONE_WARNINGS_TO_ERROR = [
     'BinderIdentityRestoredDangerously',
     'EmptyIf',
     'EqualsBrokenForNull',
@@ -509,8 +509,7 @@ def _RunCompiler(options, javac_cmd, java_files, classpath, jar_path,
       build_utils.CheckOutput(cmd,
                               print_stdout=options.chromium_code,
                               stdout_filter=ProcessJavacOutput,
-                              stderr_filter=stderr_filter,
-                              fail_on_output=options.warnings_as_errors)
+                              stderr_filter=stderr_filter)
       end = time.time() - start
       logging.info('Java compilation took %ss', end)
 
@@ -672,13 +671,12 @@ def main(argv):
   if options.enable_errorprone:
     # All errorprone args are passed space-separated in a single arg.
     errorprone_flags = ['-Xplugin:ErrorProne']
-    # Make everything a warning so that when treat_warnings_as_errors is false,
-    # they do not fail the build.
-    errorprone_flags += ['-XepAllErrorsAsWarnings']
-    for warning in ERRORPRONE_WARNINGS_TO_DISABLE:
+    for warning in ERRORPRONE_WARNINGS_TO_TURN_OFF:
       errorprone_flags.append('-Xep:{}:OFF'.format(warning))
-    for warning in ERRORPRONE_WARNINGS_TO_ENABLE:
-      errorprone_flags.append('-Xep:{}:WARN'.format(warning))
+    for warning in ERRORPRONE_WARNINGS_TO_ERROR:
+      errorprone_flags.append('-Xep:{}:ERROR'.format(warning))
+    if not options.warnings_as_errors:
+      errorprone_flags.append('-XepAllErrorsAsWarnings')
     javac_args += ['-XDcompilePolicy=simple', ' '.join(errorprone_flags)]
     # This flag quits errorprone after checks and before code generation, since
     # we do not need errorprone outputs, this speeds up errorprone by 4 seconds
@@ -696,6 +694,14 @@ def main(argv):
     # Android's boot jar doesn't contain all java 8 classes.
     options.bootclasspath.append(build_utils.RT_JAR_PATH)
 
+  if options.warnings_as_errors:
+    javac_args.extend(['-Werror'])
+  else:
+    # XDignore.symbol.file makes javac compile against rt.jar instead of
+    # ct.sym. This means that using a java internal package/class will not
+    # trigger a compile warning or error.
+    javac_args.extend(['-XDignore.symbol.file'])
+
   if options.processors:
     javac_args.extend(['-processor', ','.join(options.processors)])
   else:
@@ -731,9 +737,9 @@ def main(argv):
       options.jar_path + '.info',
   ]
 
-  input_strings = javac_cmd + javac_args + options.classpath + java_files + [
-      options.warnings_as_errors, options.jar_info_exclude_globs
-  ]
+  input_strings = javac_cmd + javac_args + options.classpath + java_files
+  if options.jar_info_exclude_globs:
+    input_strings.append(options.jar_info_exclude_globs)
 
   md5_check.CallAndWriteDepfileIfStale(
       lambda: _OnStaleMd5(options, javac_cmd, javac_args, java_files),

build/android/gyp/dex.py

@@ -88,12 +88,6 @@ def _ParseArgs(args):
       'main dex and keeps all line number information, and then some.')
   parser.add_argument(
       '--min-api', help='Minimum Android API level compatibility.')
-  parser.add_argument('--force-enable-assertions',
-                      action='store_true',
-                      help='Forcefully enable javac generated assertion code.')
-  parser.add_argument('--warnings-as-errors',
-                      action='store_true',
-                      help='Treat all warnings as errors.')
 
   group = parser.add_argument_group('Dexlayout')
   group.add_argument(
@@ -115,6 +109,11 @@ def _ParseArgs(args):
             'unobfuscated symbols present in the code. If not present, the jar '
             'is assumed not to be obfuscated.'))
 
+  parser.add_argument(
+      '--force-enable-assertions',
+      action='store_true',
+      help='Forcefully enable javac generated assertion code.')
+
   options = parser.parse_args(args)
 
   if options.dexlayout_profile:
@@ -166,7 +165,7 @@ def CreateStderrFilter(show_desugar_default_interface_warnings):
   return filter_stderr
 
 
-def _RunD8(dex_cmd, input_paths, output_path, warnings_as_errors,
+def _RunD8(dex_cmd, input_paths, output_path,
            show_desugar_default_interface_warnings):
   dex_cmd = dex_cmd + ['--output', output_path] + input_paths
 
@@ -183,9 +182,7 @@ def _RunD8(dex_cmd, input_paths, output_path, warnings_as_errors,
 
     # stdout sometimes spams with things like:
     # Stripped invalid locals information from 1 method.
-    build_utils.CheckOutput(dex_cmd,
-                            stderr_filter=stderr_filter,
-                            fail_on_output=warnings_as_errors)
+    build_utils.CheckOutput(dex_cmd, stderr_filter=stderr_filter)
 
 
 def _EnvWithArtLibPath(binary_path):
@@ -366,7 +363,6 @@ def _CreateFinalDex(d8_inputs, output, tmp_dir, dex_cmd, options=None):
     tmp_dex_dir = os.path.join(tmp_dir, 'tmp_dex_dir')
     os.mkdir(tmp_dex_dir)
     _RunD8(dex_cmd, d8_inputs, tmp_dex_dir,
-           (not options or options.warnings_as_errors),
            (options and options.show_desugar_default_interface_warnings))
     logging.debug('Performed dex merging')
 
@@ -450,7 +446,6 @@ def _CreateIntermediateDexFiles(changes, options, tmp_dir, dex_cmd):
     # Dex necessary classes into intermediate dex files.
     dex_cmd = dex_cmd + ['--intermediate', '--file-per-class-file']
     _RunD8(dex_cmd, class_files, options.incremental_dir,
-           options.warnings_as_errors,
            options.show_desugar_default_interface_warnings)
     logging.debug('Dexed class files.')
 

build/android/gyp/dex_jdk_libs.py

@@ -51,7 +51,7 @@ def DexJdkLibJar(r8_path, min_api, desugar_jdk_libs_json, desugar_jdk_libs_jar,
 
     cmd += ['--output', tmp_dir, desugar_jdk_libs_jar]
 
-    build_utils.CheckOutput(cmd, print_stdout=True)
+    subprocess.check_output(cmd, stderr=subprocess.STDOUT)
     if os.path.exists(os.path.join(tmp_dir, 'classes2.dex')):
       raise Exception('Achievement unlocked: desugar_jdk_libs is multidex!')
 

build/android/gyp/lint.py

@@ -133,7 +133,7 @@ def _RunLint(lint_binary_path,
              lint_gen_dir,
              baseline,
              testonly_target=False,
-             warnings_as_errors=False,
+             can_fail_build=False,
              silent=False):
   logging.info('Lint starting')
 
@@ -234,10 +234,10 @@ def _RunLint(lint_binary_path,
             ' - For more information about lint and how to fix lint issues,'
             ' please refer to {}\n'.format(_SrcRelative(project_xml_path),
                                            _LINT_MD_URL))
-      if warnings_as_errors:
-        raise
-      else:
-        print(e)
+    if can_fail_build:
+      raise
+    else:
+      print(e)
   else:
     # Lint succeeded, no need to keep generated files for debugging purposes.
     shutil.rmtree(resource_root_dir, ignore_errors=True)
@@ -277,9 +277,10 @@ def _ParseArgs(argv):
                       'targets.')
   parser.add_argument('--manifest-package',
                       help='Package name of the AndroidManifest.xml.')
-  parser.add_argument('--warnings-as-errors',
+  parser.add_argument('--can-fail-build',
                       action='store_true',
-                      help='Treat all warnings as errors.')
+                      help='If set, script will exit with nonzero exit status'
+                      ' if lint errors are present')
   parser.add_argument('--silent',
                       action='store_true',
                       help='If set, script will not log anything.')
@@ -346,7 +347,7 @@ def main():
            args.lint_gen_dir,
            args.baseline,
            testonly_target=args.testonly,
-           warnings_as_errors=args.warnings_as_errors,
+           can_fail_build=args.can_fail_build,
            silent=args.silent)
   logging.info('Creating stamp file')
   build_utils.Touch(args.stamp)

build/android/gyp/proguard.py

@@ -155,9 +155,6 @@ def _ParseOptions():
       action='append',
       dest='feature_names',
       help='The name of the feature module.')
-  parser.add_argument('--warnings-as-errors',
-                      action='store_true',
-                      help='Treat all warnings as errors.')
   parser.add_argument('--show-desugar-default-interface-warnings',
                       action='store_true',
                       help='Enable desugaring warnings.')
@@ -260,6 +257,16 @@ class _DexPathContext(object):
     shutil.move(tmp_jar_output, self._final_output_path)
 
 
+def _CreateStderrFilter(show_desugar_default_interface_warnings):
+  d8_filter = dex.CreateStderrFilter(show_desugar_default_interface_warnings)
+
+  def filter_stderr(output):
+    output = re.sub(r'.*_JAVA_OPTIONS.*\n?', '', output)
+    return d8_filter(output)
+
+  return filter_stderr
+
+
 def _OptimizeWithR8(options,
                     config_paths,
                     libraries,
@@ -296,11 +303,6 @@ def _OptimizeWithR8(options,
 
     cmd = [
         build_utils.JAVA_PATH,
-        '-Dcom.android.tools.r8.allowTestProguardOptions=1',
-    ]
-    if options.disable_outlining:
-      cmd += [' -Dcom.android.tools.r8.disableOutlining=1']
-    cmd += [
         '-cp',
         options.r8_path,
         'com.android.tools.r8.R8',
@@ -349,13 +351,18 @@ def _OptimizeWithR8(options,
     extra_jars = set(options.input_paths) - module_input_jars
     cmd += sorted(extra_jars)
 
+    env = os.environ.copy()
+    env['_JAVA_OPTIONS'] = '-Dcom.android.tools.r8.allowTestProguardOptions=1'
+    if options.disable_outlining:
+      env['_JAVA_OPTIONS'] += ' -Dcom.android.tools.r8.disableOutlining=1'
+
     try:
-      stderr_filter = dex.CreateStderrFilter(
+      stderr_filter = _CreateStderrFilter(
           options.show_desugar_default_interface_warnings)
       build_utils.CheckOutput(cmd,
+                              env=env,
                               print_stdout=print_stdout,
-                              stderr_filter=stderr_filter,
-                              fail_on_output=options.warnings_as_errors)
+                              stderr_filter=stderr_filter)
     except build_utils.CalledProcessError as err:
       debugging_link = ('\n\nR8 failed. Please see {}.'.format(
           'https://chromium.googlesource.com/chromium/src/+/HEAD/build/'

build/android/gyp/util/build_utils.py

@@ -234,14 +234,10 @@ def FilterReflectiveAccessJavaWarnings(output):
 # This can be used in most cases like subprocess.check_output(). The output,
 # particularly when the command fails, better highlights the command's failure.
 # If the command fails, raises a build_utils.CalledProcessError.
-def CheckOutput(args,
-                cwd=None,
-                env=None,
-                print_stdout=False,
-                print_stderr=True,
+def CheckOutput(args, cwd=None, env=None,
+                print_stdout=False, print_stderr=True,
                 stdout_filter=None,
                 stderr_filter=None,
-                fail_on_output=True,
                 fail_func=lambda returncode, stderr: returncode != 0):
   if not cwd:
     cwd = os.getcwd()
@@ -256,7 +252,7 @@ def CheckOutput(args,
   if stderr_filter is not None:
     stderr = stderr_filter(stderr)
 
-  if fail_func and fail_func(child.returncode, stderr):
+  if fail_func(child.returncode, stderr):
     raise CalledProcessError(cwd, args, stdout + stderr)
 
   if print_stdout:
@@ -264,21 +260,6 @@ def CheckOutput(args,
   if print_stderr:
     sys.stderr.write(stderr)
 
-  has_stdout = print_stdout and stdout
-  has_stderr = print_stderr and stderr
-  if fail_on_output and (has_stdout or has_stderr):
-    MSG = """\
-Command failed because it wrote to {}.
-You can often set treat_warnings_as_errors=false to not treat output as \
-failure (useful when developing locally)."""
-    if has_stdout and has_stderr:
-      stream_string = 'stdout and stderr'
-    elif has_stdout:
-      stream_string = 'stdout'
-    else:
-      stream_string = 'stderr'
-    raise CalledProcessError(cwd, args, MSG.format(stream_string))
-
   return stdout
 
 

build/config/android/config.gni

@@ -239,6 +239,9 @@ if (is_android || is_chromeos) {
     # Use hashed symbol names to reduce JNI symbol overhead.
     use_hashed_jni_names = !is_java_debug
 
+    # For local development, it's nice to not fail builds on warnings.
+    java_warnings_as_errors = !is_java_debug
+
     # Desugar lambdas and interfaces methods using Desugar.jar rather than
     # D8/R8. D8/R8 will still be used for backported method desugaring.
     enable_bazel_desugar = true

build/config/android/internal_rules.gni

@@ -6,7 +6,6 @@
 # Some projects (e.g. V8) do not have non-build directories DEPS'ed in.
 import("//build/config/android/config.gni")
 import("//build/config/android/copy_ex.gni")
-import("//build/config/compiler/compiler.gni")
 import("//build/config/coverage/coverage.gni")
 import("//build/config/dcheck_always_on.gni")
 import("//build/config/python.gni")
@@ -1038,8 +1037,8 @@ if (enable_java_templates) {
         args += [ "--manifest-package=${invoker.manifest_package}" ]
       }
 
-      if (treat_warnings_as_errors) {
-        args += [ "--warnings-as-errors" ]
+      if (java_warnings_as_errors) {
+        args += [ "--can-fail-build" ]
       }
 
       _stamp_path = "$target_out_dir/$target_name/build.lint.stamp"
@@ -1134,9 +1133,6 @@ if (enable_java_templates) {
       "--r8-path",
       rebase_path(_r8_path, root_build_dir),
     ]
-    if (treat_warnings_as_errors) {
-      _args += [ "--warnings-as-errors" ]
-    }
 
     if (enable_proguard_obfuscation) {
       # This is generally the apk name, and serves to identify the mapping
@@ -1595,9 +1591,6 @@ if (enable_java_templates) {
           "--r8-jar-path",
           rebase_path(_r8_path, root_build_dir),
         ]
-        if (treat_warnings_as_errors) {
-          args += [ "--warnings-as-errors" ]
-        }
 
         if (enable_incremental_d8 && !(defined(invoker.disable_incremental) &&
                                        invoker.disable_incremental)) {
@@ -2025,14 +2018,11 @@ if (enable_java_templates) {
         "--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.requires_android) {
-        args += [ "--sdk-classpath-jars=@FileArg($_rebased_build_config:android:sdk_jars)" ]
-      }
       if (invoker.is_prebuilt) {
         args += [ "--is-prebuilt" ]
       }
-      if (treat_warnings_as_errors) {
-        args += [ "--warnings-as-errors" ]
+      if (invoker.requires_android) {
+        args += [ "--sdk-classpath-jars=@FileArg($_rebased_build_config:android:sdk_jars)" ]
       }
       if (defined(invoker.missing_classes_allowlist)) {
         args += [
@@ -3141,7 +3131,7 @@ if (enable_java_templates) {
       }
       if (_chromium_code) {
         args += [ "--chromium-code=1" ]
-        if (treat_warnings_as_errors &&
+        if (java_warnings_as_errors &&
             !defined(invoker.errorprone_expected_warning_regex)) {
           args += [ "--warnings-as-errors" ]
         }

build/config/compiler/BUILD.gn

@@ -43,6 +43,12 @@ if (is_nacl) {
 }
 
 declare_args() {
+  # Default to warnings as errors for default workflow, where we catch
+  # warnings with known toolchains. Allow overriding this e.g. for Chromium
+  # builds on Linux that could use a different version of the compiler.
+  # With GCC, warnings in no-Chromium code are always not treated as errors.
+  treat_warnings_as_errors = true
+
   # Normally, Android builds are lightly optimized, even for debug builds, to
   # keep binary size down. Setting this flag to true disables such optimization
   android_full_debug = false

build/config/compiler/compiler.gni

@@ -23,12 +23,6 @@ if (is_mac) {
 }
 
 declare_args() {
-  # Default to warnings as errors for default workflow, where we catch
-  # warnings with known toolchains. Allow overriding this e.g. for Chromium
-  # builds on Linux that could use a different version of the compiler.
-  # With GCC, warnings in no-Chromium code are always not treated as errors.
-  treat_warnings_as_errors = true
-
   # How many symbols to include in the build. This affects the performance of
   # the build since the symbols are large and dealing with them is slow.
   #   2 means regular build with symbols.

tools/mb/mb_config.pyl

@@ -1196,7 +1196,7 @@
     ],
 
     'android_debug_trybot_compile_only': [
-      'android', 'debug_bot', 'compile_only',
+      'android', 'debug_bot', 'compile_only', 'java_warnings_as_errors',
     ],
 
     'android_debug_trybot_compile_only_arm64_fastbuild': [
@@ -2621,6 +2621,10 @@
       'args_file': '//build/args/chromeos/eve-arc-r.gni',
     },
 
+    'java_warnings_as_errors': {
+      'gn_args': ('java_warnings_as_errors=true'),
+    },
+
     'enable_vulkan': {
       'gn_args': 'enable_vulkan=true',
     },