[tool] Move Android lint checks (flutter#3816)

Moves the checks for Android warning configuration from `lint-android`, where it made sense to put them at the time, to the new `check-gradle`, which is a newer command specifically for validating that our Gradle files are following best practices.

Makes minor changes to the Pigeon platform test projects to make them conform to the checks, since they are now included in the run. The changes shouldn't actually change the behavior of the Pigeon tests.

Author: stuartmorgan-g

packages/pigeon/platform_tests/alternate_language_test_plugin/android/build.gradle

@@ -57,7 +57,3 @@ android {
         testImplementation "org.mockito:mockito-core:5.1.1"
     }
 }
-
-project.getTasks().withType(JavaCompile){
-    options.compilerArgs.addAll(["-Xlint:all", "-Werror"])
-}

packages/pigeon/platform_tests/alternate_language_test_plugin/example/android/build.gradle

@@ -29,3 +29,11 @@ subprojects {
 task clean(type: Delete) {
     delete rootProject.buildDir
 }
+
+gradle.projectsEvaluated {
+    project(":alternate_language_test_plugin") {
+        tasks.withType(JavaCompile) {
+            options.compilerArgs << "-Xlint:all" << "-Werror"
+        }
+    }
+}

packages/pigeon/platform_tests/test_plugin/android/build.gradle

@@ -58,6 +58,12 @@ android {
         }
     }
 
+    lintOptions {
+        checkAllWarnings true
+        warningsAsErrors true
+        disable 'AndroidGradlePluginVersion', 'InvalidPackage', 'GradleDependency'
+    }
+
     dependencies {
         testImplementation 'junit:junit:4.+'
         testImplementation "io.mockk:mockk:1.13.5"

packages/pigeon/platform_tests/test_plugin/example/android/build.gradle

@@ -29,3 +29,11 @@ subprojects {
 task clean(type: Delete) {
     delete rootProject.buildDir
 }
+
+gradle.projectsEvaluated {
+    project(":test_plugin") {
+        tasks.withType(JavaCompile) {
+            options.compilerArgs << "-Xlint:all" << "-Werror"
+        }
+    }
+}

script/tool/lib/src/common/repository_package.dart

@@ -151,4 +151,19 @@ class RepositoryPackage {
         .map((FileSystemEntity entity) =>
             RepositoryPackage(entity as Directory));
   }
+
+  /// Returns the package that this package is a part of, if any.
+  ///
+  /// Currently this is limited to checking up two directories, since that
+  /// covers all the example structures currently used.
+  RepositoryPackage? getEnclosingPackage() {
+    final Directory parent = directory.parent;
+    if (isPackage(parent)) {
+      return RepositoryPackage(parent);
+    }
+    if (isPackage(parent.parent)) {
+      return RepositoryPackage(parent.parent);
+    }
+    return null;
+  }
 }

script/tool/lib/src/gradle_check_command.dart

@@ -6,6 +6,7 @@ import 'package:file/file.dart';
 
 import 'common/core.dart';
 import 'common/package_looping_command.dart';
+import 'common/plugin_utils.dart';
 import 'common/repository_package.dart';
 
 /// A command to enforce gradle file conventions and best practices.
@@ -84,6 +85,8 @@ class GradleCheckCommand extends PackageLoopingCommand {
         .childFile('AndroidManifest.xml');
   }
 
+  bool _isCommented(String line) => line.trim().startsWith('//');
+
   /// Validates the build.gradle file for a plugin
   /// (some_plugin/android/build.gradle).
   bool _validatePluginBuildGradle(RepositoryPackage package, File gradleFile) {
@@ -101,6 +104,9 @@ class GradleCheckCommand extends PackageLoopingCommand {
     if (!_validateSourceCompatibilityVersion(lines)) {
       succeeded = false;
     }
+    if (!_validateGradleDrivenLintConfig(package, lines)) {
+      succeeded = false;
+    }
     return succeeded;
   }
 
@@ -110,9 +116,16 @@ class GradleCheckCommand extends PackageLoopingCommand {
       RepositoryPackage package, File gradleFile) {
     print('${indentation}Validating '
         '${getRelativePosixPath(gradleFile, from: package.directory)}.');
-    // TODO(stuartmorgan): Move the -Xlint validation from lint_android_command
-    // to here.
-    return true;
+    final String contents = gradleFile.readAsStringSync();
+    final List<String> lines = contents.split('\n');
+
+    // This is tracked as a variable rather than a sequence of &&s so that all
+    // failures are reported at once, not just the first one.
+    bool succeeded = true;
+    if (!_validateJavacLintConfig(package, lines)) {
+      succeeded = false;
+    }
+    return succeeded;
   }
 
   /// Validates the app-level build.gradle for an example app (e.g.,
@@ -193,11 +206,9 @@ build.gradle "namespace" must match the "package" attribute in AndroidManifest.x
   /// lead to compile errors that show up for clients, but not in CI).
   bool _validateSourceCompatibilityVersion(List<String> gradleLines) {
     if (!gradleLines.any((String line) =>
-            line.contains('languageVersion') &&
-            !line.trim().startsWith('//')) &&
+            line.contains('languageVersion') && !_isCommented(line)) &&
         !gradleLines.any((String line) =>
-            line.contains('sourceCompatibility') &&
-            !line.trim().startsWith('//'))) {
+            line.contains('sourceCompatibility') && !_isCommented(line))) {
       const String errorMessage = '''
 build.gradle must set an explicit Java compatibility version.
 
@@ -225,4 +236,75 @@ for more details.''';
     }
     return true;
   }
