[flutter_tools] Removes the need of a no-op plugin implementations #48614

franciscojma86 · 2020-01-11T00:31:05Z

Description

As mentioned in #46618, macOS plugins require an iOS no-op implementation and vice versa. This is due to some limitations in the generated .flutter-plugins file.

Updates .flutter-plugin-dependencies to include the plugins list and version information. Supports all platforms. Example:

{
  "info": "This is a generated file; do not edit or check into version control.",
  "plugins": {
    "ios": [
      {
        "name": "e2e",
        "path": "/path/to/plugin",
      },
      {
        "name": "shared_preferences",
        "path": "/path/to/plugin",
      }
    ],
    "macos": [
      {
        "name": "shared_preferences_macos",
        "path": "/path/to/plugin",
      }
    ],
    "web": [
      {
        "name": "shared_preferences_web",
        "path": "/path/to/plugin",
      }
    ]
  }
}

This PR doesn't include changes to the Podfile and gradle templates, since there are still some issues to consider regarding the migration strategy and CI changes.

Related Issues

#46618

Tests

I added the following tests:

Updated tests in plugins-test.dart and plugin-dependencies-test.dart. Still supports tests for current logic.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

No, no existing tests failed, so this is not a breaking change.

This reverts commit 50d4212.

franciscojma86 · 2020-01-13T23:50:22Z

/cc @amirh

jonahwilliams · 2020-01-13T23:52:54Z

packages/flutter_tools/lib/src/project.dart


 import 'dart:async';

+import 'package:flutter_tools/src/platform_plugins.dart';


This should be a relative import

packages/flutter_tools/lib/src/project.dart

jonahwilliams · 2020-01-14T00:04:34Z

packages/flutter_tools/lib/src/plugins.dart

+/// If there aren't any plugins, then the files aren't written to disk. The resulting
+/// file looks something like this:
+/// {
+///     "_info": "// This is a generated file; do not edit or check into version control.",


I would also recommend embedding some sort of version string in the JSON, just in case

Also possibly the date it was generated on/ the version of the tool used to generate it

See .dart_tool/package_config.json:

"generated": "2020-01-13T22:42:48.743606Z", "generator": "pub", "generatorVersion": "2.8.0-dev.0.0.flutter-395daaa3ec"

blasten · 2020-01-14T00:10:48Z

packages/flutter_tools/lib/src/plugins.dart

+  }
+}
+
+/// Writes the .flutter-plugins.json file based on the list of plugins.


.flutter-plugins-dependencies since new projects on stable have this file in the .gitignore already.

But that file means something different; wouldn't changing the content break a bunch of projects?

I changed it to .flutter-plugins-dependencies, and won't break since I still add the dependencyGraph to support current projects. Once it's fully deprecated we can just remove it from the json

stuartmorgan-g · 2020-01-14T02:45:42Z

Was there discussion somewhere about using a single file that I missed while out? The plan I had proposed was to make a file per platform, in the platform directories, which seemed like it would be simpler for each platform to parse, as well as sticking to the current structure of platform-specific information generally being within platform directories. I'm curious what the pro/con evaluation of that vs. the approach here was.

jonahwilliams · 2020-01-14T02:50:28Z

Was there discussion somewhere about using a single file that I missed while out?

I think we talked briefly about this before break but I've lost context on the discussion.

blasten · 2020-01-14T03:14:47Z

I don’t think I was involved in the discussion about deprecating .flutter-plugins.

Unfortunately, in some situations, Android and iOS have project files that depend on the location and format of this file. I’m looking into addressing this specific problem (managing project files) in a separate effort.

Maybe the rest of the platform can have their own file while iOS/Android continue to use .flutter-plugins until we migrate the affected projects.

jonahwilliams · 2020-01-14T03:18:02Z

Actually, I remember now. I think the context was concerns about how updates to the platform directories (if any) would effect this file. @franciscojma86 correct me if wrong

stuartmorgan-g · 2020-01-14T03:20:23Z

how updates to the platform directories (if any) would effect this file

This file is generated on demand though; I would expect that for desktop it would be in the ephemeral/ directories, which means not checked in, so I'm not sure what the problem would be.

franciscojma86 · 2020-01-14T17:18:35Z

Yeah it was a brief discussion with @jonahwilliams , but I should've opened an issue and discussed there. Looked simple enough at the beginning (doesn't it all? 😄 ).

There were no strong reasons to go either way. The one advantage of going this way, is to stop using the key-value file and turn it into a JSON which is more flexible and we can version. Specially since .flutter-plugins-dependencies is already a JSON file. I actually did a per platform implementation here #47258. It turns out it breaks other tests since it creates the ephemeral folder prematurely. The current implementation has been a bit smoother.

