Implement dartPluginClass support for plugins #74469

blasten · 2021-01-22T06:14:01Z

Tool side of #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 listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

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

blasten · 2021-01-22T06:15:52Z

@jonahwilliams what do you think of this approach? It supports hot-reload, etc... If it makes sense, I will still need to get the metadata from pubspec.yaml, which I don't think it's accessible at this point.

jonahwilliams · 2021-01-22T17:13:41Z

packages/flutter_tools/lib/src/compile.dart

+  print('_registerPlugins is called');
+}
+
+void main() {


If you're going to generate a synthetic entrypoint, then I don't think you need the pragma or anything else special - just invoke _registerPlugins() before calling entrypoint.main.

main can be overridden by the embedding at runtime.

right :) . lots of moving parts here.

jonahwilliams · 2021-01-22T17:23:27Z

IIRC this won't work as is without more changes in the resident_runner or the hot reload pipeline. The development compiler needs to be given the same entrypoint to compile as the app was built with - otherwise I don't believe hot reloads will work until a hot restart - and in this case that hot restart would blow away the plugin callback.

This will also require some special casing for g3, since they'll likely generate that file in a different way.

jonahwilliams

Still looking at actual implementation, first comment is not to introduce more globals usage

packages/flutter_tools/lib/src/compile.dart

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

jonahwilliams

General feedback: it is difficult to do a thorough review without tests, as I'm not quite sure what all of the expected changes are

jmagman · 2021-02-18T01:04:41Z

Re: test failures

