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

This reverts commit 57deecbb.

Reason for reland: Suppressed cast lint warning

Reverted in: 339e6581.

> 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.

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

Bug: 1029357
Change-Id: I56c1349c6e823737aa8a59a8e961e5a518667ba0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308868
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790410}

build/android/gyp/bytecode_processor.py

@@ -32,6 +32,9 @@ 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)
 
@@ -57,7 +60,10 @@ def main(argv):
   cmd += args.full_classpath_jars
   cmd += [str(len(args.full_classpath_gn_targets))]
   cmd += args.full_classpath_gn_targets
-  subprocess.check_call(cmd)
+  build_utils.CheckOutput(cmd,
+                          print_stdout=True,
+                          fail_func=None,
+                          fail_on_output=args.warnings_as_errors)
 
   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_TURN_OFF = [
+ERRORPRONE_WARNINGS_TO_DISABLE = [
     # These should really be turned on.
     'ParameterNotNullable',
     'CollectionUndefinedEquality',
@@ -175,8 +175,8 @@ ERRORPRONE_WARNINGS_TO_TURN_OFF = [
 
 # Full list of checks: https://errorprone.info/bugpatterns
 # Only those marked as "experimental" need to be listed here in order to be
-# enabled. We build with -Werror, so all default checks cause builds to fail.
-ERRORPRONE_WARNINGS_TO_ERROR = [
+# enabled.
+ERRORPRONE_WARNINGS_TO_ENABLE = [
     'BinderIdentityRestoredDangerously',
     'EmptyIf',
     'EqualsBrokenForNull',
@@ -509,7 +509,8 @@ 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)
+                              stderr_filter=stderr_filter,
+                              fail_on_output=options.warnings_as_errors)
       end = time.time() - start
       logging.info('Java compilation took %ss', end)
 
@@ -671,12 +672,13 @@ def main(argv):
   if options.enable_errorprone:
     # All errorprone args are passed space-separated in a single arg.
     errorprone_flags = ['-Xplugin:ErrorProne']
-    for warning in ERRORPRONE_WARNINGS_TO_TURN_OFF:
+    # 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:
       errorprone_flags.append('-Xep:{}:OFF'.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')
+    for warning in ERRORPRONE_WARNINGS_TO_ENABLE:
+      errorprone_flags.append('-Xep:{}:WARN'.format(warning))
     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
@@ -694,14 +696,6 @@ 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:
@@ -737,9 +731,9 @@ def main(argv):
       options.jar_path + '.info',
   ]
 
-  input_strings = javac_cmd + javac_args + options.classpath + java_files
-  if options.jar_info_exclude_globs:
-    input_strings.append(options.jar_info_exclude_globs)
+  input_strings = javac_cmd + javac_args + options.classpath + java_files + [
+      options.warnings_as_errors, options.jar_info_exclude_globs
+  ]
 
   md5_check.CallAndWriteDepfileIfStale(
       lambda: _OnStaleMd5(options, javac_cmd, javac_args, java_files),

build/android/gyp/dex.py

@@ -88,6 +88,12 @@ 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(
@@ -109,11 +115,6 @@ 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:
@@ -165,7 +166,7 @@ def CreateStderrFilter(show_desugar_default_interface_warnings):
   return filter_stderr
 
 
-def _RunD8(dex_cmd, input_paths, output_path,
+def _RunD8(dex_cmd, input_paths, output_path, warnings_as_errors,
            show_desugar_default_interface_warnings):
   dex_cmd = dex_cmd + ['--output', output_path] + input_paths
 
@@ -182,7 +183,9 @@ def _RunD8(dex_cmd, input_paths, output_path,
 
     # stdout sometimes spams with things like:
     # Stripped invalid locals information from 1 method.
-    build_utils.CheckOutput(dex_cmd, stderr_filter=stderr_filter)
+    build_utils.CheckOutput(dex_cmd,
+                            stderr_filter=stderr_filter,
+                            fail_on_output=warnings_as_errors)
 
 
 def _EnvWithArtLibPath(binary_path):
@@ -363,6 +366,7 @@ 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')
 
@@ -446,6 +450,7 @@ 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]
 
-    subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+    build_utils.CheckOutput(cmd, print_stdout=True)
     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,
-             can_fail_build=False,
+             warnings_as_errors=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 can_fail_build:
-      raise
-    else:
-      print(e)
+      if warnings_as_errors:
+        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,10 +277,9 @@ def _ParseArgs(argv):
                       'targets.')
   parser.add_argument('--manifest-package',
                       help='Package name of the AndroidManifest.xml.')
-  parser.add_argument('--can-fail-build',
+  parser.add_argument('--warnings-as-errors',
                       action='store_true',
-                      help='If set, script will exit with nonzero exit status'
-                      ' if lint errors are present')
+                      help='Treat all warnings as errors.')
   parser.add_argument('--silent',
                       action='store_true',
                       help='If set, script will not log anything.')
@@ -347,7 +346,7 @@ def main():
            args.lint_gen_dir,
            args.baseline,
            testonly_target=args.testonly,
-           can_fail_build=args.can_fail_build,
+           warnings_as_errors=args.warnings_as_errors,
            silent=args.silent)
   logging.info('Creating stamp file')
   build_utils.Touch(args.stamp)

build/android/gyp/proguard.py

@@ -155,6 +155,9 @@ 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.')
@@ -257,16 +260,6 @@ 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,
@@ -303,6 +296,11 @@ 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',
@@ -351,18 +349,13 @@ 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 = _CreateStderrFilter(
+      stderr_filter = dex.CreateStderrFilter(
           options.show_desugar_default_interface_warnings)
       build_utils.CheckOutput(cmd,
-                              env=env,
                               print_stdout=print_stdout,
-                              stderr_filter=stderr_filter)
+                              stderr_filter=stderr_filter,
+                              fail_on_output=options.warnings_as_errors)
     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,10 +234,14 @@ 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()
@@ -252,7 +256,7 @@ def CheckOutput(args, cwd=None, env=None,
   if stderr_filter is not None:
     stderr = stderr_filter(stderr)
 
-  if fail_func(child.returncode, stderr):
+  if fail_func and fail_func(child.returncode, stderr):
     raise CalledProcessError(cwd, args, stdout + stderr)
 
   if print_stdout:
@@ -260,6 +264,21 @@ def CheckOutput(args, cwd=None, env=None,
   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,9 +239,6 @@ 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,6 +6,7 @@
 # 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")
@@ -1037,8 +1038,8 @@ if (enable_java_templates) {
         args += [ "--manifest-package=${invoker.manifest_package}" ]
       }
 
-      if (java_warnings_as_errors) {
-        args += [ "--can-fail-build" ]
+      if (treat_warnings_as_errors) {
+        args += [ "--warnings-as-errors" ]
       }
 
       _stamp_path = "$target_out_dir/$target_name/build.lint.stamp"
@@ -1133,6 +1134,9 @@ 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
@@ -1591,6 +1595,9 @@ 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)) {
@@ -2018,11 +2025,14 @@ 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 (invoker.requires_android) {
-        args += [ "--sdk-classpath-jars=@FileArg($_rebased_build_config:android:sdk_jars)" ]
+      if (treat_warnings_as_errors) {
+        args += [ "--warnings-as-errors" ]
       }
       if (defined(invoker.missing_classes_allowlist)) {
         args += [
@@ -3131,7 +3141,7 @@ if (enable_java_templates) {
       }
       if (_chromium_code) {
         args += [ "--chromium-code=1" ]
-        if (java_warnings_as_errors &&
+        if (treat_warnings_as_errors &&
             !defined(invoker.errorprone_expected_warning_regex)) {
           args += [ "--warnings-as-errors" ]
         }

build/config/compiler/BUILD.gn

@@ -43,12 +43,6 @@ 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,6 +23,12 @@ 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.

chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCommandLineHelper.java

@@ -66,6 +66,7 @@ public class CastCommandLineHelper {
      * Reads command line args from persistent storage and initializes the CommandLine with those
      * args. Does not initialize CommandLine if it has been already been done.
      */
+    @SuppressWarnings("VisibleForTests")  // For call to cmdline.hasSwitch()
     public static void initCommandLineWithSavedArgs(CommandLineInitializer commandLineInitializer) {
         // CommandLine is a singleton, so check whether CastCommandLineHelper has initialized it
         // already and do nothing if so. We keep track of this in a static variable so we can still

tools/mb/mb_config.pyl

@@ -1196,7 +1196,7 @@
     ],
 
     'android_debug_trybot_compile_only': [
-      'android', 'debug_bot', 'compile_only', 'java_warnings_as_errors',
+      'android', 'debug_bot', 'compile_only',
     ],
 
     'android_debug_trybot_compile_only_arm64_fastbuild': [
@@ -2621,10 +2621,6 @@
       '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',
     },