Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,3 @@ android {
testImplementation "org.mockito:mockito-core:5.1.1"
}
}

project.getTasks().withType(JavaCompile){
options.compilerArgs.addAll(["-Xlint:all", "-Werror"])
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was here instead of in the example build.gradle because unlike with all of our actual plugins we don't need to worry about breaking clients, and so it was easier to just have it here. But now that we're enforcing uniformity, it's easier to make it match the rest of the plugins instead of carving out an exception.

Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
}
15 changes: 15 additions & 0 deletions script/tool/lib/src/common/repository_package.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@tarrinneal tarrinneal Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a "package" or an "example" or both? Could be a little clearer here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's vague because it could be anything when you call it. E.g., it's valid to call this on path_provider_android, in which case it will return null because the path_provider_android package isn't part of another package.

If it returns a non-null value it's probably an example in practice in our repo, but even that's not guaranteed. Take https://github.com/flutter/packages/tree/main/packages/pigeon/platform_tests/shared_test_plugin_code for example, which is not an example, but is a package that's part of the pigeon package.

///
/// 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;
}
}
96 changes: 89 additions & 7 deletions script/tool/lib/src/gradle_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -101,6 +104,9 @@ class GradleCheckCommand extends PackageLoopingCommand {
if (!_validateSourceCompatibilityVersion(lines)) {
succeeded = false;
}
if (!_validateGradleDrivenLintConfig(package, lines)) {
succeeded = false;
}
return succeeded;
}

Expand All @@ -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.,
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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;
}
}
72 changes: 2 additions & 70 deletions script/tool/lib/src/lint_android_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
}
}
Loading