I added url_launcher: and url_launcher_macos: to a project pubspec. Before this change, .flutter-plugins-dependencies looks like:

  "plugins": {
    "ios": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "android": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "macos": [
      {
        "name": "url_launcher_macos",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "linux": [
      {
        "name": "url_launcher_linux",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "windows": [
      {
        "name": "url_launcher_windows",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "web": [
      {
        "name": "url_launcher_web",
        "path": "{path}",
        "dependencies": []
      }
    ]
  },

On this PR url_launcher is listed as a plugin for Windows, Linux, and macOS, with the platform-specific variant as a dependency.

  "plugins": {
    "ios": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "android": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "macos": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": [
          "url_launcher_macos"
        ]
      },
      {
        "name": "url_launcher_macos",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "linux": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": [
          "url_launcher_linux"
        ]
      },
      {
        "name": "url_launcher_linux",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "windows": [
      {
        "name": "url_launcher",
        "path": "{path}",
        "dependencies": [
          "url_launcher_windows"
        ]
      },
      {
        "name": "url_launcher_windows",
        "path": "{path}",
        "dependencies": []
      }
    ],
    "web": [
      {
        "name": "url_launcher_web",
        "path": "{path}",
        "dependencies": []
      }
    ]
  },

If that's the intended behavior of this PR, those tests should be updated.

blasten · 2021-02-18T01:11:36Z

Thanks @jmagman. That seems correct based on my changes. (which I had to review again :))

jmagman · 2021-02-18T01:26:04Z

dev/devicelab/bin/tasks/plugin_lint_mac.dart

      || !podfileLockOutput.contains(':path: Flutter/ephemeral/.symlinks/plugins/url_launcher_macos/macos')
      || !podfileLockOutput.contains(':path: Flutter/ephemeral/.symlinks/plugins/test_plugin_swift/macos')
-      || podfileLockOutput.contains('url_launcher/')) {
+      || !podfileLockOutput.contains('url_launcher/')) {


The spirit of this check was to make sure the url_launcher framework was not embedded in the app. It was using the Podfile.lock as a shortcut for this. A more appropriate test now would be to validate that the generated app contains Contents/Frameworks/url_launcher_macos.framework but not Contents/Frameworks/url_launcher.framework.

got it. I added another check to the test for the time being. Do you have any suggestion about how to verify the frameworks?

Something like

final String appDirectory = path.join( appPath, 'build', 'macos', 'Build', 'Products', 'Release', '${path.basename(appPath)}.app', ); final String frameworksDirectory = path.join( appDirectory, 'Contents', 'Frameworks', ); checkDirectoryExists(path.join( frameworksDirectory, 'url_launcher_macos.framework', )); checkDirectoryNotExists(path.join( frameworksDirectory, 'url_launcher.framework', ));

There also may be a more appropriate test location for that check, like https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/integration.shard/macos_content_validation_test.dart

fluttergithubbot · 2021-02-18T04:26:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.

… infinite recursion in analyzing packages

This reverts commit b7d4806.

This reverts commit b7d4806. Kick.

* Revert "Enable dart_plugin_registry_test (#76645)" This reverts commit 109e0bb. * Revert "Apply changes caused by #76662 (#77093)" This reverts commit cdca648. * Revert "Disable clang format in the plugin registrants (#76662)" This reverts commit dadbd47. * Revert "Disable warnings for the dart plugin registrant (#76561)" This reverts commit 098ece5. * Revert "Remove dart_plugin_registry_test timeouts (#76838)" This reverts commit 1610a27. * Revert "Implement dartPluginClass support for plugins (#74469)" This reverts commit b7d4806. Kick.

…r#74469" (flutter#78623)" This reverts commit 5efc716.

Prototype code in the tool

453c320

blasten requested a review from jonahwilliams January 22, 2021 06:14

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

google-cla bot added the cla: yes label Jan 22, 2021

clean up

9a601d5

jonahwilliams reviewed Jan 22, 2021

View reviewed changes

Emmanuel Garcia added 16 commits January 22, 2021 14:10

Handle hot restart

8cc052f

Resolve plugin interfaces

050f9fc

Wrong comment

baa3354

Format

c11c3aa

final

54d0ba2

Add throwOnPluginPubspecError flag

a0aa899

Make isDirectDependency optional

2726833

Check if dependencies is null

4be41ef

Merge branch 'master' into tool_dart_plugin_registrant

e7ebe2b

Provide default package

8788f03

null check

66484cc

Check pubspec.yaml exist

c2806d0

Refactor

b9bc779

Fix tests

70a9504

Fix assert

5b22b61

Compilers reuse same entrypoint

dc35173

blasten requested a review from jonahwilliams January 25, 2021 19:05

jonahwilliams reviewed Jan 25, 2021

View reviewed changes

packages/flutter_tools/lib/src/compile.dart Outdated Show resolved Hide resolved

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

jonahwilliams reviewed Jan 25, 2021

View reviewed changes

Emmanuel Garcia added 2 commits January 25, 2021 13:02

Remove global and context usage

b8eb0e6

Tests

26d3590

blasten marked this pull request as ready for review January 26, 2021 01:59

jmagman mentioned this pull request Feb 17, 2021

Output unexpected Podfile.lock contents on test failure #76223

Merged

blasten force-pushed the tool_dart_plugin_registrant branch from bcea090 to 892f721 Compare February 17, 2021 18:38

Emmanuel Garcia added 2 commits February 17, 2021 17:15

Update test

a6ba1a6

Merge branch 'master' into tool_dart_plugin_registrant

171a171

jmagman reviewed Feb 18, 2021

View reviewed changes

Check specific import

1b354f6

blasten added the waiting for tree to go green label Feb 18, 2021

fluttergithubbot removed the waiting for tree to go green label Feb 18, 2021

Emmanuel Garcia added 4 commits February 18, 2021 13:30

Merge branch 'master' into tool_dart_plugin_registrant

c672fc3

Rework what it means to support a platform as current approach caused…

8ad1ce1

… infinite recursion in analyzing packages

Revert changes to plugin_lint_mac

0fc39a4

Update tests

28a3092

blasten merged commit b7d4806 into flutter:master Feb 19, 2021

blasten deleted the tool_dart_plugin_registrant branch February 19, 2021 17:22

This was referenced Feb 19, 2021

Building with a macOS plugin: "Plugin url_launcher_linux doesn't implement a plugin interface" #76396

Closed

Mac dart_plugin_registry_test flaky, times out during "Wait for registry execution" #76837

Closed

ekasetiawans mentioned this pull request Mar 9, 2021

Android. NoSuchMethodError (NoSuchMethodError: No top-level getter '_registerPlugins' declared. #77680

Closed

jonahwilliams mentioned this pull request Mar 18, 2021

Disable automatic plugin registration for the next beta release #78554

Closed

zanderso added a commit that referenced this pull request Mar 19, 2021

Revert "Implement dartPluginClass support for plugins (#74469)"

c7380fa

This reverts commit b7d4806.

zanderso added a commit that referenced this pull request Mar 20, 2021

Revert "Implement dartPluginClass support for plugins (#74469)"

eb81840

This reverts commit b7d4806.

zanderso added a commit that referenced this pull request Mar 20, 2021

Revert "Implement dartPluginClass support for plugins (#74469)"

95c2115

This reverts commit b7d4806. Kick.

zanderso added a commit that referenced this pull request Mar 23, 2021

Revert "Implement dartPluginClass support for plugins (#74469)"

1627ce5

This reverts commit b7d4806. Kick.

zanderso added a commit that referenced this pull request Mar 23, 2021

Revert "Implement dartPluginClass support for plugins (#74469)"

be4427a

This reverts commit b7d4806. Kick.

blasten pushed a commit to blasten/flutter that referenced this pull request Apr 2, 2021

Revert "Reverts "Implement dartPluginClass support for plugins flutte…

a5a7938

…r#74469" (flutter#78623)" This reverts commit 5efc716.

blasten mentioned this pull request Apr 3, 2021

Reland the Dart plugin registry #79669

Merged

Implement dartPluginClass support for plugins #74469

Implement dartPluginClass support for plugins #74469

Uh oh!

Conversation

blasten commented Jan 22, 2021

Pre-launch Checklist

Uh oh!

blasten commented Jan 22, 2021

Uh oh!

jonahwilliams Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

blasten Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jan 22, 2021

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blasten commented Feb 18, 2021

Uh oh!

jmagman Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

blasten Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Feb 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jmagman commented Feb 18, 2021 •

edited

Loading