+
+  /// Returns whether the given gradle content is configured to enable all
+  /// Gradle-driven lints (those checked by ./gradlew lint) and treat them as
+  /// errors.
+  bool _validateGradleDrivenLintConfig(
+      RepositoryPackage package, List<String> gradleLines) {
+    final List<String> gradleBuildContents = package
+        .platformDirectory(FlutterPlatform.android)
+        .childFile('build.gradle')
+        .readAsLinesSync();
+    if (!gradleBuildContents.any((String line) =>
+            line.contains('checkAllWarnings true') && !_isCommented(line)) ||
+        !gradleBuildContents.any((String line) =>
+            line.contains('warningsAsErrors true') && !_isCommented(line))) {
+      printError('${indentation}This package is not configured to enable all '
+          'Gradle-driven lint warnings and treat them as errors. '
+          'Please add the following to the lintOptions section of '
+          'android/build.gradle:');
+      print('''
+        checkAllWarnings true
+        warningsAsErrors true
+''');
+      return false;
+    }
+    return true;
+  }
+
+  /// Validates whether the given [example]'s gradle content is configured to
+  /// build its plugin target with javac lints enabled and treated as errors,
+  /// if the enclosing package is a plugin.
+  ///
+  /// This can only be called on example packages. (Plugin packages should not
+  /// be configured this way, since it would affect clients.)
+  ///
+  /// If [example]'s enclosing package is not a plugin package, this just
+  /// returns true.
+  bool _validateJavacLintConfig(
+      RepositoryPackage example, List<String> gradleLines) {
+    final RepositoryPackage enclosingPackage = example.getEnclosingPackage()!;
+    if (!pluginSupportsPlatform(platformAndroid, enclosingPackage,
+        requiredMode: PlatformSupport.inline)) {
+      return true;
+    }
+    final String enclosingPackageName = enclosingPackage.directory.basename;
+
+    // The check here is intentionally somewhat loose, to allow for the
+    // possibility of variations (e.g., not using Xlint:all in some cases, or
+    // passing other arguments).
+    if (!(gradleLines.any((String line) =>
+            line.contains('project(":$enclosingPackageName")')) &&
+        gradleLines.any((String line) =>
+            line.contains('options.compilerArgs') &&
+            line.contains('-Xlint') &&
+            line.contains('-Werror')))) {
+      printError('The example '
+          '"${getRelativePosixPath(example.directory, from: enclosingPackage.directory)}" '
+          'is not configured to treat javac lints and warnings as errors. '
+          'Please add the following to its build.gradle:');
+      print('''
+gradle.projectsEvaluated {
+    project(":$enclosingPackageName") {
+        tasks.withType(JavaCompile) {
+            options.compilerArgs << "-Xlint:all" << "-Werror"
+        }
+    }
+}
+''');
+      return false;
+    }
+    return true;
+  }
 }

