Revert "Reland: "Fix how Gradle resolves Android plugin" (#137115)" #142266

camsim99 · 2024-01-25T22:20:03Z

This reverts commit f5ac225, i.e. #137115.

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.

)" This reverts commit f5ac225.

…relevant to legacy plugins

camsim99 · 2024-01-25T22:47:19Z

@bartekpacia This revert is not clean; I have to merge changes from #140744 in flutter.groovy. Looks like your changes there had to do with configuring legacy plugins, which is removed logic by reverting this PR, so I think it should be fine but would super appreciate your review here! I ended up deleting all of your changes in flutter.groovy from that PR except for the changes to comments you made.

I also had to potentially merge changes from #141692 since I modified flutter.groovy, but I don't think I undid any of those changes.

FYI @Gustl22 to know to re-land this change!

bartekpacia · 2024-01-25T23:30:29Z

Hi @camsim99, overall this looks good, I understand the need to revert this fast. However I'd (what a surprise, heh) prefer not to have my "Add support for Gradle Kotlin DSL" PR reverted.

Will you reland my changes from #140744 in this PR, or in a different PR, or will they also be reverted and I'll have to make them again?

bartekpacia · 2024-01-25T23:31:31Z

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

-    private Boolean doesSupportAndroidPlatform(String path) {
-        File buildGradle = new File(path, 'android' + File.separator + 'build.gradle')
-        File buildGradleKts = new File(path, 'android' + File.separator + 'build.gradle.kts')
-        if (buildGradle.exists() && buildGradleKts.exists()) {
-            logger.error(
-                "Both build.gradle and build.gradle.kts exist, so " +
-                "build.gradle.kts is ignored. This is likely a mistake."
-            )
-        }


removing this also effectively reverts #140744

I see this implementation

private Boolean doesSupportAndroidPlatform(String path) { File editableAndroidProject = new File(path, 'android' + File.separator + 'build.gradle') return editableAndroidProject.exists() }

below. Does replacing your implementation with this one help keep your change?

yup, let's try that

It looks like that PR landed after the 3.19 branch point? Keep in mind that the more complex this revert is in terms of mixing pre- and post-branch code, the harder making a cherry-pick will be.

Right ok that makes sense. This was part of #140744. The other option would be removing it. If I do that to make the cherry-pick easier, @bartekpacia will your entire PR have to be reverted?

If not, we could potentially remove it for this revert to cherry-pick and then we can land adding back in a follow-up PR so it at least lives in the master branch.

Depending on the answer here, I'm thinking either also revert that PR or just delete this newer code for now to get this landed.

@Gustl22 could you try reading #141940 (comment) and trying again? It may help you repro. I will also try to repro with/without this PR!

@camsim99 do whatever is best for you, I can always reland my contribution later! If reverting my whole PR is easier for you then go for it.

It's definitely easier just to delete this implementation of the method for now, so I'll go ahead with that. I will file an issue to put it back so we can track the removal, but I will try to follow this up immediately with that :)

bartekpacia · 2024-01-25T23:31:51Z

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

-    private File settingsGradleFile(Project project) {
-        File settingsGradle = new File(project.projectDir.parentFile, "settings.gradle")
-        File settingsGradleKts = new File(project.projectDir.parentFile, "settings.gradle.kts")
-        if (settingsGradle.exists() && settingsGradleKts.exists()) {
-            logger.error(
-                "Both settings.gradle and settings.gradle.kts exist, so " +
-                "settings.gradle.kts is ignored. This is likely a mistake."
-            )
-        }
-
-        return settingsGradle.exists() ? settingsGradle : settingsGradleKts


removing this also effectively reverts #140744

I see this was used above in

try { if (!settingsGradleFile(project).text.contains("'.flutter-plugins'")) { return } } catch (FileNotFoundException ignored) { throw new GradleException("settings.gradle/settings.gradle.kts does not exist: ${settingsGradleFile(project).absolutePath}") }

Do I need to add the method back + add these checks to the all configurePlugins method to keep your changes? Trying to determine what exactly you need here (and above) and if it's possible to not trigger the regression.

Ah I think I see. If the only usage of the settingsGradleFile() method was in the code that is now being removed, I think it's fine to remove settingsGradleFile() as well.

camsim99 · 2024-01-25T23:44:49Z

Will you reland my changes from #140744 in this PR, or in a different PR, or will they also be reverted and I'll have to make them again?

Ideally, I'd like to keep your changes in this PR if it's possible! I probably will need your guidance, though; I'll leave comments inline.

Gustl22 · 2024-01-26T08:56:29Z

I'd be glad testing with and without this revert, if the issue is actually fixed, before merging. Until now I'm not able to reproduce the issue with different OS and different stages of beta & main. @camsim99 could you reproduce the steps of stuartmorgan(?) Big thanks :)

bartekpacia · 2024-01-26T09:30:08Z

Hey @camsim99:

I just checked out this branch and built a Flutter app of mine that is fully migrated to Gradle Kotlin DSL, and it builds fine.
I merged this PR of yours with master since I noticed "ci.yaml validation failed" (a new builder was added), so when you wake up CI results should be available. If that's not OK then sorry and I won't do it again.

bartekpacia · 2024-01-26T09:32:25Z

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

     * In an app project, this is ../.. since the app's Gradle build file is under android/app.
     */
-    String source = "../.."
+    String source