The only advantage I see of doing a per platform .flutter-plugins file is that we wouldn't have to add json parsing logic on the Podfile templates, but I actually already did that in another branch, and it's just a couple extra lines of codes.

@stuartmorgan do you have any concerns with this method? The other PR is not far behind so I can continue that one if people think is a better options. Personally, I don't see any clear advantages.

stuartmorgan-g · 2020-01-14T17:44:42Z

I have a couple of potential concerns, although nothing definite enough to prevent moving forward:

The JSON dependency isn't just for Podfiles, it's for every platform that needs to do something with those files. E.g., if I want to use it on Linux, then I need to do JSON parsing in... a tool called from a Makefile I guess? But since I don't have the details of how it'll be used, that's not a concrete concern, since maybe that's no harder than whatever I would need to do with the key-value version. (Also, .flutter-plugins-dependencies already being JSON may make that moot, since I may need that on other platforms as well.)
In a future where platform build details are in separate packages, having them all writing into a single file seems potentially awkward, but that's presumably something we can address by just having one of the APIs be to provide the JSON to put in the platform-specific section.

And I can see the argument for this being potentially more future-proof, since we can add new sections to a JSON file without breaking existing projects. Let's proceed with this, and if it's problematic for Linux or Windows later I can figure out how to adapt.

stuartmorgan-g · 2020-01-14T17:42:23Z

packages/flutter_tools/lib/src/plugins.dart

+  }
+}
+
+/// Writes the .flutter-plugins.json file based on the list of plugins.


But that file means something different; wouldn't changing the content break a bunch of projects?

stuartmorgan-g · 2020-01-14T17:47:53Z

packages/flutter_tools/lib/src/plugins.dart

 }

+void _addPluginsToPlatform(List<Plugin> plugins, FlutterProjectPlatform platform, Map<String, dynamic> pluginsMap) {
+  if (!platform.existsSync()) {


If the file is in a central location rather than sharded to the platform directories, why do we need this check?

It's not clear to me that if I add a new platform to an existing project this file would be regenerated unless I also change the plugins, so skipping platforms make cause weird errors for people adding a platform after the fact. And generating them all should be harmless, right?

I got rid of this and added to the map directly. I was trying to avoid writing platforms that have no plugins but as you say, there's no much point to that. If there are no plugins at all, the file won't be written.

stuartmorgan-g · 2020-01-14T17:50:11Z

packages/flutter_tools/lib/src/plugins.dart

  return plugins;
 }

+void _addPluginsToPlatform(List<Plugin> plugins, FlutterProjectPlatform platform, Map<String, dynamic> pluginsMap) {


Could you document this? It's not entirely obvious to me how the arguments interact without looking at a bunch of code (e.g., that pluginsMap is basically an out param).

stuartmorgan-g · 2020-01-14T17:50:59Z

packages/flutter_tools/lib/src/plugins.dart

+  const String info = 'This is a generated file; do not edit or check into version control.';
+
+  final Map<String, dynamic> result = <String, dynamic> {};
+  result['_info'] = '// $info';


Why does this have a C-style comment prefix, when it's just a string value in a JSON?

Bad copy paste :)

stuartmorgan-g · 2020-01-14T18:01:53Z

packages/flutter_tools/lib/src/plugins.dart

+    }
+  }
+
+  return oldPluginFileContent != _readFileContent(pluginsFile);


Since there's encoding/decoding going on now, it would be nice to make this a little bit more efficient. You don't need to json.encode above if pluginsMap.isEmpty, and you don't need to do the file readback since you already have pluginFileContent. There's already branching for empty vs. non-empty, so being more efficient here would add minimal complexity. Something like:

final File pluginsFile = project.flutterPluginsJsonFile; final String oldPluginFileContent = _readFileContent(pluginsFile); final String pluginFileContent = pluginsMap.isNotEmpty ? json.encode(result) : null; if (pluginFileContent != null) { pluginsFile.writeAsStringSync(pluginFileContent, flush: true); } else { if (pluginsFile.existsSync()) { pluginsFile.deleteSync(); } } return pluginFileContent != oldPluginFileContent;

I moved the check for empty plugins to the top. Feels a bit cleaner to me. WDYT?

packages/flutter_tools/lib/src/project.dart

stuartmorgan-g · 2020-01-14T18:04:37Z

packages/flutter_tools/lib/src/project.dart

