Add dartPluginClass support for Android and iOS #87991

stuartmorgan-g · 2021-08-10T14:48:48Z

Expands the existing desktop dartPluginClass support, with automatic Dart registration, to Android and iOS.

Since we do not have existing dartPluginClass implementations on mobile, this addresses from the outset the issue described in #87862, so that inline implementations mobile implementations will work as well. This does not attempt to change that behavior for desktop, since that has breaking implications, so will be handled separately.

Fixes #52267

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 feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

blasten · 2021-08-11T19:37:26Z

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart

-           targetPlatform != TargetPlatform.linux_arm64 &&
-           targetPlatform != TargetPlatform.windows_x64 &&
-           targetPlatform != TargetPlatform.windows_uwp_x64;
+    final TargetPlatform? targetPlatform = platformName == null ? null


nit: what about:

if (platformName == null) { return false; }

That's what was there before, and has the effect that if a platform isn't passed it silently does nothing. I'd rather we only opt out for platforms we're sure we don't want to do this, since as a failure mode, failing to do registration silently is much worse that generating a registrant that shouldn't do anything.

packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart

packages/flutter_tools/test/general.shard/dart_plugin_test.dart

blasten · 2021-08-11T20:27:26Z

packages/flutter_tools/lib/src/flutter_plugins.dart

