-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[many] Update compileOptions to use Java 17 #10186
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
[many] Update compileOptions to use Java 17 #10186
Conversation
Will get an approval then handle merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for Flutter SVG
There was a problem hiding this 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 updates a large number of packages to use Java 17, which includes updating build.gradle
files, pubspec.yaml
files, and CHANGELOG.md
files. The changes are mostly consistent, but I've found a few areas for improvement.
My review focuses on:
- Ensuring consistency in the
jvmTarget
setting inbuild.gradle
files. - Pointing out inconsistencies and redundancies in the
CHANGELOG.md
files.
Overall, the changes look good and are a welcome update. Addressing the minor issues I've pointed out will improve the consistency and clarity of the codebase.
@@ -1,3 +1,8 @@ | |||
## 2.5.0 | |||
|
|||
* Updates Java compatibility version to 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,3 +1,8 @@ | |||
## 4.11.0 | |||
|
|||
* Updates Java compatibility version to 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
kotlinOptions { | ||
jvmTarget = '11' | ||
jvmTarget = JavaVersion.VERSION_17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
kotlinOptions { | ||
jvmTarget = '11' | ||
jvmTarget = JavaVersion.VERSION_17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
kotlinOptions { | ||
jvmTarget = '11' | ||
jvmTarget = JavaVersion.VERSION_17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## 2.6.0 | ||
|
||
* Updates minimum supported SDK version to Flutter 3.29/Dart 3.7. | ||
* Updates Java compatibility version to 17. If required, Updates minimum supported SDK version to Flutter 3.35/Dart 3.9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for agreeing to let this land first: #10187. Sorry for raining on your parade :0.
|
||
* Updates minimum supported SDK version to Flutter 3.29/Dart 3.7. | ||
* Updates Java compatibility version to 17. | ||
* If required, Updates minimum supported SDK version to Flutter 3.35/Dart 3.9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "If required" is kind of confusing for a client-facing message. If you don't want to manually trim the line out of the packages that don't need it, I would just say "Sets minimum supported SDK version to Flutter 3.35/Dart 3.9."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If required was so that I could have the same message in all changelogs and handled the case where some plugins had already bumped to 3.35.
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22 | ||
|
||
version: 0.10.10+8 | ||
version: 0.10.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.10.10+1? I would expect this to be a bugfix version bump for everything, is that not the case?
@@ -1,3 +1,8 @@ | |||
## 1.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely shouldn't be a minor version bump; it's only changing example and test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is probably more manual tweaking than you want to do, but policywise it would be fine to skip changelog and version updates on the packages where the only change is to example and test code. The tool will think you need to version it because lib/main.dart
is changing, but updating the autoformat version on pub.dev is not actually something that matters.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When making package wide changes I would only do that if the tooling had a way to figure that out for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We expect all changes in this PR to be a no-op for clients thought, right? I would version all of them as a bugfix, not minor, if that's the case; no package switching required.
@@ -1,3 +1,8 @@ | |||
## 1.1.0 | |||
|
|||
* Updates Java compatibility version to 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional since it's manual work, but ideally this line should be removed from every package except the Android plugin implementation packages, since changing the Java version that example app uses is not a package-client-affecting change.
## 1.1.0 | ||
|
||
* Updates Java compatibility version to 17. | ||
* If required, Updates minimum supported SDK version to Flutter 3.35/Dart 3.9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, here's something we could consider: I'd be fine with changing the Java version used in just the example app without changing the min SDK version. Technically this means someone could fail to build an example app that it looks like they should be able to but this would only happen:
- if someone is checking out the repo and building the example app in the first place, which I believe to be quite rare outside of contributors, and
- they are using an old version of Flutter, which is very unlikely for contributors, and
- they are using an old version of Java.
So an option here would be reverting the changes in this PR for all packages where the only gradle change is in example/
, and then doing a second PR for just the examples that only changes the Java version, and has no versioning, no changelog, no min SDK updates (and thus reformatting), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont have a strong opinion on the order these things happen I just want to move to java 17 in the gradle files.
I would really like if I could use the same changelog entry for all packages and if I could ship a new version for all packages but I will do whatever you want.
Based on the comment above and @jesswrd pr that updated most packages for gradle 9 I will start this process over from scratch but invert the order. Start with the examples then try to land a pr that has tooling enforcement and the formatting and version bump changes.
Part 1/2 for flutter/flutter/issues/176027 CHANGELOG exemption: Example apps only. #10186 (comment) Tool test will come in part 2 after the non examples have been updated. ## Pre-Review Checklist
Fixes #flutter/flutter/issues/176027
Reviewers be sure to checkout
script/tool/lib/src/gradle_check_command.dart
which is a tools change.Pre-Review Checklist
[shared_preferences]
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].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].///
).