Fix Gradle warning in a freshly flutter createed Android project#122290
Fix Gradle warning in a freshly flutter createed Android project#122290auto-submit[bot] merged 17 commits intoflutter:masterfrom bartekpacia:fix/flutter_create_android_gradle_task_warning
flutter createed Android project#122290Conversation
|
Neat! Can you try adding a migration for this? That way (a) existing Flutter projects will get the benefit too, (b) the divergence between Flutter projects created at different times can be reduced. |
|
Thanks for taking a look and for these links :) Even though I consider myself a fairly experienced Flutter developer, I have never used The migration sounds like a good idea, but I don't think it's worth it for this change. After all, it's a very minor change and I'm pretty sure the performance benefit is negligible. People who'll get the warning will Google it and replace the default, eager That said, if you think it's worth adding this anyway, I'll be happy to dive in and write the migration when I find some free time :) |
Yeah, this appears to be an entirely separate system from If you take one of those existing migration classes and look at where it's used, you find that the migrations are applied as basically the first step of final List<ProjectMigrator> migrators = <ProjectMigrator>[
CmakeCustomCommandMigration(linuxProject, globals.logger),
];
final ProjectMigration migration = ProjectMigration(migrators);
migration.run();before it goes on to call So this system doesn't need any advertising; it's already integrated into people's normal workflows.
Everyone will get the warning when they upgrade to Gradle 8+, right? Everyone who's building for Android, anyway. Many Flutter developers will make that upgrade themselves after Gradle 8 reaches a stable release. And then in a few years everyone will make that upgrade who hasn't already, with the successor to #122376 when some Gradle 8+ version is needed for Java 21. That's a lot of people who can be saved the effort of spotting this warning, figuring out what it's talking about, locating the fix, and then applying it and convincing themselves it was correct and didn't break anything. (Especially as many of those developers will know very little about Gradle — I suspect that even most people writing native Android apps try to avoid thinking about Gradle much, and all the more so for people working mainly in Flutter.) So if you're up for taking the understanding you've already acquired of what this warning is about, and spending a bit of time to write and test an automated migration that encodes that understanding, then I think that would be a quite useful thing to do. It looks like this would also be the pioneering example of a migration for Android builds — all the other platforms have one or more (and iOS is in the lead with ten), but not Android. Fortunately I think that part will be straightforward, just adding a few lines at the top of |
|
Thank you very much for this thorough explanation @gnprice. I agree with this - let's spare people some Gradle warnings :) I'll add the migration code to this PR when I find some free time (in a few days). |
|
Great! If you find you have any questions about how that migration system works, I recommend you go to the tools channel in Discord and ping jmagman there. She's the expert on this system, and I've seen her several times recently recommend people use it for various changes. |
|
Hi @gnprice, I added a migrator :) I'll be happy if you review it! |
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
gnprice
left a comment
There was a problem hiding this comment.
Thanks for writing this! Generally looks great. Various small comments below.
packages/flutter_tools/lib/src/android/migrations/top_level_gradle_build_file_migration.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/lib/src/android/migrations/top_level_gradle_build_file_migration.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/lib/src/android/migrations/top_level_gradle_build_file_migration.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
|
Would also be great to have a review from someone who's worked with this migration system. @jmagman, tagging you in as the expert, but please feel free to pass it off to someone else. |
|
Thanks a lot for the review @gnprice! I addressed all the things you mentioned. |
gnprice
left a comment
There was a problem hiding this comment.
Thanks! LGTM except one nit. We'll also want that second review as mentioned above at #122290 (comment) .
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
jmagman
left a comment
There was a problem hiding this comment.
This migration should be run in all the example/integration test projects so when folks run them locally they don't have changes to their working copy.
Here's an example of a migration that did that: https://github.com/flutter/flutter/pull/108331/files
|
@reidbaker can you add the right person to review? |
|
Uh, EDIT 1: Looks like it's a known issue #123018 EDIT 2: Works after rebase |
I don't have any experience writing migrators, so I would defer to you @reidbaker. |
There was a problem hiding this comment.
@camsim99 @reidbaker @gnprice I can review the "migration" part (that LGTM) but I don't know anything about the gradle part of this.
This only changes android/build.gradle but not android/app/build.gradle, do they both need to be changed?
There was a problem hiding this comment.
Yeah, changing just the one is correct. That's the only one where the templates happen to contain the pattern that generates the Gradle warning in #122289.
packages/flutter_tools/test/general.shard/android/android_project_migration_test.dart
Outdated
Show resolved
Hide resolved
This update gets made automatically by a `flutter build` or `flutter run` command targeting Android. It was introduced last week: flutter/flutter#122290 flutter/flutter@c40dd27
This update gets made automatically by a `flutter build` or `flutter run` command targeting Android. It was introduced last week: flutter/flutter#122290 flutter/flutter@c40dd27
This PR fixes #122289
Pre-launch Checklist
///).