Skip to content
Open
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
53 changes: 43 additions & 10 deletions script/tool/lib/src/gradle_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -356,21 +356,19 @@ build.gradle "namespace" must match the "package" attribute in AndroidManifest.x
/// than using whatever the client's local toolchaing defaults to (which can
/// lead to compile errors that show up for clients, but not in CI).
bool _validateCompatibilityVersions(List<String> gradleLines) {
const String requiredJavaVersion = '17';
const int minimumJavaVersion = 17;
final bool hasLanguageVersion = gradleLines.any((String line) =>
line.contains('languageVersion') && !_isCommented(line));
final bool hasCompabilityVersions = gradleLines.any((String line) =>
line.contains(
'sourceCompatibility = JavaVersion.VERSION_$requiredJavaVersion') &&
line.contains('sourceCompatibility = JavaVersion.VERSION_') &&
!_isCommented(line)) &&
// Newer toolchains default targetCompatibility to the same value as
// sourceCompatibility, but older toolchains require it to be set
// explicitly. The exact version cutoff (and of which piece of the
// toolchain; likely AGP) is unknown; for context see
// https://github.com/flutter/flutter/issues/125482
gradleLines.any((String line) =>
line.contains(
'targetCompatibility = JavaVersion.VERSION_$requiredJavaVersion') &&
line.contains('targetCompatibility = JavaVersion.VERSION_') &&
!_isCommented(line));
if (!hasLanguageVersion && !hasCompabilityVersions) {
const String javaErrorMessage = '''
Expand All @@ -379,15 +377,15 @@ build.gradle(.kts) must set an explicit Java compatibility version.
This can be done either via "sourceCompatibility"/"targetCompatibility":
android {
compileOptions {
sourceCompatibility = JavaVersion.VERSION_$requiredJavaVersion
targetCompatibility = JavaVersion.VERSION_$requiredJavaVersion
sourceCompatibility = JavaVersion.VERSION_$minimumJavaVersion
targetCompatibility = JavaVersion.VERSION_$minimumJavaVersion
}
}

or "toolchain":
java {
toolchain {
languageVersion = JavaLanguageVersion.of($requiredJavaVersion)
languageVersion = JavaLanguageVersion.of($minimumJavaVersion)
}
}

Expand All @@ -403,7 +401,7 @@ for more details.''';
line.contains('kotlinOptions') && !_isCommented(line);
final bool hasKotlinOptions = gradleLines.any(isKotlinOptions);
final bool kotlinOptionsUsesJavaVersion = gradleLines.any((String line) =>
line.contains('jvmTarget = JavaVersion.VERSION_$requiredJavaVersion') &&
line.contains('jvmTarget = JavaVersion.VERSION_') &&
!_isCommented(line));
// Either does not set kotlinOptions or does and uses non-string based syntax.
if (hasKotlinOptions && !kotlinOptionsUsesJavaVersion) {
Expand All @@ -421,7 +419,7 @@ If build.gradle(.kts) sets jvmTarget then it must use JavaVersion syntax.
Good:
android {
kotlinOptions {
jvmTarget = JavaVersion.VERSION_$requiredJavaVersion.toString()
jvmTarget = JavaVersion.VERSION_$minimumJavaVersion.toString()
}
}
BAD:
Expand All @@ -432,6 +430,41 @@ If build.gradle(.kts) sets jvmTarget then it must use JavaVersion syntax.
return false;
}

final List<String> javaVersions = <String>[];
// Some java versions have the format VERSION_1_8 but we dont need to handle those
// because they are below the minimum.
final RegExp javaVersionMatcher =
RegExp(r'JavaVersion.VERSION_(?<javaVersion>\d+)');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than tagging this on at the end, you could restructure the earlier somewhat to avoid having multiple passes over the same file looking for the same lines different ways, which I feel like makes the code harder to follow. You could start this whole method with something like (this is freehand so is probably wrong, but should give the idea):

final Map<String, String> javaVersionValues = <String, String>{};
final RegExp javaVersionMatcher = 
    RegExp(r'(?<value>\w+)\s*=\s*JavaVersion.VERSION_(?<javaVersion>\d+)');
for (final String line in gradleLines) {
  final RegExpMatch? match = javaVersionMatcher.firstMatch(line);
  if (!_isCommented(line) && match != null) {
    if (foundVersion != null) {
      javaVersions[match.namedGroup('value')!] = match.namedGroup('javaVersion')!;
    }
  }
}

Then all of the gradleLines.any((String line) => line.contains(...) checks above are just key checks in that map, and you can use the same map below for the version checks.

for (final String line in gradleLines) {
final RegExpMatch? match = javaVersionMatcher.firstMatch(line);
if (!_isCommented(line) && match != null) {
final String? foundVersion = match.namedGroup('javaVersion');
if (foundVersion != null) {
javaVersions.add(foundVersion);
}
}
}
if (javaVersions.isNotEmpty) {
final int version = int.parse(javaVersions.first);
if (version < minimumJavaVersion) {
final String minimumJavaVersionError = '''
build.gradle(.kts) uses "JavaVersion.VERSION_$version".
Which is below the minimum required. Use at least "JavaVersion.VERSION_$minimumJavaVersion".
''';
printError(
'$indentation${minimumJavaVersionError.split('\n').join('\n$indentation')}');
return false;
}
if (!javaVersions.every((String element) => element == '$version')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would strongly prefer making this the first check instead of the second, since if you don't already know there's going to be a check that they all match, the check above that only the first element is the right version seems wrong.

const String javaVersionAlignmentError = '''
If build.gradle(.kts) uses JavaVersion.* versions must be the same.
''';
printError(
'$indentation${javaVersionAlignmentError.split('\n').join('\n$indentation')}');
return false;
}
}

return true;
}

Expand Down
95 changes: 88 additions & 7 deletions script/tool/test/gradle_check_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ void main() {
String compileSdk = '36',
bool includeKotlinOptions = true,
bool commentKotlinOptions = false,
bool useDeprecatedJvmTarget = false,
bool useDeprecatedJvmTargetStyle = false,
int jvmTargetValue = 17,
int kotlinJvmValue = 17,
}) {
final File buildGradle = package
.platformDirectory(FlutterPlatform.android)
Expand All @@ -74,16 +76,17 @@ java {

''';
final String sourceCompat =
'${commentSourceLanguage ? '// ' : ''}sourceCompatibility = JavaVersion.VERSION_17';
'${commentSourceLanguage ? '// ' : ''}sourceCompatibility = JavaVersion.VERSION_$jvmTargetValue';
final String targetCompat =
'${commentSourceLanguage ? '// ' : ''}targetCompatibility = JavaVersion.VERSION_17';
'${commentSourceLanguage ? '// ' : ''}targetCompatibility = JavaVersion.VERSION_$jvmTargetValue';
final String namespace =
" ${commentNamespace ? '// ' : ''}namespace = '$_defaultFakeNamespace'";
final String jvmTarget =
useDeprecatedJvmTarget ? '17' : 'JavaVersion.VERSION_17.toString()';
final String kotlinJvmTarget = useDeprecatedJvmTargetStyle
? '$jvmTargetValue'
: 'JavaVersion.VERSION_$kotlinJvmValue.toString()';
final String kotlinConfig = '''
${commentKotlinOptions ? '//' : ''}kotlinOptions {
${commentKotlinOptions ? '//' : ''}jvmTarget = $jvmTarget
${commentKotlinOptions ? '//' : ''}jvmTarget = $kotlinJvmTarget
${commentKotlinOptions ? '//' : ''}}''';

buildGradle.writeAsStringSync('''
Expand Down Expand Up @@ -391,6 +394,84 @@ dependencies {
);
});

test('fails when sourceCompatibility/targetCompatibility are below minimum',
() async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir, examples: <String>[]);
writeFakePluginBuildGradle(package,
includeSourceCompat: true,
includeTargetCompat: true,
jvmTargetValue: 11);
writeFakeManifest(package);

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

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains(
'Which is below the minimum required. Use at least "JavaVersion.VERSION_'),
]),
);
});