script/tool/lib/src/lint_android_command.dart

@@ -38,22 +38,6 @@ class LintAndroidCommand extends PackageLoopingCommand {
           'Plugin does not have an Android implementation.');
     }
 
-    bool failed = false;
-
-    // Ensure that the plugin has a strict Gradle-driven lint configuration, so
-    // that this test actually catches most issues.
-    if (!_mainGradleHasLintConfig(package)) {
-      failed = true;
-      printError('This plugin is not configured to enable all Gradle-driven '
-          'lint warnings and treat them as errors. '
-          'Please add the following to the lintOptions section of '
-          'android/build.gradle:');
-      print('''
-        checkAllWarnings true
-        warningsAsErrors true
-''');
-    }
-
     for (final RepositoryPackage example in package.getExamples()) {
       final GradleProject project = GradleProject(example,
           processRunner: processRunner, platform: platform);
@@ -75,62 +59,10 @@ class LintAndroidCommand extends PackageLoopingCommand {
       // inline, and the rest have to be checked via the CI-uploaded artifact.
       final int exitCode = await project.runCommand('$packageName:lintDebug');
       if (exitCode != 0) {
-        failed = true;
+        return PackageResult.fail();
       }
-
-      // In addition to running the Gradle-driven lint step, also ensure that
-      // the example project is configured to build with javac lints enabled and
-      // treated as errors.
-      if (!_exampleGradleHasJavacLintConfig(example, packageName)) {
-        failed = true;
-        printError('The example '
-            '"${getRelativePosixPath(example.directory, from: package.directory)}" '
-            'is not configured to treat javac lints and warnings as errors. '
-            'Please add the following to its build.gradle:');
-        print('''
-gradle.projectsEvaluated {
-    project(":${package.directory.basename}") {
-        tasks.withType(JavaCompile) {
-            options.compilerArgs << "-Xlint:all" << "-Werror"
-        }
     }
-}
-''');
-      }
-    }
-
-    return failed ? PackageResult.fail() : PackageResult.success();
-  }
-
-  /// Returns whether the plugin project is configured to enable all Gradle
-  /// lints and treat them as errors.
-  bool _mainGradleHasLintConfig(RepositoryPackage package) {
-    final List<String> gradleBuildContents = package
-        .platformDirectory(FlutterPlatform.android)
-        .childFile('build.gradle')
-        .readAsLinesSync();
-    return gradleBuildContents
-            .any((String line) => line.contains('checkAllWarnings true')) &&
-        gradleBuildContents
-            .any((String line) => line.contains('warningsAsErrors true'));
-  }
 
-  /// Returns whether the example project is configured to build with javac
-  /// lints enabled and treated as errors.
-  bool _exampleGradleHasJavacLintConfig(
-      RepositoryPackage example, String pluginPackageName) {
-    final List<String> gradleBuildContents = example
-        .platformDirectory(FlutterPlatform.android)
-        .childFile('build.gradle')
-        .readAsLinesSync();
-    // The check here is intentionally somewhat loose, to allow for the
-    // possibility of variations (e.g., not using Xlint:all in some cases, or
-    // passing other arguments).
-    return gradleBuildContents.any(
-            (String line) => line.contains('project(":$pluginPackageName")')) &&
-        gradleBuildContents.any((String line) =>
-            line.contains('options.compilerArgs') &&
-            line.contains('-Xlint') &&
-            line.contains('-Werror'));
+    return PackageResult.success();
   }
 }