-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Reland: "Fix how Gradle resolves Android plugin" #137115
Conversation
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. |
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) |
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.
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).
The migration tool was not completed, and is not currently in development. |
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). |
3981576
to
60f9b84
Compare
Indeed I found the problem:
And now the problem is more clear: Now plugins are also included from We could either:
|
60f9b84
to
c2603ce
Compare
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.
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. |
7aa2178
to
2cbe6b8
Compare
2cbe6b8
to
d1148c3
Compare
I added a test for the legacy
Can I somehow use the legacy script |
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. |
I noticed the |
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 And what about the migration scripts here, can't we use them, to update non changed deprecated Edit: I try now another solution, by explicitly excluding the packages which are not wanted, which can be easily determined. |
6808f3d
to
7dd1f7a
Compare
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
49b109a
to
060a07e
Compare
@stuartmorgan kindly asking for a review :) |
The primary review here should be from someone on the Android team with more Gradle expertise. |
There was a problem hiding this 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.
packages/flutter_tools/test/integration.shard/android_plugin_skip_unsupported_test.dart
Show resolved
Hide resolved
packages/flutter_tools/test/integration.shard/android_plugin_skip_unsupported_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/integration.shard/deprecated_gradle_settings_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/integration.shard/deprecated_gradle_settings_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/integration.shard/test_data/legacy_settings_gradle_project.dart
Outdated
Show resolved
Hide resolved
Reverting due to #141940. |
Time to revert pull request flutter/flutter/137115 has elapsed. |
)" This reverts commit f5ac225.
)" This reverts commit f5ac225.
…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
)" (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
#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.
…tter#137115)" (flutter#142464)" This reverts commit 995e3fa.
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
togetPluginList
which uses aList
of Object instead ofProperties
.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"
insettings.gradle
(pluginEach) in old projects causes theproject.rootProject.findProject
to also find the plugin "project", so it is not failing on theafterEvaluate
method. But the plugin shouldn't be included in the first place as it fails withCould 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
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.