Convert base application name handling to kotlin source (start of FGP kt conversion) #155963

gmackall · 2024-09-30T22:52:19Z

Wires up a new gradle subproject defining kotlin classes to be used by the FGP, so that we can incrementally move the entire plugin to be written in kotlin source.

Starts by moving a piece of kotlin script.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
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.

gmackall · 2024-09-30T23:08:01Z

cc @reidbaker I couldn't figure out how to wire up kotlin files to work in the same gradle project, but I was able to get it working if I define them in a sub project like this. WDYT about this approach?

(still needs tests/to figure out how to make the module case work, so not ready for review)

gmackall · 2024-10-01T00:00:09Z

Looks like in particular this doesn't work with the legacy script application of the flutter gradle plugin. I wonder if I can somehow hack flutter.gradle to get it working.

reidbaker · 2024-10-01T14:11:17Z

Looks like in particular this doesn't work with the legacy script application of the flutter gradle plugin. I wonder if I can somehow hack flutter.gradle to get it working.

I think it is time to go back to java class based conversions. We ran into the kotlin legacy script issue several times before. Java conversions are net better, net reduction in languages and net easier to test.

gmackall · 2024-10-01T17:18:35Z

Looks like in particular this doesn't work with the legacy script application of the flutter gradle plugin. I wonder if I can somehow hack flutter.gradle to get it working.

I think it is time to go back to java class based conversions. We ran into the kotlin legacy script issue several times before. Java conversions are net better, net reduction in languages and net easier to test.

This is unfortunately also a problem with the approach used for the Java conversion, and for the same reason.

The new application of the flutter gradle plugin includes the directory that contains the plugin code in the build as a composite build, like this:

flutter/examples/hello_world/android/settings.gradle.kts

Line 19 in f05e92f

includeBuild("$flutterSdkPath/packages/flutter_tools/gradle")

But the old application applies it as a script plugin, which means that the existing build.gradle.kts is ignored, and the script is instead built only with the included buildscript block. So we ignore the application of the java-gradle-plugin and then subsequent configuration needed for that plugin, aka these lines

flutter/packages/flutter_tools/gradle/build.gradle.kts

Lines 5 to 26 in f7bcb28

    
           plugins { 
        
               `java-gradle-plugin` 
        
               `groovy` 
        
           } 
        
           group = "dev.flutter.plugin" 
        
           version = "1.0.0" 
        
           gradlePlugin { 
        
               plugins { 
        
                   // The "flutterPlugin" name isn't used anywhere. 
        
                   create("flutterPlugin") { 
        
                       id = "dev.flutter.flutter-gradle-plugin" 
        
                       implementationClass = "FlutterPlugin" 
        
                   } 
        
                   // The "flutterAppPluginLoaderPlugin" name isn't used anywhere. 
        
                   create("flutterAppPluginLoaderPlugin") { 
        
                       id = "dev.flutter.flutter-plugin-loader" 
        
                       implementationClass = "FlutterAppPluginLoaderPlugin" 
        
                   } 
        
               } 
        
           }

gmackall · 2024-10-11T21:00:10Z

This is going to be blocked until we turn down the script application of the flutter gradle plugin, which we can start to do after Feb 2025.

It would be good to migrate all of our own code in the meantime.

gmackall · 2024-12-13T18:15:32Z

cc @bartekpacia

bartekpacia · 2025-01-07T22:31:17Z

packages/flutter_tools/gradle/src/main/kotlin/flutter.gradle.kts

finally we're getting rid of this hack

and we'll get proper support in IntelliJ for developing the gradle plugin

I'm excited!

bartekpacia · 2025-01-07T22:33:02Z

...flutter_tools/gradle/kt/src/test/kotlin/com/flutter/gradle/BaseApplicationNameHandlerTest.kt

