-
Notifications
You must be signed in to change notification settings - Fork 28.5k
Enforce minSdk constraint for Android Flutter #164251
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
Conversation
autosubmit label was removed for flutter/flutter/164251, because - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/test/kotlin/DependencyVersionCheckerTest.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/test/kotlin/DependencyVersionCheckerTest.kt
Outdated
Show resolved
Hide resolved
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'd recommend also adding the code path for the error version in this pr, but just setting the actual error version to 1 for now (similar to what is done for Java)
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
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 w/ tiny nits
packages/flutter_tools/gradle/src/test/kotlin/DependencyVersionCheckerTest.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/test/kotlin/DependencyVersionCheckerTest.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/test/kotlin/DependencyVersionCheckerTest.kt
Outdated
Show resolved
Hide resolved
autosubmit label was removed for flutter/flutter/164251, because This PR has not met approval requirements for merging. Changes were requested by {reidbaker}, please make the needed changes and resubmit this PR.
|
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Show resolved
Hide resolved
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
val androidComponents = | ||
project.extensions.findByType(AndroidComponentsExtension::class.java) |
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.
Do you also need to check for com.android.build.gradle.LibraryExtension to support non apps like aar's?
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'm not sure if this would even be possible to warn via the way we are applying this gradle plugin. Since the gradle plugin is only added for flutter applications and not flutter plugins, this code would never run, therefore making any checks on LibraryExtension to not run.
I feel like adding support for plugins would require deeper integration work and would be out of the scope for this PR. WDYT?
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 was more thinking flutter modules that get built from either source like a locally developed plugin or a flutter module assembled as an aar. I am pretty confident that flutter tooling is run in that case (e.g. flutter build aar
).
I am ok if you do library work in another pr but I think it is in scope for this issue.
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.
Looks like for modules/plugins, the "dev.flutter.flutter-gradle-plugin" is not applied, so none of our dependency checking code gets ran. Once that plugin is added to the projects, I believe this code should automatically start checking for those dependencies since AndroidComponentsExtension is a supertype of ApplicationAndroidComponentsExtension and LibraryAndroidComponentsExtension
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.
Flutter tooling is run, but we don't apply the flutter gradle plugin to either modules or plugins, so I don't think we will be able to handle those cases in the DependencyVersionChecker
.
But also, AndroidComponentsExtension
is a supertype of ApplicationAndroidComponentsExtension
and LibraryAndroidComponentsExtension
, so I think this would work if we did eventually apply to a Library
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.
ok then with grays comment it is not in scope.
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Outdated
Show resolved
Hide resolved
…avor in min sdk checker
…avor in min sdk checker
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.
Approved % comments.
packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt
Show resolved
Hide resolved
@@ -105,6 +131,35 @@ object DependencyVersionChecker { | |||
// KGP is not required, so don't log any warning if we can't find the version. | |||
} | |||
|
|||
private fun checkMinSdkForAndroidComponents(project: Project) { |
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.
Naming nit: I think this should be called something like configureMinSdkCheck
since that is what this method does and the actual check is in checkMinSdkVersion
which feels appropriately named.
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.
Done
val taskName = "${it.name.capitalize()}$MIN_SDK_CHECK_TASK_POSTFIX" | ||
val minSdkCheckTask = | ||
project.tasks.register(taskName) { | ||
doLast { |
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.
Does this need to be in a doLast block? there is no prework and we are creating the task so I would think it could run in the "normal order" part of the task.
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.
In this case, it wouldn't really matter if we call doFirst or doLast since there is no other code being executed, but I think typically I've seen doLast more often than doFirst. If you mean by "normal order" as in without the doLast
or doFirst
, we would end up adding code in the configuration stage of the action which is not something we would want to do. Alternatively we can also just do actions.add { ... }
but I think it would be more idiomatic to just do doLast { ... }
. WDYT?
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.
No strong opinion but we should probably have a strong opinion. I am leaning towards having it be part of the regular actions based on this documentation of task outputs.
Then we can save doFirst
for setup and doLast
for clean up. I think you are right that our current code does use doLast the most.
@gmackall fyi.
|
||
@VisibleForTesting internal fun checkMinSdkVersion( | ||
minSdkVersion: MinSdkVersion, | ||
project: Project |
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.
Scope nit: consider passing the rootDir path, and the logger then returning if OUT_OF_SUPPORT_RANGE_PROPERTY should be set.
Project as you can see is a big object that has a lot of scope and we can minimize that scope in this method.
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.
Done
@@ -209,6 +220,98 @@ class DependencyVersionCheckerTest { | |||
} | |||
verify(exactly = 0) { mockExtraPropertiesExtension.set(OUT_OF_SUPPORT_RANGE_PROPERTY, true) } | |||
} | |||
|
|||
@Test | |||
fun `min SDK version in warn range results in warning logs`() { |
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 like the 2 roughly end to end tests you have added here. Consider adding unit tests for some of the extracted logic you made @VisibleForTesting that are smaller in scope and require less expansive mocking (or no mocking).
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 added some more tests for checkMinSdkVersion since that's the only other non-private method added.
- refactored checkMinSdkVersion - added more test cases for min sdk check
flutter/flutter@2578d97...321fbc0 2025-03-06 [email protected] Roll Skia from fefecd49e03a to ccd8cc23aa94 (1 revision) (flutter/flutter#164712) 2025-03-06 [email protected] [web] Detect scrollable semantics nodes more reliably (flutter/flutter#164491) 2025-03-06 [email protected] [windows] wire the focus request and the focus events through the Windows platform (flutter/flutter#164296) 2025-03-06 [email protected] Roll Skia from 02897747c7d5 to fefecd49e03a (1 revision) (flutter/flutter#164701) 2025-03-06 [email protected] Roll Skia from e315b0ab7c84 to 02897747c7d5 (1 revision) (flutter/flutter#164677) 2025-03-06 [email protected] Roll Skia from 0c3880f94970 to e315b0ab7c84 (1 revision) (flutter/flutter#164669) 2025-03-06 [email protected] [Impeller] use device private on non-iOS devices. (flutter/flutter#164601) 2025-03-05 [email protected] Roll Skia from 43294a662fd0 to 0c3880f94970 (1 revision) (flutter/flutter#164661) 2025-03-05 [email protected] Add a workflow (only triggered from rest events) for hasing experiment (flutter/flutter#164657) 2025-03-05 [email protected] Roll Skia from 4cf9f0b77d41 to 43294a662fd0 (4 revisions) (flutter/flutter#164649) 2025-03-05 [email protected] Adds animateToItem to the CarouselController (flutter/flutter#162694) 2025-03-05 [email protected] Cleanup content context (flutter/flutter#164229) 2025-03-05 [email protected] Fix: Update CupertinoSheetRoute transition rounded corner (flutter/flutter#163700) 2025-03-05 [email protected] [Impeller] fix macOS managed memory. (flutter/flutter#164635) 2025-03-05 [email protected] [skwasm] Clear font collection cache when font is loaded manually. (flutter/flutter#164588) 2025-03-05 [email protected] Fix race condition causing crash when interacting with an animating scrollable (flutter/flutter#164392) 2025-03-05 [email protected] Use dwds 24.3.6 and pass uri for the reload scripts path to FrontendServerDdcLibraryBundleProvider (flutter/flutter#164582) 2025-03-05 [email protected] Roll Packages from 9e4684e to abba683 (8 revisions) (flutter/flutter#164630) 2025-03-05 [email protected] Roll Skia from 7e4323f72c9d to 4cf9f0b77d41 (1 revision) (flutter/flutter#164622) 2025-03-05 [email protected] Fix to Linux_pixel_7pro integration_ui_keyboard_resize test flakiness (flutter/flutter#162308) 2025-03-05 [email protected] Roll Skia from 03a3f653d64e to 7e4323f72c9d (1 revision) (flutter/flutter#164599) 2025-03-05 [email protected] Implement `clipPath` Mutator for hcpp (flutter/flutter#164525) 2025-03-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] use DeviceLocal textures for gifs on non-iOS devices. (#164573)" (flutter/flutter#164600) 2025-03-05 [email protected] [macos] prefer integrated GPU. (flutter/flutter#164569) 2025-03-05 [email protected] Enforce minSdk constraint for Android Flutter (flutter/flutter#164251) 2025-03-05 [email protected] Add `clipRSuperellipse`, and use them for dialogs (flutter/flutter#161111) 2025-03-05 [email protected] Roll Skia from 46705a22edc3 to 03a3f653d64e (1 revision) (flutter/flutter#164590) 2025-03-05 [email protected] when resetting FlutterPlatformViewsController, clear out some additional internal state to prevent it from carrying over across a Hot Restart (flutter/flutter#164456) 2025-03-05 [email protected] Roll Fuchsia Linux SDK from Rt6pxGFLVAJHduM0V... to fhm5z889sA5T1AQao... (flutter/flutter#164583) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This commit adds validation for the Android SDK version within the existing dependency version checker. It introduces a warning when the SDK version is below a predefined threshold. The validation checks for the
minSdk
property in theandroid
block of the app'sbuild.gradle
file. It will warn users if theirminSdk
version is out of Flutter's supportedrange and guide them on how to fix it.
Fixes #134570
Pre-launch Checklist
///
).