+        final bool hasInlineDartImplementation =
+          plugin.pluginDartClassPlatforms[platform] != null;
+        if (defaultImplementation == null && !hasInlineDartImplementation) {
          if (throwOnPluginPubspecError) {


fyi: throwOnPluginPubspecError is false due to backward compatibilities.

flutter/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart

Line 58 in 4c6a9dc

throwOnPluginPubspecError: false,

Should it still print the error, but not exit with code?

Since the goal is to eliminate this error in a follow-up PR, I think we can leave it as-is for now.

blasten

Looks pretty good. Just some minor comments

stuartmorgan-g · 2021-08-17T15:45:33Z

@blasten How is this intended to interact with flutter test? The reason CI is failing is that there's an end-to-end test that is making an app that includes path_provider (which means it will have a generated Dart registrant), building it for Android, and then running flutter test; that test is failing now. Experimenting locally, what seems to be happening is that if generated_main.dart exists, then flutter test is running its 'main' instead of the widget_test.dart's 'main', and generated_main.dart points directly to main.dart, so the test's main is never run.

If I delete generated_main.dart after building for Android and before running flutter test everything works, but presumably "delete that file at the beginning of flutter test" is not the right fix. Is something supposed to be restructuring the chain of mains here? Or was this a bug with the desktop version that just never got caught?

I'm not sure how to correctly wire this up so that we're calling all of the relevant mains in a test scenario.

blasten · 2021-08-17T18:19:11Z

That is definitely problematic. We would detect if a test target is currently used, and set checkDartPluginRegistry: false in

flutter/packages/flutter_tools/lib/src/build_system/targets/common.dart

Line 226 in 00263c4

checkDartPluginRegistry: environment.generateDartPluginRegistry,

blasten · 2021-08-17T18:20:08Z

Maybe, it can be safely inferred from the target file? If not, we would need to pass that information to the build system.

flutter-dashboard · 2021-09-10T11:38:20Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

stuartmorgan-g · 2021-10-01T15:13:09Z

With #90288 landed, it looks like the test issues are all resolved. Just waiting on Google test

This reverts commit 23cea26.

generateDartPluginRegistry was being set to true unconditionally in ResidentRunner, bypassing the primary check in DartPluginRegistrantTarget, and the targetPlatform was not set in that codepath, bypassing the second after the changes in flutter#87991. This caused web hot restarts to be slower due to doing unnecessary work. This ensures that generateDartPluginRegistry is false in the ResidentWebRunner to skip that unnecessary step. Fixes flutter#91262

* Don't generate plugin registry in ResidentWebRunner generateDartPluginRegistry was being set to true unconditionally in ResidentRunner, bypassing the primary check in DartPluginRegistrantTarget, and the targetPlatform was not set in that codepath, bypassing the second after the changes in #87991. This caused web hot restarts to be slower due to doing unnecessary work. This ensures that generateDartPluginRegistry is false in the ResidentWebRunner to skip that unnecessary step. Fixes #91262 * Formatting Co-authored-by: Zachary Anderson <[email protected]> Co-authored-by: Zachary Anderson <[email protected]>

* Don't generate plugin registry in ResidentWebRunner generateDartPluginRegistry was being set to true unconditionally in ResidentRunner, bypassing the primary check in DartPluginRegistrantTarget, and the targetPlatform was not set in that codepath, bypassing the second after the changes in flutter#87991. This caused web hot restarts to be slower due to doing unnecessary work. This ensures that generateDartPluginRegistry is false in the ResidentWebRunner to skip that unnecessary step. Fixes flutter#91262 * Formatting Co-authored-by: Zachary Anderson <[email protected]> Co-authored-by: Zachary Anderson <[email protected]>

These changes allow to override existing native endorsed (federated & inline) plugins with e.g. non-endorsed plugin implementations via direct dependencies as described in the section *Platform implementation selection* in the [design doc](https://docs.google.com/document/d/1LD7QjmzJZLCopUrFAAE98wOUQpjmguyGTN2wd_89Srs). ```yaml # pubspec.yaml name: example dependencies: my_plugin: ^0.0.1 my_plugin_android_alternative: ^0.0.1 ``` Closes #80374, closes #59657 Related: Override Dart plugins (#87991, #79669)

stuartmorgan-g added 3 commits August 10, 2021 10:47

Implement dartPluginClass support for mobile

ef03d95

Re-add desktop requirement for implements

d1f351a

Remove cruft

242879b

stuartmorgan-g requested a review from blasten August 10, 2021 14:48

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 10, 2021

google-cla bot added the cla: yes label Aug 10, 2021

stuartmorgan-g changed the title ~~Add dartPluginClass support for i~~ Add dartPluginClass support for Android and iOS Aug 10, 2021

blasten reviewed Aug 11, 2021

View reviewed changes

packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart Show resolved Hide resolved

blasten reviewed Aug 11, 2021

View reviewed changes

packages/flutter_tools/test/general.shard/dart_plugin_test.dart Outdated Show resolved Hide resolved

blasten reviewed Aug 11, 2021

View reviewed changes

packages/flutter_tools/test/general.shard/dart_plugin_test.dart Outdated Show resolved Hide resolved

blasten reviewed Aug 11, 2021

View reviewed changes

blasten approved these changes Aug 11, 2021

View reviewed changes

stuartmorgan-g added 4 commits August 15, 2021 13:32

Address review comments

8627838

Merge branch 'master' into dart-plugin-class-mobile

f52d80a

Fix unit test expectations

687982c

Merge branch 'master' into dart-plugin-class-mobile

011d0f0

stuartmorgan-g mentioned this pull request Aug 21, 2021

Flutter test not working trying to run the application entrypoint, instead of those of test fixtures #88632

Closed

stuartmorgan-g mentioned this pull request Sep 9, 2021

Mac_staging plugin_test_ios keeps failing in staging #88794

Closed

stuartmorgan-g mentioned this pull request Sep 17, 2021

Implement dartPluginClass support for plugins #52267

Closed

stuartmorgan-g mentioned this pull request Sep 29, 2021

flutter run does not use its given target #90920

Closed

Merge branch 'flutter:master' into dart-plugin-class-mobile

afa06d9

stuartmorgan-g added the waiting for tree to go green label Oct 1, 2021

fluttergithubbot merged commit 23cea26 into flutter:master Oct 1, 2021

zanderso mentioned this pull request Oct 4, 2021

Regression in restart time benchmarks #91262

Closed

christopherfujino added a commit that referenced this pull request Oct 4, 2021

Revert "Add dartPluginClass support for Android and iOS (#87991)"

e7368b2

This reverts commit 23cea26.

stuartmorgan-g mentioned this pull request Oct 5, 2021

Don't generate plugin registry in ResidentWebRunner #91281

Merged

8 tasks

mateusfccp mentioned this pull request Oct 8, 2021

Implement the federated plugin interface correctly mateusfccp/update_available#7

Closed

blasten mentioned this pull request Oct 14, 2021

Dart plugin registrant broke top-level Dart entrypoints #91832

Closed

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021

Add dartPluginClass support for Android and iOS (flutter#87991)

41d9e22

Gustl22 mentioned this pull request Oct 22, 2023

feat: Support overriding native endorsed plugins #137040

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add dartPluginClass support for Android and iOS #87991

Add dartPluginClass support for Android and iOS #87991

Uh oh!

stuartmorgan-g commented Aug 10, 2021 •

edited

Loading

Uh oh!

blasten Aug 11, 2021

Uh oh!

stuartmorgan-g Aug 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blasten Aug 11, 2021

Uh oh!

stuartmorgan-g Aug 15, 2021

Uh oh!

blasten left a comment

Uh oh!

stuartmorgan-g commented Aug 17, 2021 •

edited

Loading

Uh oh!

blasten commented Aug 17, 2021

Uh oh!

blasten commented Aug 17, 2021

Uh oh!

flutter-dashboard bot commented Sep 10, 2021

Uh oh!

stuartmorgan-g commented Oct 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add dartPluginClass support for Android and iOS #87991

Add dartPluginClass support for Android and iOS #87991

Uh oh!

Conversation

stuartmorgan-g commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

blasten Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blasten Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 15, 2021

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blasten commented Aug 17, 2021

Uh oh!

blasten commented Aug 17, 2021

Uh oh!

flutter-dashboard bot commented Sep 10, 2021

Uh oh!

stuartmorgan-g commented Oct 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stuartmorgan-g commented Aug 10, 2021 •

edited

Loading

stuartmorgan-g commented Aug 17, 2021 •

edited

Loading