test('fails when compatibility values do not match kotlinOptions', () async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir, examples: <String>[]);
writeFakePluginBuildGradle(
package,
includeSourceCompat: true,
includeTargetCompat: true,
jvmTargetValue: 21,
// ignore: avoid_redundant_argument_values
kotlinJvmValue: 17,
);
writeFakeManifest(package);

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

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains(
'If build.gradle(.kts) uses JavaVersion.* versions must be the same.'),
]),
);
});

test('passes when jvmValues are higher than minimim', () async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir, examples: <String>[]);
writeFakePluginBuildGradle(
package,
includeSourceCompat: true,
includeTargetCompat: true,
jvmTargetValue: 21,
kotlinJvmValue: 21,
);
writeFakeManifest(package);

final List<String> output =
await runCapturingPrint(runner, <String>['gradle-check']);

expect(
output,
containsAllInOrder(<Matcher>[
contains('Validating android/build.gradle'),
]),
);
});

test('passes when sourceCompatibility and targetCompatibility are specified',
() async {
final RepositoryPackage package =
Expand Down Expand Up @@ -1242,7 +1323,7 @@ dependencies {
writeFakePluginBuildGradle(
package,
includeLanguageVersion: true,
useDeprecatedJvmTarget: true,
useDeprecatedJvmTargetStyle: true,
);
writeFakeManifest(package);

Expand Down