Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland: "Fix how Gradle resolves Android plugin" #137115

Merged

Conversation

Gustl22
Copy link
Contributor

@Gustl22 Gustl22 commented Oct 24, 2023

Relands #97823

When the tool migrated to .flutter-plugins-dependencies, the Gradle plugin was never changed.
Until now, the plugin had the heuristic that a plugin with a android/build.gradle file supported the Android platform.

Also applies schema of getPluginDependencies to getPluginList which uses a List of Object instead of Properties.

Fixes #97729
Cause of the error: https://github.com/flutter/flutter/blob/5f105a6ca7a5ac7b8bc9b241f4c2d86f4188cf5c/packages/flutter_tools/gradle/flutter.gradle#L421C25-L421C25

Fixes #98048
The deprecated line include ":$name" in settings.gradle (pluginEach) in old projects causes the project.rootProject.findProject to also find the plugin "project", so it is not failing on the afterEvaluate method. But the plugin shouldn't be included in the first place as it fails with Could not find method implementation() for arguments error in special cases.

Related to #48918, see _writeFlutterPluginsListLegacy.

Co-authored-by: Emmanuel Garcia [email protected]

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.

@google-cla
Copy link

google-cla bot commented Oct 24, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 24, 2023
@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 24, 2023

Regarding the missing CLA: I wanted to honor Emmanuel Garcia (blasten) in 965397c as he did the initial MR. Don't know how to circumvent this, without removing the contribution email.

See also: #129095 (comment)

@stuartmorgan
Copy link
Contributor

Honestly I don't know how to reproduce the issue described in #98048, which was the reason why #97823 was reverted.

Based on the discussion there it involved having old versions of Gradle and/or AGP. The legacy app packages test might be helpful in trying to reproduce it.

So it may be fixed, comparing the old and the new settings.gradle.

Template updates can't fix a problem that is caused by the values that are in old templates, because once a project is instantiated it is generally not re-created. If thousands of projects (or more) in the wild still have a version that's incompatible with the change, then the change can't land without some kind of mitigation (e.g., a clear error message with instructions on updating).

Don't know how it's handled by the migration tool though.

The migration tool was not completed, and is not currently in development.

@stuartmorgan
Copy link
Contributor

Regarding the missing CLA: I wanted to honor Emmanuel Garcia (blasten) in 965397c as he did the initial MR. Don't know how to circumvent this, without removing the contribution email.

It is far easier to remove the email from the authorship and just reference it in the PR than it is to bypass the CLA check (which requires one of a handful of people to go through a manual process).

@Gustl22 Gustl22 force-pushed the 48918-flutter-plugins-dependencies-migration branch from 3981576 to 60f9b84 Compare October 25, 2023 15:01
@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 25, 2023

Indeed I found the problem:
The mentioned settings.gradle is even older than the one used for testing legacy projects, see:

include ':app'

def flutterProjectRoot = rootProject.projectDir.parentFile.toPath()

def plugins = new Properties()
def pluginsFile = new File(flutterProjectRoot.toFile(), '.flutter-plugins')
if (pluginsFile.exists()) {
    pluginsFile.withReader('UTF-8') { reader -> plugins.load(reader) }
}

plugins.each { name, path ->
    def pluginDirectory = flutterProjectRoot.resolve(path).resolve('android').toFile()
    include ":$name"
    project(":$name").projectDir = pluginDirectory
}

And now the problem is more clear: Now plugins are also included from '.flutter-plugins', which do not have a android/build.gradle file, which were sorted out with the doesSupportAndroidPlatform method.

We could either:

  • Drop support for this old version of settings.gradle file, as it isn't even used for testing against legacy project.
    • Write some warning, to upgrade the settings.gradle file (?)
  • Just keep the check with doesSupportAndroidPlatform in the flutter.groovy file, which works also quite well, without compromises, but
    • Open a new issue, to describe the problem and link it in the code, also deprecate the doesSupportAndroidPlatform method.
    • Or even write a test to check this old settings.gradle file in a separate PR (e.g. for legacy projects), and if so, need this be done before this one?

@Gustl22 Gustl22 force-pushed the 48918-flutter-plugins-dependencies-migration branch from 60f9b84 to c2603ce Compare October 25, 2023 19:22
@stuartmorgan
Copy link
Contributor

Drop support for this old version of settings.gradle file, [...]

  • Write some warning, to upgrade the settings.gradle file (?)

Anything that would drop support for old projects needs at least a warning, if not an automigration. We don't want people with old projects to suddenly be unable to compile without any idea why.

as it isn't even used for testing against legacy project.

The test I linked to is a specific legacy test based on the constraints of what was relatively straightforward to set up in the flutter/packages CI; it's not comprehensive or authoritative. We should not knowingly break people just because we haven't written a test that would fail in that scenario.

@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 26, 2023

I now also replaced dependencyGraph with plugins as stated here.

