Android: Use a unified lint target

Lint is no longer implicitly run as part of an apk or bundle target. If
a specific apk or bundle wants to turn on its own lint and have that run
automatically on every build, the enable_lint flag can be used. See:
"boundary_interface_example_apk".

A new target //chrome/android:android_lint (or simply "android_lint")
has been added. This currently runs either monochrome_public_bundle's
lint task or a list of default targets if such a list is defined.

For chrome apk/bundle targets based on chrome_public_apk_tmpl.gni, they
will automatically get a data_dep added on the new unified lint target
so most devs' workflow will not need to change.

APKs in for {remoting,cronet,cast} have enable_lint added explicitly in
this CL to preserve their existing behaviour.

NewApi and VisibleForTests checks have been disabled for test apks, with
a TODO for turning at least NewApi back on.

Bug: 1108791
Fixed: 1108791
Change-Id: I6534fcd6dc7af1d7a12720a98f8d0d09bee8f161
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2318549
Commit-Queue: Peter Wen <wnwen@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Auto-Submit: Peter Wen <wnwen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792728}

android_webview/support_library/boundary_interfaces/BUILD.gn

@@ -56,5 +56,9 @@ android_apk("boundary_interface_example_apk") {
   # minSdkVersion of the support library increases, so should this value. See
   # http://crbug.com/828184 for more details.
   min_sdk_version = 14
+
+  # Explicitly enable lint for this apk.
+  enable_lint = true
+
   deps = [ ":boundary_interface_java" ]
 }

build/android/gyp/lint.py

@@ -36,6 +36,13 @@ _DISABLED_FOR_TESTS = [
     # Test targets often use the same strings target and resources target as the
     # production targets but may not use all of them.
     "UnusedResources",
+    # TODO(wnwen): Turn this back on since to crash it would require running on
+    #     a device with all the various minSdkVersions.
+    # Real NewApi violations crash the app, so the only ones that lint catches
+    # but tests still succeed are false positives.
+    "NewApi",
+    # Tests should be allowed to access these methods/classes.
+    "VisibleForTests",
 ]
 
 _RES_ZIP_DIR = 'RESZIPS'

build/config/android/internal_rules.gni

@@ -998,13 +998,8 @@ if (enable_java_templates) {
         _suppressions_file = "//build/android/lint/suppressions.xml"
       }
 
-      # Allow callers to override lint's minSdkVersion via lint_min_sdk_version
-      # so that it can be different from the caller's min_sdk_version.
       _min_sdk_version = default_min_sdk_version
