Skip to content

[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.

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[Many] Migrate non examples (and pigeon test) to java 17 #10201

wants to merge 11 commits into from

Conversation

reidbaker
Copy link
Contributor

Fixes flutter/flutter/issues/176027

  • Update tooling to enforce java 17 compile options and kotlinOptions
  • Update interactive_media_ads and camera_android
  • Update compileOptions to java 17
  • Update changelog and pubspec for modified packages
  • Fix pigeon example

Pre-Review Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 9, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request migrates a number of packages to Java 17. This includes updating build.gradle files to set Java 17 compatibility, updating pubspec.yaml files with new SDK constraints, and adding corresponding changelog entries. The PR also updates the gradle-check tool to enforce these new Java 17 requirements, including a check for kotlinOptions configuration. My review focuses on improving the robustness of the updated tool script.

script/tool/lib/src/gradle_check_command.dart Outdated
Comment on lines 359 to 370
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));

This comment was marked as off-topic.

Sign in to view

script/tool/lib/src/gradle_check_command.dart
Comment on lines +400 to +402
final bool kotlinOptionsUsesJavaVersion = gradleLines.any((String line) =>
line.contains('jvmTarget = JavaVersion.VERSION_$requiredJavaVersion') &&
!_isCommented(line));

This comment was marked as off-topic.

Sign in to view

script/tool/lib/src/gradle_check_command.dart
final bool kotlinOptionsUsesJavaVersion = gradleLines.any((String line) =>
line.contains('jvmTarget = JavaVersion.VERSION_$requiredJavaVersion') &&
!_isCommented(line));
// Either does not set kotlinOptions or does and uses non-string based syntax.

This comment was marked as off-topic.

Sign in to view

bparrishMines
bparrishMines approved these changes Oct 9, 2025
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@reidbaker reidbaker force-pushed the i176027_attempt_4_non_examples branch from 1045ced to 5ec1943 Compare October 9, 2025 19:30
stuartmorgan-g
stuartmorgan-g reviewed Oct 10, 2025
View reviewed changes
script/tool/lib/src/gradle_check_command.dart
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

script/tool/lib/src/gradle_check_command.dart
badLines += '${gradleLines[i]}\n';
}
final String kotlinErrorMessage = '''
If build.gradle(.kts) sets jvmTarget then it must use JavaVersion syntax.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, if I'm reading this right, setting, say jvmTarget = JavaVersion.VERSION_21.toString() would lead to an error message saying that they are using the wrong syntax, when the actual problem in that the version is wrong.

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.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Oct 10, 2025

autosubmit label was removed for flutter/packages/10201, because - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@reidbaker reidbaker mentioned this pull request Oct 10, 2025
Open
11 tasks
@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Oct 10, 2025

autosubmit label was removed for flutter/packages/10201, because - The status or check suite Linux_android android_platform_tests_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Oct 10, 2025

autosubmit label was removed for flutter/packages/10201, because - The status or check suite Linux_android android_platform_tests_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g left review comments

@bparrishMines bparrishMines bparrishMines approved these changes

@tarrinneal tarrinneal tarrinneal approved these changes

@ash2moon ash2moon Awaiting requested review from ash2moon ash2moon is a code owner

@camsim99 camsim99 Awaiting requested review from camsim99 camsim99 is a code owner

@mboetger mboetger Awaiting requested review from mboetger mboetger is a code owner

@gmackall gmackall Awaiting requested review from gmackall gmackall is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p: camera p: flutter_plugin_android_lifecycle p: google_maps_flutter p: google_sign_in p: image_picker p: in_app_purchase p: interactive_media_ads Plugin for IMA SDK p: local_auth p: path_provider p: pigeon p: video_player platform-android platform-ios

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate flutter owned packages to use jvm version 17 (away from jvm 11)

4 participants

@reidbaker @stuartmorgan-g @bparrishMines @tarrinneal