And there were some tests regarding the legacy settings.gradle (#35217), but they were removed in #86911.

@Gustl22 Gustl22 force-pushed the 48918-flutter-plugins-dependencies-migration branch from 2cbe6b8 to d1148c3 Compare October 26, 2023 11:49
@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 26, 2023

I added a test for the legacy settings.gradle but I think the other stuff such as gradle and agp version aren't compatible as it gives an error before actually running the code in settings.gradle.

Build file 'C:\Users\Gustl\AppData\Local\Temp\flutter_flutter_plugin_test.6be279f\test_plugin\example\android\app\build.gradle' line: 2
Plugin [id: 'com.android.application'] was not found in any of the following sources:

- Gradle Core Plugins (plugin is not in 'org.gradle' namespace)
- Plugin Repositories (plugin dependency must include a version number for this source)

Can I somehow use the legacy script flutter/packages/.ci/scripts/build_all_packages_app_legacy.sh ? Or should I add a test in the flutter/packages repository? Can you help me out on this one? I don't know what else to do... Thank you!

@stuartmorgan
Copy link
Contributor

Can I somehow use the legacy script flutter/packages/.ci/scripts/build_all_packages_app_legacy.sh ? Or should I add a test in the flutter/packages repository?

flutter/packages tooling is not applicable in this repo, and any tests for changes here should be in this repo. I was just suggesting looking at that test as way to potentially reproduce the problem manually, as a starting point for figuring out how to reproduce it, and thus design a test for it.

@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 26, 2023

I noticed the generate_gradle_lockfile tool was introduced, which changes the files I'm writing a test about. And every other test also uses the generated files since it was introduced. Even the gradle tests which are supposed to validate this old settings.gradle was updated (I think by accident) with #83067. I think these files testing old gradle stuff must be reverted then (?). And there should be added a possibility to exclude single files from the generate_gradle_lockfile tool or check these files don't change in an extra test (?)

@Gustl22
Copy link
Contributor Author

Gustl22 commented Oct 27, 2023

This should now build successfully, but this does not help with #137040 at all, as it then loads all plugins as before and one cannot decide which plugin to load as long the plugins are imported using dependencyGraph from the .flutter-plugins-dependencies file. So settings.gradle needs to be migrated non the less (#54566), but only for such a case as #97729 (the other cases don't raise the problem). More explanation is here.

And what about the migration scripts here, can't we use them, to update non changed deprecated settings.gradle files?

Edit: I try now another solution, by explicitly excluding the packages which are not wanted, which can be easily determined.
Edit2: Exclusion after inclusion of plugins is not possible, but I now only use the old behavior (by adding non supported android plugins as API), if settings.gradle doesn't reference the .flutter-plugins file.

@Gustl22 Gustl22 force-pushed the 48918-flutter-plugins-dependencies-migration branch from 6808f3d to 7dd1f7a Compare October 27, 2023 12:45
@Gustl22 Gustl22 marked this pull request as draft October 28, 2023 21:59
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Gustl22 Gustl22 force-pushed the 48918-flutter-plugins-dependencies-migration branch from 49b109a to 060a07e Compare October 29, 2023 22:04
@Gustl22 Gustl22 marked this pull request as ready for review October 29, 2023 22:05
@Gustl22
Copy link
Contributor Author

Gustl22 commented Nov 6, 2023

@stuartmorgan kindly asking for a review :)

@stuartmorgan stuartmorgan added the team-android Owned by Android platform team label Nov 6, 2023
@stuartmorgan
Copy link
Contributor

The primary review here should be from someone on the Android team with more Gradle expertise.

Copy link
Contributor

@reidbaker reidbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming nits I admit are probably the most annoying.
From #121552 I think we should call the current system something like gradle plugin dsl or declarative plugins. The system we had immediately before could be called 'ApplySource' but I am open to more descriptive names than legacy. From you back and forth with @stuartmorgan in this PR I also gather we had a previous method of application which I think is best described in #54566 for lack of a better name maybe we call that version 'PluginEach'. Then define what it means with an inline comment.

@camsim99
Copy link
Contributor

Reverting due to #141940.

Copy link
Contributor

auto-submit bot commented Jan 25, 2024

Time to revert pull request flutter/flutter/137115 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Jan 25, 2024
camsim99 added a commit to camsim99/flutter that referenced this pull request Jan 25, 2024
camsim99 added a commit to camsim99/flutter that referenced this pull request Jan 29, 2024
auto-submit bot pushed a commit that referenced this pull request Jan 29, 2024
…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
camsim99 added a commit to camsim99/flutter that referenced this pull request Jan 29, 2024
)" (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
auto-submit bot pushed a commit that referenced this pull request Jan 31, 2024
#137115)" (#142491)

Cherry-pick for #137115 to fix #141940.  Steps taken to create this cherry-pick:

1. Cherry pick commit 995e3fa and fix merge conflicts
2. Delete the following code in `flutter.groovy` that I accidentally kept in the merge but was added in #137115
```groovy
// Load shared gradle functions
project.apply from: Paths.get(flutterRoot.absolutePath, "packages", "flutter_tools", "gradle", "src", "main", "groovy", "native_plugin_loader.groovy")
```
3. Deleted `android_plugin_skip_unsupported_test.dart` that was left incorrectly by merge.
Gustl22 added a commit to Gustl22/flutter that referenced this pull request Feb 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
auto-submit bot pushed a commit that referenced this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App team-android Owned by Android platform team tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
8 participants