-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[espresso] updates build files to use JVM 17 #10162
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
base: main
Are you sure you want to change the base?
Conversation
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
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 the espresso
package to use Java 17. This is achieved by updating the sourceCompatibility
and targetCompatibility
in android/build.gradle
, and increasing the minimum required Flutter version to 3.35.0
and Dart SDK to ^3.9.0
in pubspec.yaml
. The CHANGELOG.md
and pubspec.yaml
are updated to version 0.4.0+15
.
While the changes are functionally correct for the package itself, I have a couple of points for consideration. According to the pub versioning philosophy, increasing the minimum Flutter version is a breaking change, which for a pre-1.0.0 package suggests a minor version bump to 0.5.0
rather than a build number increment. I've left specific comments on this in the relevant files. Additionally, the pubspec.yaml
for the example app has not been updated with the new SDK constraints, which may cause it to fail and should be updated to match the package's new requirements.
@@ -1,3 +1,8 @@ | |||
## 0.4.0+15 |
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.
According to the pub versioning philosophy, increasing the minimum Flutter SDK version is a breaking change.1 For a package with a version less than 1.0.0, this should be a minor version bump. Please update the version to 0.5.0
.
## 0.4.0+15 | |
## 0.5.0 |
Style Guide References
Footnotes
-
The style guide states that bumping the minimum Flutter version is a breaking change, which requires a major version increment (or minor for pre-1.0.0 packages). ↩
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.
According to the pub versioning philosophy, increasing the minimum Flutter SDK version is a breaking change.
[...]
The style guide states that bumping the minimum Flutter version is a breaking change
Sure, Jan.
repository: https://github.com/flutter/packages/tree/main/packages/espresso | ||
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+espresso%22 | ||
version: 0.4.0+14 | ||
version: 0.4.0+15 |
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.
Bumping the minimum required Flutter version is a breaking change according to the pub versioning philosophy.1 For a pre-1.0.0 package, this should be reflected as a minor version bump. Please consider changing the version to 0.5.0
.
version: 0.5.0
Style Guide References
Footnotes
-
The style guide considers bumping the minimum Flutter version a breaking change. For pre-1.0.0 packages, this should result in a minor version bump (e.g., 0.4.x to 0.5.0). ↩
Holding until we see more users using the existing migrations https://pub.dev/packages/file_selector_android/score |
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 whenever you're ready to land it.
Update minimum flutter version to 3.35 to force a minimum AGP version to something that requires java 17.
part of flutter/flutter#176027
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].///
).