+  bool existsSync();
+
+  /// Creates a list with the project plugins supported for the current platform.
+  List<Map<String, dynamic>> pluginsList(List<Plugin>plugins) {


filterPlugins? The current name suggests to me that it's returning a list of plugins that are a property of the project, rather than being driven by the input list. (It feels a bit odd for this to be part of the project, rather than a stand-alone utility or something on a plugins class, but I don't feel super strongly about that.)

I agree it feels better as a helper function. Done

stuartmorgan-g · 2020-01-14T18:06:42Z

Maybe the rest of the platform can have their own file while iOS/Android continue to use .flutter-plugins until we migrate the affected projects.

Isn't that why the legacy file is still being written in this patch? We should still be creating the new file for all platforms so there's something to migrate to later.

stuartmorgan-g

Just small nits; LGTM

packages/flutter_tools/lib/src/plugins.dart

stuartmorgan-g · 2020-01-15T17:29:11Z

packages/flutter_tools/lib/src/plugins.dart

+/// If there aren't any plugins, then the files aren't written to disk. The resulting
+/// file looks something like this:
+/// {
+///   "info": "This is a generated file; do not edit or check into version control.",


Isn't the location of this line in the file going to be "random" (not from run to run unless the map implementation changes, but not guaranteed to be first)? I don't have a solution that doesn't break existing apps since we're re-using a file that's already a dictionary--otherwise we could wrap the real content in an array with the comment then the content--but I'm wondering how useful this will end up being without an order dependency. I guess better than nothing...

I changed the test to not require an order comparison and clarified in the comment that the order is not guaranteed

stuartmorgan-g · 2020-01-15T17:31:54Z

packages/flutter_tools/lib/src/plugins.dart

+  return directAppDependencies;
+}
+
+// This method will be DEPRECATED in favor of _writeFlutterPluginsList.


Since this isn't an API surface, it won't ever actually be deprecated, will it, just removed? The API is the existence of the file, not the method.

packages/flutter_tools/lib/src/project.dart

stuartmorgan-g · 2020-01-15T17:36:24Z

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

          'plugin-c=/.tmp_rand0/plugin.rand2/\n'
          ''
        );
        expect(flutterProject.flutterPluginsDependenciesFile.readAsStringSync(),


Do we actually want this test to break if the order of the elements in this file change? I.e., do we consider the order of elements in a JSON dictionary to be an API? That seems extreme; I know in some cases Ian has advocated for that, but for a file that's only intended for use as JSON I'm not sure we actually want that.

If we don't, this test should parse and then do a deep structure equality test, instead of comparing strings.

Updated test as discussed

franciscojma86 · 2020-01-15T19:37:26Z

dev/devicelab/bin/tasks/plugin_dependencies_test.dart


-      // https://github.com/flutter/flutter/issues/46898
-      // https://github.com/flutter/flutter/issues/39657
-      File(path.join(pluginCDirectory.path, 'android', 'build.gradle')).deleteSync();


Removed this line since the top issue is closed and the second one I assume would be fixed by this PR.

franciscojma86 · 2020-01-15T19:45:26Z

@blasten @jonahwilliams I updated it to include the plugins list in .flutter-plugins-dependencies rather than replacing it. Updated a device lab test as well. PTAL!

blasten · 2020-01-15T20:46:21Z

packages/flutter_tools/lib/src/plugins.dart

+///     "windows": [],
+///     "web": []
+///   },
+///   "dependencyGraph": [


Is the plan to split "dependencyGraph" per platform in a separate PR?

I don't think it makes much sense to split it anymore. It doesn't really add any value. I started that way, but then we'd have change the way the dependencies are handled in gradle, which at that point felt unnecessary. What do you think?

Gradle is working around the fact that dependencyGraph isn't split per platform. I think it would be ideal if Gradle didn't have to workaround that and instead just read this file. This could be done in a separate PR though if that makes sense.

Makes sense, I did it here :)

packages/flutter_tools/lib/src/plugins.dart

blasten

LGTM! + comment

