[AGP 9] Template Bump to AGP 9#185730
Conversation
github-actions
Bot
added
platform-android
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Android build configuration by upgrading the Gradle version to 9.1.0 and the Android Gradle Plugin (AGP) to 9.0.0. It includes updates to project lockfiles, build templates, and the Flutter tools' Gradle logic to support these newer versions. Several review comments highlight potential issues with the current configuration of android.builtInKotlin and android.newDsl flags in gradle.properties, as well as the removal of the kotlin-android plugin in templates, which may lead to build failures. Additionally, a suggestion was made to improve the regex patterns used for detecting Kotlin plugin declarations in project.dart.
|
|
||
| androidComponentsExtension.defaultConfig.manifestPlaceholders["applicationName"] = | ||
| baseApplicationName | ||
| androidComponentsExtension.defaultConfig { |
There was a problem hiding this comment.
Can you explain this change? I don't understand why a bump to our template values would necessitate this
There was a problem hiding this comment.
I see you covered this in the pr body:
previously we were using the internal AGP defaultConfig() method. Starting AGP 9, internal methods can no longer be accessed.
But we would have been calling this method on apps using AGP 9 already right?
- Was it completely broken?
- If so, again this doesn't particularly pertain to templates does it? Isn't this core functionality for support of AGP 9 independent of templates? I think this is another example of a logically independent piece that could be it's own pr
There was a problem hiding this comment.
I think we don't have any apps already on AGP 9, and if they are, they all have the flag android.newDsl=false which allows for use of internal AGP methods. If apps on AGP 9 do not have that flag, they will fail.
I made this change because integration tests (where app creation is involved) and unit tests (where app creation was not involved I think) were failing because of these defaultConfig() calls. To address the failing unit tests where app creation was not involved, I think I would have to add a gradle.properties file and android.newDsl=false to the Flutter Gradle Plugin itself. Let me know if you still want this change in a different PR.
There was a problem hiding this comment.
Again, though, if this is happening where app creation is not involved isn't this unrelated to the template change? If so we should favor landing it independently. Especially where cherry picks are involved, we should try to scope PRs narrowly to solve individual problems
There was a problem hiding this comment.
It's happening both when app creation is involved and when it is not involved. I believe adding android.newDsl=false to the template addresses the issue on app creation.
Regardless, I'm going to try to split this into a different PR for the CP.
There was a problem hiding this comment.
Hmm but if it's fixed by android.newDsl=false, shouldn't our migrator prevent this problem? Or is it somehow hitting a workflow that isn't covered by the migrator (maybe add to app?)
There was a problem hiding this comment.
I think that failure comes from integration tests (the ones with long workflows that setup AAR, Flutter modules for add-to-app) that first run gradle build commands then run flutter build and flutter build is what invokes the migrator. So the test workflow fails on that first gradle build command. This seems like a fringe case (maybe less fringe for setting up add-to-app) but if a Flutter developer runs gradle build before running Flutter build, then they will run into failures too. So there is a case to add the flags to the templates.
I think the add-to-app integration tests clone/reuse one host app. I manually added the neccessary flags in the gradle.properties of the host app. Manually adding the flags is necessary to pass the "building thte host app" section of an add-to-app test workflow.
Additionally, the migrator doesn't cover adding android.newDsl=false to FGP tool for those non app cases.
| extension.defaultConfig.ndk { | ||
| abiFilters.clear() | ||
| abiFilters.addAll(PLATFORM_ABI_LIST) | ||
| extension.defaultConfig { |
There was a problem hiding this comment.
Same here, why does a template bump necessitate this?
There was a problem hiding this comment.
| */ | ||
| internal const val BUILT_IN_KOTLIN_DOCS_TO_REPORT_UNMIGRATED_PLUGINS = | ||
| "https://docs.flutter.dev/release/breaking-changes/migrate-to-built-in-kotlin/for-app-developers/report-incompatible-kotlin-gradle-plugin-usage-to-plugin-authors" | ||
| "https://docs.flutter.dev/release/breaking-changes/migrate-to-built-in-kotlin/for-app-developers#report-incompatible-kotlin-gradle-plugin-usage-to-plugin-authors" |
There was a problem hiding this comment.
If we encounter difficulties cherry picking this overall pr, consider making this change independently
In general, it's probably good to make small changes like this independently in general. Favor more small prs over one big pr as the review is faster and also the git history is easier to understand
| // Supported here means supported by the tooling for | ||
| // flutter analyze --suggestions and does not imply broader flutter support. | ||
| const maxKnownAgpVersionWithFullKotlinSupport = '8.13.0'; | ||
| const maxKnownAgpVersionWithFullKotlinSupport = '9.0.0'; |
There was a problem hiding this comment.
Does this mean versions that support using not-built-in-kotlin? If so does 9.0.1? If so
| const maxKnownAgpVersionWithFullKotlinSupport = '9.0.0'; | |
| const maxKnownAgpVersionWithFullKotlinSupport = '9.0.1'; |
There was a problem hiding this comment.
Done. Yes, 9.0.1 still supports not-built-in-kotlin because the legacy KGP is the default for AGP 9+ on Flutter.
| multiLine: true, | ||
| ); | ||
|
|
||
| static final _kotlinCompilerOptionsPattern = RegExp( |
There was a problem hiding this comment.
Updates to these regex patterns seem outside the scope of a template bump. Is there a reason this needs to be done in this PR?
There was a problem hiding this comment.
Ah existing unit tests were failing because my change removed usage of the helper firstMatchInFile which read values line by line. Removing that made it necessary to add multiLine:true to the regexes.
I'm going to revert the regex to their original form and use firstMatchInFile when matching against original regexes.
| }); | ||
| return imperativeMatch || declarativeMatch; | ||
| if (appGradleFile.existsSync()) { | ||
| try { |
| .childDirectory('app') | ||
| .childFile('build.gradle.kts'); | ||
| // write a build.gradle.kts with compileSdkVersion as `android-UpsideDownCake` which is a string preview version | ||
| // write a build.gradle.kts with compileSdkVersion as `android-<api-version>` which is a string preview version |
There was a problem hiding this comment.
Hmm is it though? I think this breaks the test as it isn't a string preview.
Also, when changing this you should be aware that this test is hosted on its own ci yaml target which has UpsideDownCake pre installed. These preview versions, referred to by string, exist for some time but then are deleted. For example the UDC one we are using has been deleted. We worked around this by pre-installing it on the machine through cipd (otherwise this test would have broken out of band by now).
Anyways, I think this isn't a valid replacement. Do we need to replace this? We are still using the deprecated compileSdkVersion. Is it because of a min sdk restriction with AGP 9 or something?
There was a problem hiding this comment.
I think aaa and integration_test packages require compileSdk 36, so we do need to update to a higher compileSdk version because this is the error I see (from the closed PR): https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20android_preview_tool_integration_tests/33226/overview
It looks like compileSdk = is only valid for integers here so preview versions can't be set. However, preview versions can be set like so here:
android {
compileSdk {
version = preview("Baklava")
}
}
I replaced the test with this.
| .childDirectory('app') | ||
| .childFile('build.gradle.kts'); | ||
| // write a build.gradle.kts with compileSdkPreview as `UpsideDownCake` which is a string preview version | ||
| // write a build.gradle.kts with compileSdkPreview as `Baklava` which is a string preview version |
There was a problem hiding this comment.
Baklava will presumably be deleted, so if we want to make this replacement we should pre-install on the machine as mentioned above. Otherwise this test will break out of band (the string previews are, unlike the number values, short lived in how they are vended by android).
There was a problem hiding this comment.
At least, this used to be true. So feel free to check if it isn't true anymore and we don't need to pre-install.
There was a problem hiding this comment.
Yeah I think Android still deletes the preview. I think we have to pre-install.
|
Narrowing the scope of this PR for CP. Also no longer bumping AGP in FGP |
These are the minimum required changes to update our templates to AGP 9. Narrowed the scope from [here](flutter#185730). This must be CP'ed to beta and stable. Bumped templates to AGP 9 to ensure all newly created Flutter apps are on AGP 9 (and do not get the KGP warning). AGP: 9.0.0 Gradle: 9.1.0 KGP: 2.3.20 Migrated app, module, and plugin templates from KGP to Built-in Kotlin [Built-in Kotlin](https://docs.flutter.dev/release/breaking-changes/migrate-to-built-in-kotlin). Even though all necessary changes have been made to migrate to Built-in Kotlin, Flutter does not support actually using Built-in Kotlin because the default is still the legacy (by design). Fixed some breaking changes when upgrading templates: - android projects that contain similar package names fail to build starting AGP 9 [here](https://developer.android.com/build/releases/agp-9-0-0-release-notes#:~:text=Enforces%20that%20each%20library%20has%20a%20distinct%20package%20name). - compileSdkVersion was deprecated in AGP < 9 and completely removed in AGP 9 [here](https://developer.android.com/reference/tools/gradle-api/8.11/com/android/build/api/dsl/CommonExtension#compileSdkVersion(kotlin.Int)). - Java 17 is the new minimum for Gradle 9+ [here](https://docs.gradle.org/9.0.0/release-notes.html#jvm-17). This is a no-op (except for unit tests) on our end because Java 17 has been the minimum for AGP since AGP 8. Due to this, the warning message cites Gradle version as the reason Java must be bumped instead of the AGP version. ## 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. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
3 participants
Bumped templates to AGP 9 to ensure all newly created Flutter apps are on AGP 9 (and do not get the KGP warning).
AGP: 9.0.0
Gradle: 9.1.0
KGP: 2.3.20
Migrated app, module, and plugin templates from KGP to Built-in Kotlin Built-in Kotlin. Even though all necessary changes have been made to migrate to Built-in Kotlin, Flutter does not support actually using Built-in Kotlin because the default is still the legacy (by design).
Fixed some breaking changes when upgrading:
This will be CP to beta and stable
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.