-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[Many] Migrate non examples (and pigeon test) to java 17 #10201
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.
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
Changes from 1 commit
071f1d5
d2069f0
50b804a
164f7b2
fd45002
44c074e
5ec1943
a366555
33dbb85
cbb7daf
927d508
80b4dc7
a417381
a0c885a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:math' as math; | ||
|
|
||
| import 'package:collection/collection.dart'; | ||
| import 'package:file/file.dart'; | ||
| import 'package:meta/meta.dart'; | ||
|
|
@@ -354,33 +356,34 @@ 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'; | ||
| final bool hasLanguageVersion = gradleLines.any((String line) => | ||
| line.contains('languageVersion') && !_isCommented(line)); | ||
| final bool hasCompabilityVersions = gradleLines.any((String line) => | ||
| line.contains('sourceCompatibility') && !_isCommented(line)) && | ||
| line.contains('sourceCompatibility = JavaVersion.VERSION_$requiredJavaVersion') && !_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') && !_isCommented(line)); | ||
| line.contains('targetCompatibility = JavaVersion.VERSION_$requiredJavaVersion') && !_isCommented(line)); | ||
| if (!hasLanguageVersion && !hasCompabilityVersions) { | ||
| const String errorMessage = ''' | ||
| build.gradle must set an explicit Java compatibility version. | ||
| const String javaErrorMessage = ''' | ||
| build.gradle(.kts) must set an explicit Java compatibility version. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should say "must set an explicit Java compatibility version of $requiredJavaVersion." With the updated check, someone who adds an incorrect compatibility version will get an error message that just says that they need to set an explicit version, which they did, so it will be confusing. |
||
|
|
||
| This can be done either via "sourceCompatibility"/"targetCompatibility": | ||
| android { | ||
| compileOptions { | ||
| sourceCompatibility = JavaVersion.VERSION_11 | ||
| targetCompatibility = JavaVersion.VERSION_11 | ||
| sourceCompatibility = JavaVersion.VERSION_$requiredJavaVersion | ||
| targetCompatibility = JavaVersion.VERSION_$requiredJavaVersion | ||
| } | ||
| } | ||
|
|
||
| or "toolchain": | ||
| java { | ||
| toolchain { | ||
| languageVersion = JavaLanguageVersion.of(11) | ||
| languageVersion = JavaLanguageVersion.of($requiredJavaVersion) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -389,9 +392,38 @@ https://docs.gradle.org/current/userguide/java_plugin.html#toolchain_and_compati | |
| for more details.'''; | ||
|
|
||
| printError( | ||
| '$indentation${errorMessage.split('\n').join('\n$indentation')}'); | ||
| '$indentation${javaErrorMessage.split('\n').join('\n$indentation')}'); | ||
| return false; | ||
| } | ||
| bool isKotlinOptions(String line) => line.contains('kotlinOptions') && !_isCommented(line); | ||
| final bool hasKotlinOptions = gradleLines.any(isKotlinOptions); | ||
| final bool kotlinOptionsUsesJavaVersion = gradleLines.any((String line) => | ||
| line.contains('jvmTarget = JavaVersion.VERSION_$requiredJavaVersion') && | ||
| !_isCommented(line)); | ||
|
Comment on lines
+405
to
+407
This comment was marked as off-topic.
Sorry, something went wrong. |
||
| // Either does not set kotlinOptions or does and uses non-string based syntax. | ||
This comment was marked as off-topic.
Sorry, something went wrong. |
||
| if (hasKotlinOptions && !kotlinOptionsUsesJavaVersion) { | ||
| // Bad lines contains the first 4 lines including the kotlinOptions section. | ||
| String badLines = ''; | ||
| final int startIndex = gradleLines.indexOf(gradleLines.firstWhere(isKotlinOptions)); | ||
| for(int i = startIndex; i < math.min(startIndex + 4, gradleLines.length); i++) { | ||
| badLines += '${gradleLines[i]}\n'; | ||
| } | ||
| final String kotlinErrorMessage = ''' | ||
| If build.gradle(.kts) sets jvmTarget then it must use JavaVersion syntax. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, if I'm reading this right, setting, say I think we should either detect those cases separately and have different messages, or just make the message more specific (e.g., "it must set version $requiredJavaVersion using JavaVersion syntax.")
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stuartmorgan-g and I talked offline and I will make this change in my next pr. Tooling is easier to land and less likely to have a conflict and this corner case is unlikely to hit anyone in the short term. |
||
| Good: | ||
| android { | ||
| kotlinOptions { | ||
| jvmTarget = JavaVersion.VERSION_$requiredJavaVersion.toString() | ||
| } | ||
| } | ||
| BAD: | ||
| $badLines | ||
| '''; | ||
| printError( | ||
| '$indentation${kotlinErrorMessage.split('\n').join('\n$indentation')}'); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
|
|
||
This comment was marked as off-topic.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.