…tions (#48614)" This reverts commit 5eb394e.

…tions (#48614)" (#49005) This reverts commit 5eb394e.

…tions #48614" (#49085)

franciscojma86 added 3 commits December 27, 2019 11:11

Reland "fix duration event of timeline summary (flutter#47742)"

822528a

This reverts commit 50d4212.

Merge branch 'master' of github.com:flutter/flutter

79096d4

Merge branch 'master' of github.com:flutter/flutter

0ace7f2

franciscojma86 added the work in progress; do not review label Jan 11, 2020

franciscojma86 requested a review from jmagman as a code owner January 11, 2020 00:31

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 11, 2020

googlebot added the cla: yes label Jan 11, 2020

franciscojma86 removed the request for review from jmagman January 11, 2020 00:32

franciscojma86 changed the title ~~WIP no-ops with json~~ [flutter_tools] Removes the need of a no-op plugin implementations Jan 13, 2020

franciscojma86 requested review from blasten, jonahwilliams and stuartmorgan-g January 13, 2020 23:39

franciscojma86 removed the work in progress; do not review label Jan 13, 2020

jonahwilliams reviewed Jan 13, 2020

View reviewed changes

packages/flutter_tools/lib/src/project.dart Show resolved Hide resolved

jonahwilliams reviewed Jan 13, 2020

View reviewed changes

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

jonahwilliams reviewed Jan 13, 2020

View reviewed changes

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

jonahwilliams reviewed Jan 13, 2020

View reviewed changes

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

jonahwilliams reviewed Jan 14, 2020

View reviewed changes

blasten reviewed Jan 14, 2020

View reviewed changes

stuartmorgan-g reviewed Jan 14, 2020

View reviewed changes

franciscojma86 added 4 commits January 14, 2020 16:34

Add to right clock

6295171

Address some review comments on code improvements

ae0528a

Move filterPlugins as a helper

1b93962

Analyzer

d3fd61a

franciscojma86 force-pushed the plugins-json branch from 6aba1b9 to d3fd61a Compare January 15, 2020 00:34

franciscojma86 added 2 commits January 14, 2020 16:39

Rebased and updated escapePath

4437b31

Mock flutter version

78a470b

stuartmorgan-g approved these changes Jan 15, 2020

View reviewed changes

franciscojma86 added 2 commits January 15, 2020 10:11

Update dependency test and remove dependencies from each plugin

9862fe9

Update test without order

413db39

franciscojma86 commented Jan 15, 2020

View reviewed changes

franciscojma86 added 2 commits January 15, 2020 11:38

space

492f3ae

Update test

b3b4926

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jan 15, 2020

blasten reviewed Jan 15, 2020

View reviewed changes

packages/flutter_tools/lib/src/plugins.dart Show resolved Hide resolved

franciscojma86 mentioned this pull request Jan 15, 2020

[flutter_tools] Migrate platforms away from .flutter-plugins #48918

Closed

Add issue

609ecfb

blasten approved these changes Jan 15, 2020

View reviewed changes

franciscojma86 added 2 commits January 15, 2020 16:27

Add dependencies to plugins

54c749d

Fix plugin filtering

23cb397

franciscojma86 added the waiting for tree to go green label Jan 16, 2020

franciscojma86 merged commit 5eb394e into flutter:master Jan 16, 2020

jmagman added a commit that referenced this pull request Jan 17, 2020

Revert "[flutter_tools] Removes the need of a no-op plugin implementa…

0fbbd04

…tions (#48614)" This reverts commit 5eb394e.

jmagman mentioned this pull request Jan 17, 2020

Revert "[flutter_tools] Removes the need of a no-op plugin implementations" #49005

Merged

jmagman added a commit that referenced this pull request Jan 17, 2020

Revert "[flutter_tools] Removes the need of a no-op plugin implementa…

b225cb8

…tions (#48614)" (#49005) This reverts commit 5eb394e.

franciscojma86 mentioned this pull request Jan 17, 2020

Reland "[flutter_tools] Removes the need of a no-op plugin implementations #48614" #49085

Merged

franciscojma86 added a commit that referenced this pull request Jan 17, 2020

Reland "[flutter_tools] Removes the need of a no-op plugin implementa…

fcf341e

…tions #48614" (#49085)

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021


		import 'dart:async';

		import 'package:flutter_tools/src/platform_plugins.dart';

[flutter_tools] Removes the need of a no-op plugin implementations #48614

[flutter_tools] Removes the need of a no-op plugin implementations #48614

Uh oh!

Conversation

franciscojma86 commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

franciscojma86 commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

stuartmorgan-g commented Jan 14, 2020

Uh oh!

jonahwilliams commented Jan 14, 2020

Uh oh!

blasten commented Jan 14, 2020

Uh oh!

jonahwilliams commented Jan 14, 2020

Uh oh!

stuartmorgan-g commented Jan 14, 2020

Uh oh!

franciscojma86 commented Jan 14, 2020

Uh oh!

stuartmorgan-g commented Jan 14, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jan 14, 2020

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

franciscojma86 commented Jan 11, 2020 •

edited

Loading