Skip to content

[various] Enable warnings as errors, and all warnings, for Android lint #3648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stuartmorgan-g merged 7 commits into from
Apr 7, 2023
Merged

[various] Enable warnings as errors, and all warnings, for Android lint #3648

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e54d7f3
Enable all warnings and warnings as errors in all plugins
stuartmorgan-g Apr 5, 2023
03dd9ca
New baselines
stuartmorgan-g Apr 5, 2023
0fed722
Add a missed InvalidPackage suppression
stuartmorgan-g Apr 5, 2023
af01e52
revert accidental changes
stuartmorgan-g Apr 5, 2023
1f880d8
Add tooling check for new config
stuartmorgan-g Apr 6, 2023
47aaaa8
Merge branch 'main' into android-lint-werror
stuartmorgan-g Apr 7, 2023
06e9068
Baseline update
stuartmorgan-g Apr 7, 2023
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
Prev Previous commit
Next Next commit
Add tooling check for new config
  • Loading branch information
@stuartmorgan-g
stuartmorgan-g committed Apr 6, 2023
commit 1f880d8169692d5f2c09595c448d089e2a19f010
69 changes: 53 additions & 16 deletions script/tool/lib/src/lint_android_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,28 @@ class LintAndroidCommand extends PackageLoopingCommand {
}

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);

if (!project.isConfigured()) {
// TODO(stuartmorgan): Replace this with a --config-only build once
// that's available on stable.
return PackageResult.fail(<String>['Build examples before linting']);
}

Expand All @@ -61,25 +78,13 @@ class LintAndroidCommand extends PackageLoopingCommand {
failed = true;
}