just a note:

If we want to have a default value, we should also have a test that checks that the app builds fine if flutter { source "../.." } is missing from android/app/build.gradle

fyi @Gustl22

See also:

this discussion

Thank you @bartekpacia. I actually don't propose to remove the source property from the build.gradle. I propose to test and/or remove it in a separate step. Should I open an issue? (We also discussed the possibilities in Discord)
https://discord.com/channels/608014603317936148/1186378330178601000

… revert_137115

Gustl22 · 2024-01-27T04:04:55Z

Thank you for ya help. I am currently moving so I have time on Monday again.

Would it be a valid approach after reverting my PR (with this PR) to revert the revert and add the fix and tests on top of it(?)
Then I expect, the changes of @bartekpacia should be the same as before.

camsim99 · 2024-01-29T17:18:27Z

Would it be a valid approach after reverting my PR (with this PR) to revert the revert and add the fix and tests on top of it(?) Then I expect, the changes of @bartekpacia should be the same as before.

This makes sense to me!

camsim99 · 2024-01-29T19:28:46Z

I was able to verify this revert does fix the issue. Going to close this in favor of #142464 to make this PR easier to revert since I made some choices here without considering the beta branch cutoff as I should have as noted above.

@Gustl22

…142464) This reverts commit f5ac225, i.e. #137115. This is a continuation of #142266 that was redone based on feedback to make this easier to revert in the future. The exact steps I took to create this revert: 1. Revert commit noted above 2. Fix merge conflicts, that notably involved reverting some changes in #140744 ~and #141417 (fixed my merge to avoid the second PR from being affected) 3. Delete `packages/flutter_tools/test/integration.shard/android_plugin_skip_unsupported_test.dart` as this was added in the commit noted above cc @Gustl22 since I couldn't tag as a reviewer

@Gustl22

)" (flutter#142464) This reverts commit f5ac225, i.e. flutter#137115. This is a continuation of flutter#142266 that was redone based on feedback to make this easier to revert in the future. The exact steps I took to create this revert: 1. Revert commit noted above 2. Fix merge conflicts, that notably involved reverting some changes in flutter#140744 ~and flutter#141417 (fixed my merge to avoid the second PR from being affected) 3. Delete `packages/flutter_tools/test/integration.shard/android_plugin_skip_unsupported_test.dart` as this was added in the commit noted above cc @Gustl22 since I couldn't tag as a reviewer

Revert "Reland: "Fix how Gradle resolves Android plugin" (flutter#137115

7d5d68e

)" This reverts commit f5ac225.

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

camsim99 added 2 commits January 25, 2024 14:34

Merge master + left todo for merge

e3c64da

remove todo since flutter#140744 changes in flutter.groovy seem only …

0f708e0

…relevant to legacy plugins

camsim99 requested a review from bartekpacia January 25, 2024 22:47

add back test data removed in merge

1d2c273

camsim99 mentioned this pull request Jan 25, 2024

error: package io.flutter.plugins.* does not exist when running app on Flutter beta/master channel #141940

Closed

bartekpacia reviewed Jan 25, 2024

View reviewed changes

Adds back newer doesSupportAndroidPlatform

b55d35b

Merge branch 'master' into revert_137115

f03cee1

bartekpacia reviewed Jan 26, 2024

View reviewed changes

camsim99 added 5 commits January 26, 2024 08:53

redo merge

1287ea8

Merge remote-tracking branch 'refs/remotes/origin/revert_137115' into…

0531151

… revert_137115

Remove tests for removed functionality

0d36346

Remove unused import

164a141

Merge remote-tracking branch 'upstream/master' into revert_137115

6e55c52

reidbaker self-requested a review January 29, 2024 16:13

Use old doesSupportAndroidPlatform for cp

5b479c1

Merge remote-tracking branch 'upstream/master' into revert_137115

91abf8a

camsim99 mentioned this pull request Jan 29, 2024

Revert "Reland: "Fix how Gradle resolves Android plugin" (#137115)" #142464

Merged

8 tasks

bartekpacia mentioned this pull request Jan 29, 2024

Migrate examples/hello_world Android app to Gradle Kotlin DSL #142300

Closed

camsim99 closed this Jan 29, 2024

bartekpacia mentioned this pull request Feb 9, 2024

Migrate integration_test plugin to Gradle Kotlin DSL #142008

Merged

Revert "Reland: "Fix how Gradle resolves Android plugin" (#137115)" #142266

Revert "Reland: "Fix how Gradle resolves Android plugin" (#137115)" #142266

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

camsim99 commented Jan 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jan 26, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camsim99 commented Jan 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

bartekpacia commented Jan 26, 2024

Uh oh!

bartekpacia Jan 26, 2024 • 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!

Gustl22 Feb 2, 2024 • 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!

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

Uh oh!

Uh oh!

camsim99 commented Jan 29, 2024

Uh oh!

camsim99 commented Jan 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

camsim99 commented Jan 25, 2024 •

edited

Loading

bartekpacia commented Jan 25, 2024 •

edited

Loading

stuartmorgan-g Jan 26, 2024 •

edited

Loading

Gustl22 commented Jan 26, 2024 •

edited

Loading

bartekpacia Jan 26, 2024 •

edited

Loading

Gustl22 Feb 2, 2024 •

edited

Loading

Gustl22 commented Jan 27, 2024 •

edited

Loading