-      if (defined(invoker.lint_min_sdk_version)) {
-        _min_sdk_version = invoker.lint_min_sdk_version
-        not_needed(invoker, [ "min_sdk_version" ])
-      } else if (defined(invoker.min_sdk_version)) {
+      if (defined(invoker.min_sdk_version)) {
         _min_sdk_version = invoker.min_sdk_version
       }
 

build/config/android/rules.gni

@@ -3324,50 +3324,30 @@ if (enable_java_templates) {
       _final_deps += [ ":$_apk_operations_target_name" ]
     }
 
-    if (defined(invoker.enable_lint)) {
-      # Allow individual apks to opt-in to lint.
-      _enable_lint = invoker.enable_lint && !disable_android_lint
-    } else {
-      # This is necessary so we only lint chromium apks.
-      if (defined(invoker.chromium_code)) {
-        _chromium_code = invoker.chromium_code
-      } else {
-        # Default based on whether target is in third_party.
-        _chromium_code =
-            filter_exclude([ get_label_info(":$target_name", "dir") ],
-                           [ "*\bthird_party\b*" ]) != []
-      }
-
-      # Bundle modules get linted via android_app_bundle targets.
-      # Skip testonly apks since many of them trigger lint warnings.
-      _enable_lint = _chromium_code && !_is_bundle_module &&
-                     (!defined(testonly) || !testonly) && !disable_android_lint
-    }
+    _enable_lint = defined(invoker.enable_lint) && invoker.enable_lint &&
+                   !disable_android_lint
     if (_enable_lint) {
-      # Only run lint on final apk targets so that lint can run on all source
-      # files and all resources. This is both faster and more correct for checks
-      # like UnusedResources and IntDef.
-      _android_lint_target = "${target_name}__lint"
-      android_lint(_android_lint_target) {
+      android_lint("${target_name}__lint") {
         forward_variables_from(invoker,
                                [
-                                 "lint_min_sdk_version",
                                  "lint_suppressions_dep",
                                  "lint_suppressions_file",
                                  "manifest_package",
                                  "min_sdk_version",
                                ])
+        if (defined(invoker.lint_min_sdk_version)) {
+          min_sdk_version = invoker.lint_min_sdk_version
+        }
         build_config = _build_config
         build_config_dep = ":$_build_config_target"
         deps = [ ":$_java_target" ]
       }
     } else {
+      # TODO(wnwen): Remove manifest_package when all usages are removed.
       not_needed(invoker,
                  [
-                   "lint_min_sdk_version",
-                   "lint_suppressions_dep",
-                   "lint_suppressions_file",
                    "manifest_package",
+                   "lint_min_sdk_version",
                  ])
     }
 
@@ -3391,7 +3371,7 @@ if (enable_java_templates) {
       data_deps += _all_native_libs_deps
 
       if (_enable_lint) {
-        data_deps += [ ":$_android_lint_target" ]
+        data_deps += [ ":${target_name}__lint" ]
       }
 
       if (_incremental_apk) {
@@ -5002,31 +4982,30 @@ if (enable_java_templates) {
       }
     }
 
-    if (!disable_android_lint) {
-      # Only run lint on final output targets so that lint can run on all source
-      # files and all resources. This is both faster and more correct for checks
-      # like UnusedResources and IntDef.
-      _android_lint_target = "${_target_name}__lint"
-      android_lint(_android_lint_target) {
+    _enable_lint = defined(invoker.enable_lint) && invoker.enable_lint &&
+                   !disable_android_lint
+    if (_enable_lint) {
+      android_lint("${target_name}__lint") {
         forward_variables_from(invoker,
                                [
-                                 "lint_min_sdk_version",
                                  "lint_suppressions_dep",
                                  "lint_suppressions_file",
                                  "manifest_package",
                                  "min_sdk_version",
                                ])
+        if (defined(invoker.lint_min_sdk_version)) {
+          min_sdk_version = invoker.lint_min_sdk_version
+        }
         build_config = _build_config
         build_config_dep = ":$_build_config_target"
         deps = _module_java_targets
       }
     } else {
+      # TODO(wnwen): Remove manifest_package when all usages are removed.
       not_needed(invoker,
                  [
-                   "lint_min_sdk_version",
-                   "lint_suppressions_dep",
-                   "lint_suppressions_file",
                    "manifest_package",
+                   "lint_min_sdk_version",
                  ])
     }
 
@@ -5038,11 +5017,11 @@ if (enable_java_templates) {
       if (defined(_size_info_target)) {
         public_deps += [ ":$_size_info_target" ]
       }
-      if (!disable_android_lint) {
+      if (_enable_lint) {
         if (!defined(data_deps)) {
           data_deps = []
         }
-        data_deps += [ ":$_android_lint_target" ]
+        data_deps += [ ":${target_name}__lint" ]
       }
     }
 

chrome/android/BUILD.gn

@@ -2312,8 +2312,9 @@ template("monochrome_test_apk_tmpl") {
 }
 
 chrome_test_apk_tmpl("chrome_public_test_apk") {
-  # TODO(wnwen): Re-enable when new lint failures are fixed.
+  # TODO(wnwen): Re-enable when new lint failures are disabled for test targets.
   #enable_lint = true
+
   apk_name = "ChromePublicTest"
   android_manifest = chrome_public_test_apk_manifest
   android_manifest_dep = ":chrome_public_test_apk_manifest"
@@ -2705,9 +2706,11 @@ template("monochrome_or_trichrome_public_bundle_tmpl") {
   chrome_bundle(target_name) {
     forward_variables_from(invoker,
                            [
+                             "enable_lint",
                              "include_32_bit_webview",
                              "include_64_bit_webview",
                              "is_64_bit_browser",
+                             "lint_min_sdk_version",
                              "static_library_provider",
                              "static_library_synchronized_proguard",
                              "expected_libs_and_assets",
@@ -2727,16 +2730,29 @@ template("monochrome_or_trichrome_public_bundle_tmpl") {
   }
 }
 
+group("android_lint") {
+  if (!disable_android_lint) {
+    if (defined(default_chrome_lint_targets)) {
+      deps = default_chrome_lint_targets
+    } else {
+      deps = [ ":monochrome_public_bundle__lint" ]
+    }
+  }
+}
+
 # Public webview targets don't work with non-public sdks.
 # https://crbug.com/1000763
 monochrome_or_trichrome_public_bundle_tmpl("monochrome_public_bundle") {
-  bundle_suffix = ""
+  # Monochrome bundle is used as our unified lint target, so it needs to set the
+  # lowest shipping minSdkVersion to catch all potential NewApi errors.
+  lint_min_sdk_version = default_min_sdk_version
+  enable_lint = true
 
+  bundle_suffix = ""
   if (android_64bit_target_cpu) {
     is_64_bit_browser = false
     include_64_bit_webview = true
   }
-
   if (_enable_manifest_verification) {
     expected_android_manifest =
         "expectations/monochrome_public_bundle.AndroidManifest.expected"

chrome/android/chrome_public_apk_tmpl.gni

@@ -367,6 +367,16 @@ template("chrome_public_common_apk_or_module_tmpl") {
 
     deps += [ "//chrome/android:chrome_all_java" ]
 
+    # Prefer to add this data_dep on the final target instead of java targets
+    # like chrome_all_java so that all other targets can build in parallel with
+    # lint.
+    if (!disable_android_lint) {
+      if (!defined(data_deps)) {
+        data_deps = []
+      }
+      data_deps += [ "//chrome/android:android_lint" ]
+    }
+
     if (!defined(version_code)) {
       if (_is_trichrome) {
         version_code = trichrome_version_code

chrome/android/modules/chrome_bundle_tmpl.gni

@@ -77,6 +77,7 @@ template("chrome_bundle") {
                              "base_module_target",
                              "bundle_name",
                              "compress_shared_libraries",
+                             "enable_lint",
                              "expected_libs_and_assets",
                              "expected_libs_and_assets_base",
                              "expected_proguard_config",

chromecast/BUILD.gn

@@ -645,6 +645,8 @@ if (is_android) {
   }
 
   android_apk("cast_shell_apk") {
+    enable_lint = true
+
     apk_name = "CastShell"
     android_manifest = "$root_gen_dir/cast_shell_manifest/AndroidManifest.xml"
     android_manifest_dep = "//chromecast/browser/android:cast_shell_manifest"

components/cronet/android/BUILD.gn

@@ -1018,6 +1018,9 @@ if (!is_component_build) {
   }
 
   instrumentation_test_apk("cronet_test_instrumentation_apk") {
+    # This is the only Cronet APK with lint enabled. This one was chosen because
+    # it depends on basically all source files.
+    enable_lint = true
     apk_name = "CronetTestInstrumentation"
     android_manifest = "test/javatests/AndroidManifest.xml"
     min_sdk_version = _cronet_min_sdk_version

remoting/android/BUILD.gn

@@ -146,6 +146,8 @@ remoting_android_client_java_tmpl("remoting_android_client_java") {
 }
 
 remoting_apk_tmpl("remoting_apk") {
+  enable_lint = true
+
   apk_name = "Chromoting"
   sources = [ "//remoting/android/apk/src/org/chromium/chromoting/RemotingApplication.java" ]
   deps = [