// In addition to running the Gradle lint step, also ensure that the
// example project is configured to build with javac lints enabled and
// 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.
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).
if (!gradleBuildContents.any(
(String line) => line.contains('project(":$packageName")')) ||
!gradleBuildContents.any((String line) =>
line.contains('options.compilerArgs') &&
line.contains('-Xlint') &&
line.contains('-Werror'))) {
if (!_exampleGradleHasJavacLintConfig(example, packageName)) {
failed = true;
printError('The example '
'${getRelativePosixPath(example.directory, from: package.directory)} '
'"${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('''
Expand All @@ -96,4 +101,36 @@ gradle.projectsEvaluated {

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
Copy link
Contributor

@reidbaker reidbaker Apr 12, 2023

Choose a reason for hiding this comment

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

Consider writing this match to ignore lines that are commented out.

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Apr 13, 2023

Choose a reason for hiding this comment

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

I can make that change if you want; there are potential downsides though. The current approach is to enforce that we haven't just forgotten to set this up when adding a new package. Allowing commenting it out allows for someone to explicitly say "I know that this is supposed to be set up, but I'm turning it off for now for a specific reason". I actually did exactly that with the javac part of this file previously, where for packages that weren't ready yet I added the line it was looking for commented out, with a TODO referencing an issue.

That may not be a useful thing to support for this though, where we can use the baseline file as an escape hatch.

Copy link
Contributor

@reidbaker reidbaker Apr 17, 2023

Choose a reason for hiding this comment

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

If we want to support disabling a check that makes sense. I guess I think of commented out lines passing a check that is expected as a foot gun. I would expect something like # ignore checkAllWarnings to be how it was disabled.

I don't think it is important enough to go back and rework unless you happen to be working on the code already.

.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'));
}
}
86 changes: 79 additions & 7 deletions script/tool/test/lint_android_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,46 @@ void main() {
runner.addCommand(command);
});

void writeFakeBuildGradle(RepositoryPackage example, String pluginName,
void writeFakePluginBuildGradle(RepositoryPackage plugin,
{bool warningsConfigured = true}) {
const String warningConfig = '''
lintOptions {
checkAllWarnings true
warningsAsErrors true
disable 'AndroidGradlePluginVersion', 'InvalidPackage', 'GradleDependency'
baseline file("lint-baseline.xml")
}
''';
final File gradleFile = plugin
.platformDirectory(FlutterPlatform.android)
.childFile('build.gradle');
gradleFile.createSync(recursive: true);
gradleFile.writeAsStringSync('''
android {
compileSdkVersion 33

defaultConfig {
minSdkVersion 16
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
}
${warningsConfigured ? warningConfig : ''}
testOptions {
unitTests.returnDefaultValues = true
unitTests.all {
testLogging {
events "passed", "skipped", "failed", "standardOut", "standardError"
outputs.upToDateWhen {false}
showStandardStreams = true
}
}
}
}

''');
}

void writeFakeExampleBuildGradle(
RepositoryPackage example, String pluginName,
{bool warningsConfigured = true}) {
final String warningConfig = '''
gradle.projectsEvaluated {
Expand Down Expand Up @@ -80,7 +119,8 @@ ${warningsConfigured ? warningConfig : ''}
], platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakeBuildGradle(plugin.getExamples().first, 'plugin1');
writeFakePluginBuildGradle(plugin);
writeFakeExampleBuildGradle(plugin.getExamples().first, 'plugin1');

final Directory androidDir =
plugin.getExamples().first.platformDirectory(FlutterPlatform.android);
Expand Down Expand Up @@ -118,8 +158,9 @@ ${warningsConfigured ? warningConfig : ''}
platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakePluginBuildGradle(plugin);
for (final RepositoryPackage example in plugin.getExamples()) {
writeFakeBuildGradle(example, 'plugin1');
writeFakeExampleBuildGradle(example, 'plugin1');
}

final Iterable<Directory> exampleAndroidDirs = plugin.getExamples().map(
Expand Down Expand Up @@ -150,10 +191,11 @@ ${warningsConfigured ? warningConfig : ''}
});

test('fails if gradlew is missing', () async {
createFakePlugin('plugin1', packagesDir,
final RepositoryPackage plugin = createFakePlugin('plugin1', packagesDir,
platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakePluginBuildGradle(plugin);

Error? commandError;
final List<String> output = await runCapturingPrint(
Expand All @@ -178,7 +220,8 @@ ${warningsConfigured ? warningConfig : ''}
], platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakeBuildGradle(plugin.getExamples().first, 'plugin1');
writeFakePluginBuildGradle(plugin);
writeFakeExampleBuildGradle(plugin.getExamples().first, 'plugin1');

final String gradlewPath = plugin
.getExamples()
Expand Down Expand Up @@ -206,14 +249,43 @@ ${warningsConfigured ? warningConfig : ''}
));
});

test('fails if gradle-driven lint-warnings-as-errors is missing', () async {
final RepositoryPackage plugin =
createFakePlugin('plugin1', packagesDir, extraFiles: <String>[
'example/android/gradlew',
], platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakePluginBuildGradle(plugin, warningsConfigured: false);
writeFakeExampleBuildGradle(plugin.getExamples().first, 'plugin1');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['lint-android'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(
<Matcher>[
contains('This plugin is not configured to enable all '
'Gradle-driven lint warnings and treat them as errors.'),
contains('The following packages had errors:'),
],
));
});

test('fails if javac lint-warnings-as-errors is missing', () async {
final RepositoryPackage plugin =
createFakePlugin('plugin1', packagesDir, extraFiles: <String>[
'example/android/gradlew',
], platformSupport: <String, PlatformDetails>{
platformAndroid: const PlatformDetails(PlatformSupport.inline)
});
writeFakeBuildGradle(plugin.getExamples().first, 'plugin1',
writeFakePluginBuildGradle(plugin);
writeFakeExampleBuildGradle(plugin.getExamples().first, 'plugin1',
warningsConfigured: false);

Error? commandError;
Expand All @@ -227,7 +299,7 @@ ${warningsConfigured ? warningConfig : ''}
output,
containsAllInOrder(
<Matcher>[
contains('The example example is not configured to treat javac '
contains('The example "example" is not configured to treat javac '
'lints and warnings as errors.'),
contains('The following packages had errors:'),
],
Expand Down