Repository navigation
[AGP 9.1.0 Migration #2] Extract internal utilities - #190957
Conversation
…page draft) Phase P0 of the Flutter Gradle Plugin migration to the AGP public API surface (flutter#180137, flutter#166550): the contributor migration record (replacement map, decision records, phase map, revert-window table) and the draft of the user-facing breaking-change page to be published to docs.flutter.dev before the newDsl flip reaches beta. Revert-safe: always.
…k/ndk reads to the public DSL Phase P1 of the AGP public-API migration (flutter#180137, flutter#166550): - VersionFetcher no longer calls the internal com.android.build.gradle.internal.utils.getKotlinAndroidPluginVersion. getKGPVersion now relies on the existing public fallback chain (kotlin_version property -> KotlinAndroidPluginWrapper.pluginVersion -> reflection) and documents that null is the expected result when KGP is absent (e.g. under AGP built-in Kotlin). DependencyVersionChecker already treats null as "KGP not required". - AgpCommonExtensionWrapper gains compileSdkPreview. - getCompileSdkFromProject now reads compileSdk/compileSdkPreview from the new DSL via the wrapper and returns a structured CompileSdkVersion instead of parsing the legacy "android-NN" string. The PluginHandler compileSdk mismatch warning uses CompileSdkVersion.isHigherThan, which defines the preview semantics the old string comparison got wrong (resolves the PluginHandler TODO): preview > numeric, distinct preview codenames incomparable (no warning), numeric compared numerically. Warning message text is unchanged. - getConfiguredNdkVersion reads through the wrapper instead of the legacy BaseExtension fallback. - Deletes the setAgpKotlinVersionToNull test helper (existed only to stub the internal AGP call) and updates unit tests accordingly; adds tests for both preview-vs-numeric directions and the codename reset case. Verification note: the FGP unit test suite could not be executed in this sandbox (the network policy blocks dl.google.com, so AGP artifacts do not resolve); run 'gradle test' in packages/flutter_tools/gradle in CI. Revert-safe until P2 lands.
… isHigherThan logic
ef0ab7b to
12de6b5
Compare
… and restore 'unknown' KGP version check
There was a problem hiding this comment.
Code Review
This pull request refactors the retrieval and comparison of the Android compile SDK version by introducing a structured CompileSdkVersion class, which properly handles numeric versions and preview codenames like 'Baklava'. It also simplifies Kotlin Gradle Plugin version fetching when KGP is absent and updates corresponding tests. The review feedback correctly identifies a duplicated mock configuration line in FlutterPluginUtilsTest.kt that should be removed.
Sorry there was a missing import that caused the tests to fail. I had a round of passing tests, gave some feedback then had to head for a conference. The change to make the tests pass should hopefully change nothing about the structure of the code that needs review. That said I will make sure I have green tests before asking for a second review on this pr. |
gmackall
left a comment
There was a problem hiding this comment.
lgtm w/ one comment about version comparison
| * Whether this compile SDK is known to be higher than [other]. | ||
| * | ||
| * - numeric vs numeric: numeric comparison. | ||
| * - preview vs numeric: a preview codename targets an unreleased SDK, so it is |
There was a problem hiding this comment.
I'm not sure this is necessarily true, you can have a preview version on your machine that was a preview of, say, 36, if you haven't deleted it since you used it. This would then be less than 37.
But I'm not sure it really matters for the way we use the value.
There was a problem hiding this comment.
hmm I agree it may not matter for how we currently do it. What if we said that unknown preview releases were considered newer than any api number then has a list of preview releases and the numbers they were attached to.
There was a problem hiding this comment.
We could do that, but I'd probably prefer not to be responsible for constantly keeping it up to date.
In practice we are using this for comparing host app to plugin compile sdks right, and printing a warning if your app is lower than the plugins. I think the current implementation is good enough for that. The main use case here is really just putting a preview compile sdk as your app's compile sdk to validate behavior in advance of the new version, and as long as we aren't blocking the build I think we are good (and in particular, we would only be wrong here if
- a plugin is publishing using a preview compile sdk, which I don't think we really care to support beyond not crashing - preview compile sdks should be for validation, not publishing
- a host app is using a preview compile sdk from a previous-generation api level, which again I don't think we should care to support beyond not crashing)
…dependencies through the new DSL (flutter#191218) This is PR 3 of 11 in the AGP 9.1.0 / public `gradle-api`/ newdsl migration stack. * It migrates many (but not all) usages of getLegacyAndroidExtension. * Introduces consistency in renaming of imports. * Adds a test to ensure we are not adding internal apis (thanks @mboetger from pr1) * builds the ability to run our gradle tests with multiple AGP versions (see packages/flutter_tools/gradle/build.gradle.kts) Between PR 3 and PR 8, an Add-to-app host app embedding a Flutter module with a custom build type (for example "staging") gets release engine artifacts. That means hot reload, debugger attach, and DevTools do not operate in that build. We can't move the work in PR 8 up but I would not want to cut a release between this pr and 8 landing. If we keep reviewing one pr a day then that is not a risk. Apps impacted by this change can use matchingFallbacks to avoid this problem (see code below). In pr 8 we change when we look for "isDebuggable" to much later in the gradle lifeycle when all the variants have been created which then lets us use a new api to understand if the variant is intended to be debuggable. Kotlin ```kotlin // host app build.gradle.kts android { buildTypes { create("staging") { isDebuggable = true applicationIdSuffix = ".staging" matchingFallbacks += "debug" /// This line. } } } ``` Groovy ```groovy // host app build.gradle android { buildTypes { staging { debuggable true applicationIdSuffix ".staging" matchingFallbacks = ['debug'] /// This line. } } } ``` Depends on flutter#190957 (PR 2). - @reidbaker --- Standard review context for this pr stack This is PR is part of an 11 pr stack to migrate the "newdsl" `gradle-api` specifically in agp 9.1.0. The complete stack has passed presubmits, postsubmits, and customer tests: https://flutter-dashboard.appspot.com/#/build?repo=flutter&branch=experimental/agp-gradle-api All of the code was LLM authored. A mix of manual prompting, automatic prompting, several models and adversarial review. The combined sessions are enough that I cannot include relevant prompts like I have been doing on other prs. If you want to review the pr stack you can find it here. These prs will be abandoned/closed as prs land into flutter/flutter. 1. reidbaker-agent#1 (branch: agp-api-doc) 2. reidbaker-agent#2 (branch: agp-internal-utils) 3. reidbaker-agent#3 (branch: agp-buildmode-deps) 4. reidbaker-agent#4 (branch: agp-plugin-buildtypes) 5. reidbaker-agent#5 (branch: agp-ndk-fallback) 6. reidbaker-agent#6 (branch: agp-assets-onvariants) 7. reidbaker-agent#7 (branch: agp-apk-copy-versioncode) 8. reidbaker-agent#8 (branch: agp-add-to-app) 9. reidbaker-agent#9 (branch: agp-aar-script) 10. reidbaker-agent#10 (branch: agp-newdsl-flip) 11. reidbaker-agent#11 (branch: agp-gradle-api) This work is urgent in the sense that we are worried that android will publish agp 10 with no opt out but not so urgent that we are willing to break flutter users because we didn't review or understand the code because we were in a rush. Breaking changes are expected as part of this work. There are patterns the android team explicitly does not want apps to use and apis that have no equivalent. As part of the effort to ensure this work does not slip into ai slop, prs from this stack will be reviewed by me (@reidbaker) before asking for review. Then we will have 2 android expert reviewers also review every pr. --- Agent authored description. This is PR 3 of 11 in the AGP 9.1.0 / public `gradle-api` migration stack (flutter#180137, flutter#166550). ### Key Changes - **DSL Build Mode Overloads**: Adds `buildModeFor` overloads accepting `ApplicationBuildType`, `DynamicFeatureBuildType`, and `LibraryBuildType` DSL types alongside the `(buildTypeName, isDebuggable)` core overload. - **Robust CompileSdkVersion Domain Model**: Extracts `CompileSdkVersion` with constructor invariant validation (`init { require(...) }`) enforcing mutual exclusivity between `apiLevel` and `previewCodename`. - **Namespaced Multi-AGP Build Property**: Namespaces the AGP version property in `packages/flutter_tools/gradle/build.gradle.kts` as `flutter.internal.agpVersion` (defaulting to `8.11.1`) to prevent user app properties from leaking into the included build during app compilation. - **Standardized Type Aliasing**: Applies non-temporal type aliasing (`import com.android.build.api.dsl.BuildType as DslBuildType`) across `FlutterPlugin.kt` and `PluginHandler.kt`. - **Public DSL Extension Access**: Updates `addFlutterDependencies` and `PluginHandler` to iterate `AgpCommonExtensionWrapper.buildTypes`. - **Build & Bytecode Validation**: Adds the `:validateNoCommonExtensionInBytecode` task in `build.gradle.kts` to prevent compiled main classes from referencing binary-incompatible `CommonExtension`, and adds `BytecodeValidatorTest.kt` verifying the binary pattern-matching and class scanning logic. - **Comprehensive Unit Tests**: Adds full 4-way dispatch test coverage in `AgpCommonExtensionWrapperTest`, dependency wiring tests in `FlutterPluginTest`, and decomposes `PluginHandlerTest` mock fixtures into focused helpers with positive assertions. ### Add-to-App & DSL Scope Context `LibraryBuildType` does not expose `isDebuggable` at DSL scope. In PR 3, custom build types on library projects (e.g. host app 'staging') fall back to `"release"` at DSL scope. In PR 8 ("Unify add-to-app module wiring on the variant API"), module wiring will be unified on `Component.debuggable` at variant scope. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: reidbaker-agent <reidbaker@google.com> Co-authored-by: Reid Baker <1063596+reidbaker@users.noreply.github.com>
…91281) ## Description This closes out flutter#191081. The bogus "compiles against Android SDK 2147483647" warning was caused by `getCompileSdkFromProject` parsing the deprecated `compileSdkVersion` platform-hash string (`"android-36-ext19".substring(8).toIntOrNull() ?: Int.MAX_VALUE`). That codepath no longer exists: flutter#190957 (part of the AGP 9.1.0 migration stack) already rewrote `getCompileSdkFromProject` to read AGP's typed `compileSdk: Int?` / `compileSdkPreview: String?` DSL properties directly. Since `compileSdk` never folds in the extension suffix, the failure mode in flutter#191081 is now structurally impossible — no plugin's `compileSdkExtension` can leak into this comparison. What was missing was test coverage for exactly this scenario (a plugin with `compileSdkExtension` set, as required by AARs with a `minCompileSdkExtension` metadata constraint — e.g. `androidx.health.connect:connect-client`). This PR adds that regression test to `FlutterPluginUtilsTest.kt`, confirming `compileSdkExtension` plays no part in the resulting `CompileSdkVersion` or in `isHigherThan` comparisons, so a future refactor can't reintroduce the bug. No production code changes — this is test-only. ## Pre-launch Checklist - [x] I read the [Contributor Guide](https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md) and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene](https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md) wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide](https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md). - [x] I signed the [CLA](https://cla.developers.google.com/). - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making. Fixes flutter#191081 Co-authored-by: Devasy Patel <110348311+Devasy23@users.noreply.github.com> Co-authored-by: Reid Baker <1063596+reidbaker@users.noreply.github.com> Co-authored-by: Camille Simon <43054281+camsim99@users.noreply.github.com>
This is PR 2 of 11 in the AGP 9.1.0 / public
gradle-api/ newdls migration stack.This PR extracts some common utilities used in the Flutter Gradle Plugin to internal functions, to be used in upcoming PRs in this stack. It also introduces a typesafe CompileSdkVersion that handles comparisons between api versions and preview versions which are strings.
First attempt was here #190949 this pr includes my feedback from that first review. The first attempt did not follow the pattern of having the agent account open the pr because of rebase shenanigans that ended up touching freeze.yml.
Standard review context for this pr stack
This is PR is part of an 11 pr stack to migrate the "newdsl"
gradle-apispecifically in agp 9.1.0.The complete stack has pass pre submits, post submits and customer tests. https://flutter-dashboard.appspot.com/#/build?repo=flutter&branch=experimental/agp-gradle-api
All of the code was LLM authored. A mix of manual prompting, automatic prompting, several models and adversarial review. The combined sessions are enough that I cannot include relevant prompts like I have been doing on other prs.
If you want to review the pr stack you can find it here. These prs will be abandoned/closed as prs land into flutter/flutter.
This work is urgent in the sense that we are worried that android will publish agp 10 with no opt out but not so urgent that we are willing to break flutter users because we didn't review or understand the code because we were in a rush.
Breaking changes are expected as part of this work. There are patterns the android team explicitly does not want apps to use and apis that have no equivalent.
As part of the effort to ensure this work does not slip into ai slop, prs from this stack will be reviewed by me (@reidbaker) before asking for review. Then we will have 2 android expert reviewers also review the every pr.
Agent authored description.
This is PR 2 of 11 in the AGP 9.1.0 / public
gradle-apimigration stack.This PR extracts some common utilities used in the Flutter Gradle Plugin to internal functions, to be used in upcoming PRs in this stack.
Pre-launch Checklist
///).