+
+class BaseApplicationNameHandlerTest {
+    @Test
+    fun setBaseName_respectsFlutterToolProperty() {


Kotlin supports this:

Suggested change

fun setBaseName_respectsFlutterToolProperty() {

fun `setBaseName respects Flutter tool property`() {

it's a matter of taste but I think it's cool.

(not sure if there are some android-specific problems with it though)

that's awesome, changed!

Still need to get these tests hooked up to run in CI, that is the last part of the PR. Working on it now.

FYI that they are runnable (with coverage even) in Android studio now through the ui

bartekpacia · 2025-01-07T22:34:21Z

...ges/flutter_tools/gradle/kt/src/main/kotlin/com/flutter/gradle/BaseApplicationNameHandler.kt

+class BaseApplicationNameHandler {
+    companion object {


I think this can be simply an object (instead of class + companion object)?

Seems to work, changed! I haven't had the chance to write much Kotlin, so extra scrutiny on things that look like abnormal Kotlin is appreciated

bartekpacia · 2025-01-09T18:58:56Z

Question:
Why make a kt subproject and then include it?

I think a better layout would be this (what I have currently in #161352):

$ pwd
(...)/flutter/packages/flutter_tools/gradle
$ tree ./src
src
└── main
    ├── groovy
    │   ├── CMakeLists.txt
    │   ├── flutter.groovy
    │   └── native_plugin_loader.groovy
    ├── kotlin
    │   ├── app_plugin_loader.kt
    │   └── native_plugin_loader.kt
    └── kotlin_scripts
        ├── dependency_version_checker.gradle.kts
        └── flutter.gradle.kts

gmackall · 2025-01-09T19:15:23Z

Question: Why make a kt subproject and then include it?

I think a better layout would be this (what I have currently in #161352):

$ pwd
(...)/flutter/packages/flutter_tools/gradle
$ tree ./src
src
└── main
    ├── groovy
    │   ├── CMakeLists.txt
    │   ├── flutter.groovy
    │   └── native_plugin_loader.groovy
    ├── kotlin
    │   ├── app_plugin_loader.kt
    │   └── native_plugin_loader.kt
    └── kotlin_scripts
        ├── dependency_version_checker.gradle.kts
        └── flutter.gradle.kts

Hmm I can try changing to that structure, I don't remember the reason why at this point if there was one (the PR is pretty old)

bartekpacia · 2025-01-09T19:23:05Z

cool!

I'll just add that kotlin_scripts is temporary (.kts files aren't "proper" .kt files, i.e. they contain executable code at top-level), so they have to be outside the Kotlin source set src/main/kotlin.
We want to get rid of kotlin_scripts by converting them to proper Kotlin and put them inside src/main/kotlin.

bkonyi

Tool test LGTM, RSLGTM on the Kotlin changes.

…t of FGP kt conversion) (flutter/flutter#155963)

Roll Flutter from 72db8f6 to 40c2b86 (33 revisions) flutter/flutter@72db8f6...40c2b86 2025-01-14 [email protected] update changelog for 3.27.2 release (flutter/flutter#161569) 2025-01-14 [email protected] Fix `showLicensePage` does not inherit ambient `Theme` (flutter/flutter#161599) 2025-01-14 [email protected] Added special case for fat width arcs (flutter/flutter#161255) 2025-01-14 [email protected] Replace `fetch `with `gclient sync`. (flutter/flutter#161565) 2025-01-14 [email protected] Roll Packages from 3c3bc68 to d1fd623 (4 revisions) (flutter/flutter#161597) 2025-01-14 [email protected] Remove unused method (flutter/flutter#161572) 2025-01-14 [email protected] Fix crash when closing a window with `Alt+F4` in multi-win Flutter on Windows (flutter/flutter#161375) 2025-01-14 [email protected] Update InputDecoration.border documentation (flutter/flutter#161415) 2025-01-14 [email protected] [Web] Allow specifying the strategy on when to use <img> element to display images (flutter/flutter#159917) 2025-01-14 [email protected] Roll Dart to Version 3.7.0-323.0.dev (flutter/flutter#161567) 2025-01-14 [email protected] Use wildcards (flutter/flutter#161548) 2025-01-14 [email protected] Autocomplete Options Width (flutter/flutter#143249) 2025-01-13 [email protected] Move the analyzer_benchmark to Mac arm64 devicelab bots (flutter/flutter#161405) 2025-01-13 [email protected] Remove references to `cirrus`, mostly in doc comments. (flutter/flutter#161529) 2025-01-13 [email protected] Fix paths when running clang-tidy on git diffs (flutter/flutter#161496) 2025-01-13 [email protected] [web:a11y] treat empty tappables as buttons (flutter/flutter#161360) 2025-01-13 [email protected] Add route settings to CupertinoSheetRoute (flutter/flutter#161528) 2025-01-13 [email protected] Copy `linux_host_engine` as `linux_host_engine_test`, removing `archives: [...]`. (flutter/flutter#161532) 2025-01-13 [email protected] Remove last two references to Cirrus CI. (flutter/flutter#161530) 2025-01-13 [email protected] Mark `Mac_mokey microbenchmarks` as flakey (flutter/flutter#161550) 2025-01-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (#160241)" (flutter/flutter#161555) 2025-01-13 [email protected] Add validator execution times to `flutter doctor --verbose` (flutter/flutter#158124) 2025-01-13 [email protected] Explain more specifically how to use `flutter drive`/what it does (flutter/flutter#161450) 2025-01-13 [email protected] Fixed repeated strings for incompatible Gradle or AGP version in `create` command (flutter/flutter#161223) 2025-01-13 [email protected] Remove `WEB_SHARD_COUNT`, which no longer exists post-Cirrus. (flutter/flutter#161527) 2025-01-13 [email protected] [Impeller] Update guidance on prebuilt artifacts. (flutter/flutter#161251) 2025-01-13 [email protected] Migrate DisplayList unit tests to DL/Impeller geometry classes (flutter/flutter#161453) 2025-01-13 [email protected] Context menu button callback docs clarification (flutter/flutter#161451) 2025-01-13 [email protected] Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (flutter/flutter#160241) 2025-01-13 [email protected] Udpate documentation on the third_party directories (flutter/flutter#161407) 2025-01-13 [email protected] Propagate environment variables when `flutter drive` is invoked. (flutter/flutter#161452) 2025-01-13 [email protected] Convert base application name handling to kotlin source (start of FGP kt conversion) (flutter/flutter#155963) 2025-01-13 [email protected] [Impeller] remove API 30 restriction for SurfaceControl testing. (flutter/flutter#161438) 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] 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: ...

…t of FGP kt conversion) (flutter/flutter#155963)

Roll Flutter from 72db8f6 to 40c2b86 (33 revisions) flutter/flutter@72db8f6...40c2b86 2025-01-14 [email protected] update changelog for 3.27.2 release (flutter/flutter#161569) 2025-01-14 [email protected] Fix `showLicensePage` does not inherit ambient `Theme` (flutter/flutter#161599) 2025-01-14 [email protected] Added special case for fat width arcs (flutter/flutter#161255) 2025-01-14 [email protected] Replace `fetch `with `gclient sync`. (flutter/flutter#161565) 2025-01-14 [email protected] Roll Packages from 3c3bc68 to d1fd623 (4 revisions) (flutter/flutter#161597) 2025-01-14 [email protected] Remove unused method (flutter/flutter#161572) 2025-01-14 [email protected] Fix crash when closing a window with `Alt+F4` in multi-win Flutter on Windows (flutter/flutter#161375) 2025-01-14 [email protected] Update InputDecoration.border documentation (flutter/flutter#161415) 2025-01-14 [email protected] [Web] Allow specifying the strategy on when to use <img> element to display images (flutter/flutter#159917) 2025-01-14 [email protected] Roll Dart to Version 3.7.0-323.0.dev (flutter/flutter#161567) 2025-01-14 [email protected] Use wildcards (flutter/flutter#161548) 2025-01-14 [email protected] Autocomplete Options Width (flutter/flutter#143249) 2025-01-13 [email protected] Move the analyzer_benchmark to Mac arm64 devicelab bots (flutter/flutter#161405) 2025-01-13 [email protected] Remove references to `cirrus`, mostly in doc comments. (flutter/flutter#161529) 2025-01-13 [email protected] Fix paths when running clang-tidy on git diffs (flutter/flutter#161496) 2025-01-13 [email protected] [web:a11y] treat empty tappables as buttons (flutter/flutter#161360) 2025-01-13 [email protected] Add route settings to CupertinoSheetRoute (flutter/flutter#161528) 2025-01-13 [email protected] Copy `linux_host_engine` as `linux_host_engine_test`, removing `archives: [...]`. (flutter/flutter#161532) 2025-01-13 [email protected] Remove last two references to Cirrus CI. (flutter/flutter#161530) 2025-01-13 [email protected] Mark `Mac_mokey microbenchmarks` as flakey (flutter/flutter#161550) 2025-01-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (#160241)" (flutter/flutter#161555) 2025-01-13 [email protected] Add validator execution times to `flutter doctor --verbose` (flutter/flutter#158124) 2025-01-13 [email protected] Explain more specifically how to use `flutter drive`/what it does (flutter/flutter#161450) 2025-01-13 [email protected] Fixed repeated strings for incompatible Gradle or AGP version in `create` command (flutter/flutter#161223) 2025-01-13 [email protected] Remove `WEB_SHARD_COUNT`, which no longer exists post-Cirrus. (flutter/flutter#161527) 2025-01-13 [email protected] [Impeller] Update guidance on prebuilt artifacts. (flutter/flutter#161251) 2025-01-13 [email protected] Migrate DisplayList unit tests to DL/Impeller geometry classes (flutter/flutter#161453) 2025-01-13 [email protected] Context menu button callback docs clarification (flutter/flutter#161451) 2025-01-13 [email protected] Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (flutter/flutter#160241) 2025-01-13 [email protected] Udpate documentation on the third_party directories (flutter/flutter#161407) 2025-01-13 [email protected] Propagate environment variables when `flutter drive` is invoked. (flutter/flutter#161452) 2025-01-13 [email protected] Convert base application name handling to kotlin source (start of FGP kt conversion) (flutter/flutter#155963) 2025-01-13 [email protected] [Impeller] remove API 30 restriction for SurfaceControl testing. (flutter/flutter#161438) 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] 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: ...

Gray Mackall added 5 commits September 30, 2024 15:14

working state

82b6690

working state

fb173b4

move base application name handling to kotlin source

f31231e

delete proof of concept

2863ac6

don't push logging :)

2fdccbb

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 30, 2024

todo:

bc0a375

add breadcrumb

cd59cdb

Gray Mackall added 2 commits October 3, 2024 12:26

write unit tests (still need to wire them up to run

72ff261

change visible for testing to internal

186a0e6

gmackall mentioned this pull request Dec 10, 2024

Partially remove Groovy dynamic dispatch from portion of FGP that handles setting application name #152509

Closed

9 tasks

Merge branch 'master' into new_kotlin_approach

e8b032e

github-actions bot added the platform-android Android applications specifically label Jan 7, 2025

bartekpacia reviewed Jan 7, 2025

View reviewed changes

gmackall and others added 2 commits January 8, 2025 10:51

Merge branch 'master' into new_kotlin_approach

a03f517

return early if no ApplicationExtension

cc5d4e0

Gray Mackall added 3 commits January 9, 2025 12:16

copy barteks structure and move and fix tests

e942200

rename tests, this is cool

aafd1bd

make it just an object (not companion)

4d47b58

make makeexecutable a no op on windows

5d66117

gmackall marked this pull request as ready for review January 13, 2025 17:30

gmackall requested review from a team and bkonyi January 13, 2025 17:33

bkonyi approved these changes Jan 13, 2025

View reviewed changes

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

auto-submit bot added this pull request to the merge queue Jan 13, 2025

Merged via the queue into flutter:master with commit 6e8d807 Jan 13, 2025
166 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

gmackall mentioned this pull request Jan 13, 2025

[FGP to Kotlin conversion] Convert app_plugin_loader Gradle plugin to Kotlin #161352

Closed

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to f2c15f9debb2 (27 revisions) flutter/packages#8424

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

6e8d807 Convert base application name handling to kotlin source (star…

2c9147a

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to c9923ca609cc (28 revisions) flutter/packages#8425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

6e8d807 Convert base application name handling to kotlin source (star…

436c493

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to ed9f9206629b (29 revisions) flutter/packages#8426

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

6e8d807 Convert base application name handling to kotlin source (star…

9cdbe75

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to 40c2b86fdf65 (33 revisions) flutter/packages#8428

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

6e8d807 Convert base application name handling to kotlin source (star…

fc7cf97

…t of FGP kt conversion) (flutter/flutter#155963)

gmackall mentioned this pull request Jan 15, 2025

Comparison method violates its general contract! error across many Android tests. #103230

Closed

gmackall mentioned this pull request Jan 23, 2025

☂️ [FGP Kotlin conversion] Umbrella bug to track conversion progress #162103

Open

15 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

6e8d807 Convert base application name handling to kotlin source (star…

10e052f

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

6e8d807 Convert base application name handling to kotlin source (star…

966b3f1

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

6e8d807 Convert base application name handling to kotlin source (star…

ef5438c

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

6e8d807 Convert base application name handling to kotlin source (star…

2274ebb

…t of FGP kt conversion) (flutter/flutter#155963)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

6e8d807 Convert base application name handling to kotlin source (star…

d371ca1

…t of FGP kt conversion) (flutter/flutter#155963)

	fun setBaseName_respectsFlutterToolProperty() {
	fun `setBaseName respects Flutter tool property`() {

Convert base application name handling to kotlin source (start of FGP kt conversion) #155963

Convert base application name handling to kotlin source (start of FGP kt conversion) #155963

Uh oh!

Conversation

gmackall commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gmackall commented Sep 30, 2024

Uh oh!

gmackall commented Oct 1, 2024

Uh oh!

reidbaker commented Oct 1, 2024

Uh oh!

gmackall commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmackall commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmackall commented Dec 13, 2024

Uh oh!

bartekpacia Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartekpacia Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

gmackall Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

bartekpacia Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

gmackall Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

bartekpacia commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmackall commented Jan 9, 2025

Uh oh!

bartekpacia commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkonyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gmackall commented Sep 30, 2024 •

edited

Loading

gmackall commented Oct 1, 2024 •

edited

Loading

gmackall commented Oct 11, 2024 •

edited

Loading

bartekpacia Jan 7, 2025 •

edited

Loading

bartekpacia commented Jan 9, 2025 •

edited

Loading

bartekpacia commented Jan 9, 2025 •

edited

Loading