Reland (2): "Fix how Gradle resolves Android plugin" #142498

Gustl22 · 2024-01-30T00:41:01Z

Previous PR: #137115,
Revert: #142464
Fixes #141940
Closes #142487

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.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…tter#137115)" (flutter#142464)" This reverts commit 995e3fa.

Gustl22 · 2024-02-02T19:59:03Z

Tests fail as expected for enabled gradle daemon (before the fix): https://github.com/flutter/flutter/pull/142498/checks?check_run_id=21166626702

I reverted the revert with 309767c and added the tests with 851fe0d and added the fix with cddc017

Gustl22 · 2024-02-02T20:05:31Z

CC @bartekpacia @camsim99 @reidbaker

…-resolve-android # Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

reidbaker

This looks good and I appreciate the effort you took to write a test that overrides the daemon options to catch a regression.

I am going to give this another pass after I have had some time to check it out and build locally but it looks good.

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

reidbaker · 2024-02-05T15:17:35Z

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

+            if (pluginProject == null) {
+                // Plugin was not included in `settings.gradle`, but is listed in `.flutter-plugins`.
+                project.logger.error("Plugin project :${it.name} listed, but not found. Please fix your settings.gradle/settings.gradle.kts.")
+            } else if (doesSupportAndroidPlatform(pluginProject.projectDir.parentFile.path as String)) {


Should this line warn the caller that their project is misconfigured?

Indeed, an example is tested here

…-resolve-android

See [#142498](#142498 (comment)) See this discussion: https://discord.com/channels/608014603317936148/1186378330178601000

…-resolve-android # Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

Gustl22 · 2024-02-15T16:31:30Z

@reidbaker small ping if there's time to finish the review :)

reidbaker · 2024-02-15T17:00:18Z

I added a teammate who can give this a faster review. Sorry for the delay.

camsim99 · 2024-02-15T20:55:07Z

Tests fail as expected for enabled gradle daemon (before the fix): https://github.com/flutter/flutter/pull/142498/checks?check_run_id=21166626702

I reverted the revert with 309767c and added the tests with 851fe0d and added the fix with cddc017

The test added LGTM but can you give some context as to why cddc017
fixes #141940?

Also pinged @gmackall for a review if you can since you are more of a Gradle expert!

Gustl22 · 2024-02-15T23:22:24Z

Thank you :)

The test added LGTM but can you give some context as to why cddc017
fixes #141940?

As stuartmorgan described in #141940 (comment) a Singleton persists across builds in gradle daemon. So the Singleton keyword is removed in cddc017. To not read the same file twice during the same build, some local variables were introduced (pluginList and pluginDependencies) in flutter.groovy to ensure consistency and push performance.

Edit: I just noticed to be able optimize further in 530b0c3

camsim99

The fix for the cause of the revert LGTM! @bartekpacia or @gmackall for second review?

gmackall · 2024-02-16T18:04:50Z

I would defer to someone who reviewed the original PR (I think the delta here between original (reland 1) and reland 2 is small) but can take a look if no one else is able to. It'll take me a bit though, as I'll have to review (original+reland delta)

reidbaker

Thank you for your effort to reland this pr.

Gustl22 · 2024-02-19T19:53:41Z

Thank you for reviewing!

…/flutter#142498)

flutter/flutter@064c340...5129806 2024-02-20 [email protected] Roll Flutter Engine from 0abe2b9d6c7c to 781f308c6555 (1 revision) (flutter/flutter#143750) 2024-02-20 [email protected] Roll Flutter Engine from 92aad0d0fcee to 0abe2b9d6c7c (1 revision) (flutter/flutter#143745) 2024-02-20 [email protected] Roll Flutter Engine from 2335115f08d3 to 92aad0d0fcee (2 revisions) (flutter/flutter#143736) 2024-02-20 [email protected] Roll Flutter Engine from e96c18b6c5ee to 2335115f08d3 (1 revision) (flutter/flutter#143731) 2024-02-20 [email protected] Small fixes in TextEditingController docs (flutter/flutter#143717) 2024-02-19 [email protected] Roll Flutter Engine from b41494f009f4 to e96c18b6c5ee (1 revision) (flutter/flutter#143722) 2024-02-19 [email protected] Roll Flutter Engine from 714215d42e57 to b41494f009f4 (1 revision) (flutter/flutter#143713) 2024-02-19 [email protected] Reland (2): "Fix how Gradle resolves Android plugin" (flutter/flutter#142498) 2024-02-19 [email protected] Roll Packages from 0af905d to 84ff11d (1 revision) (flutter/flutter#143710) 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],[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

…/flutter#142498)

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

Gustl22 force-pushed the 137115-reland-2-fix-resolve-android branch from 67ba0c2 to de0027c Compare February 2, 2024 13:47

Revert "Revert "Reland: "Fix how Gradle resolves Android plugin" (flu…

309767c

…tter#137115)" (flutter#142464)" This reverts commit 995e3fa.

Gustl22 force-pushed the 137115-reland-2-fix-resolve-android branch 3 times, most recently from 9140096 to 391b2a9 Compare February 2, 2024 18:39

Gustl22 added 2 commits February 2, 2024 20:05

Gradle daemon test

851fe0d

Replace singleton (closes flutter#141940)

cddc017

Gustl22 force-pushed the 137115-reland-2-fix-resolve-android branch from 9e3e4c3 to 851fe0d Compare February 2, 2024 19:07

Gustl22 marked this pull request as ready for review February 2, 2024 19:59

Merge branch 'master' into 137115-reland-2-fix-resolve-android

088c929

Merge remote-tracking branch 'upstream/main' into 137115-reland-2-fix…

efb0248

…-resolve-android # Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

Gustl22 mentioned this pull request Feb 2, 2024

Reland "Add support for Gradle Kotlin DSL (#140744)" #142752

Merged

8 tasks

Remove whitespace

e80682f

reidbaker requested review from bartekpacia and reidbaker February 5, 2024 15:07

reidbaker reviewed Feb 5, 2024

View reviewed changes

Gustl22 added 4 commits February 5, 2024 18:12

Adapt string comparison tests

8e5404f

Test build.gradle without source path

dd13bd3

Merge remote-tracking branch 'upstream/main' into 137115-reland-2-fix…

fce6d39

…-resolve-android

Remove unnecessary plugins to test default flutter source path

408a161

Gustl22 added a commit to Gustl22/flutter that referenced this pull request Feb 5, 2024

Set default flutter source directory (flutter#142498)

2ed4522

Gustl22 mentioned this pull request Feb 5, 2024

Set default flutter source directory for gradle builds #142934

Merged

8 tasks

Revert adding default flutter source directory path

c76821c

Gustl22 added 3 commits February 9, 2024 01:43

Merge remote-tracking branch 'upstream/main' into 137115-reland-2-fix…

fad71ea

…-resolve-android # Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

Merge branch 'master' into 137115-reland-2-fix-resolve-android

95cd8d1

Merge branch 'master' into 137115-reland-2-fix-resolve-android

9d6be76

Gustl22 requested a review from reidbaker February 15, 2024 15:32

reidbaker requested a review from camsim99 February 15, 2024 16:39

camsim99 requested a review from gmackall February 15, 2024 17:02

Gustl22 added 2 commits February 16, 2024 00:32

feat: Optimize pluginDependencies assignment

530b0c3

Merge branch 'master' into 137115-reland-2-fix-resolve-android

670afbd

camsim99 approved these changes Feb 16, 2024

View reviewed changes

reidbaker approved these changes Feb 19, 2024

View reviewed changes

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 19, 2024

auto-submit bot merged commit 9620e3f into flutter:master Feb 19, 2024

Gustl22 deleted the 137115-reland-2-fix-resolve-android branch February 19, 2024 19:53

engine-flutter-autoroll mentioned this pull request Feb 20, 2024

Roll Flutter from 064c340baf0e to 1a7a0618a718 (8 revisions) flutter/packages#6163

Closed

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

9620e3f Reland (2): "Fix how Gradle resolves Android plugin" (flutter…

a8ecaee

…/flutter#142498)

engine-flutter-autoroll mentioned this pull request Feb 20, 2024

Roll Flutter from 064c340baf0e to 5129806e6c63 (9 revisions) flutter/packages#6164

Merged

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

9620e3f Reland (2): "Fix how Gradle resolves Android plugin" (flutter…

fb11244

…/flutter#142498)

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

9620e3f Reland (2): "Fix how Gradle resolves Android plugin" (flutter…

0960cbb

…/flutter#142498)

Reland (2): "Fix how Gradle resolves Android plugin" #142498

Reland (2): "Fix how Gradle resolves Android plugin" #142498

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

Gustl22 commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gustl22 commented Feb 2, 2024

Uh oh!

reidbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reidbaker Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

Gustl22 Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

Gustl22 commented Feb 15, 2024

Uh oh!

reidbaker commented Feb 15, 2024

Uh oh!

camsim99 commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gustl22 commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

reidbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Gustl22 commented Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Gustl22 commented Jan 30, 2024 •

edited

Loading

Gustl22 commented Feb 2, 2024 •

edited

Loading

camsim99 commented Feb 15, 2024 •

edited

Loading

Gustl22 commented Feb 15, 2024 •

edited

Loading

gmackall commented Feb 16, 2024 •

edited

Loading