Add `--platforms` to `flutter create -t plugin` command #59507

cyanglaz · 2020-06-15T22:47:36Z

Description

Adds a --platforms flag to the flutter create. The flag only works in -t plugin and -t app
This Pr doesn't make --platforms support web yet because web template is not ready.

Note: We need to decide what to do with the version constraint before landing this.

Related Issues

#59493

Tests

plugin supports ios if requested
plugin supports android if requested
create an empty plugin, then add ios
create an empty plugin, then add android
create an empty plugin, then add linux
create an empty plugin, then add macos
create an empty plugin, then add windows
create a plugin with ios, then add macos
create a plugin with ios and android
create a module with --platforms throws error.
create a package with --platforms throws error.

Also updated some existing tests to fit the new flag.

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.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

jonahwilliams

This PR is really hard to review, because I'm not really sure which code was changed and which was original. I'm not convinced that doing a massive refactor ahead of trying to change the behavior is the best way to go about this.

Could I suggest a more incremental approach: make the methods you need to override on create visibleForTesting, then create an AddPackage command that extends flutter create.

Then, you could gradually refactor shared functionality with huge diffs

packages/flutter_tools/lib/src/commands/create_mixin.dart

packages/flutter_tools/lib/src/commands/add_platform.dart

jonahwilliams · 2020-06-17T21:02:44Z

packages/flutter_tools/lib/src/commands/create_mixin.dart

+  /// Perform an initial validation of the environment and args.
+  ///
+  /// Should be called as early as possible in [FlutterCommand.runCommand].
+  void doInitialValidation() {


There is already a hook for this: FlutterCommand.verifyThenRunCommand

packages/flutter_tools/lib/src/commands/create_mixin.dart

packages/flutter_tools/test/commands.shard/permeable/add_platform_test.dart

jonahwilliams · 2020-06-18T17:23:00Z

packages/flutter_tools/lib/src/commands/create.dart

+  /// Adding argument is optional if the command does not require the user to explicitly state what platforms the project supports.
+  void _addPlatformsOptions() {
+    argParser.addMultiOption('platforms',
+        help: 'the platforms supported by this project.',


I would expand this comment a bit for each kind of template. Talk about what it does/does not generate

updated, see if you like this.

jonahwilliams · 2020-06-18T17:24:01Z

packages/flutter_tools/lib/src/commands/create.dart

+        globals.printError('''
+You have generated a new plugin project without
+specifying the `--platforms` flag. A plugin project supports no platforms is generated.
+The project will not compile until you add platforms implementation.


project -> plugin?

The project will not compile until you add platforms implementation.

This isn't quite true, it just isn't useful

Or remove this line?

jonahwilliams · 2020-06-18T17:24:16Z

packages/flutter_tools/lib/src/commands/create.dart

    generatedCount += await _renderTemplate('plugin', directory, templateContext, overwrite: overwrite);
+    if (!argResults.wasParsed('platforms')) {
+        globals.printError('''
+You have generated a new plugin project without


Can you hoist this out into a constant so it is easier to read?

please move to the top of the file

jonahwilliams · 2020-06-18T17:26:13Z

packages/flutter_tools/lib/src/commands/create.dart

+          context['android'] = true;
+          break;
+        case 'web':
+          context['web'] =  featureFlags.isWebEnabled && true;


Rather than conditionally checking here, you might be able to only add web, linux, macos, windows to the list itself if the experiment is enabled. Then you can do:

for (final String platform in platforms) { context[platform] = true; }

Updated with a slightly different approach, please take a look and see if you like it.

packages/flutter_tools/lib/src/commands/create.dart

packages/flutter_tools/test/commands.shard/permeable/create_test.dart

cyanglaz

@jonahwilliams I updated the PR with some of your suggestions. I'll work on the other half of the comments and ping you once they are all addressed.

cyanglaz · 2020-06-18T17:56:54Z

packages/flutter_tools/lib/src/commands/create.dart

+        globals.printError('''
+You have generated a new plugin project without
+specifying the `--platforms` flag. A plugin project supports no platforms is generated.
+The project will not compile until you add platforms implementation.


Or remove this line?

cyanglaz · 2020-06-18T17:57:00Z

packages/flutter_tools/lib/src/commands/create.dart

    generatedCount += await _renderTemplate('plugin', directory, templateContext, overwrite: overwrite);
+    if (!argResults.wasParsed('platforms')) {
+        globals.printError('''
+You have generated a new plugin project without


cyanglaz · 2020-06-18T18:20:29Z

packages/flutter_tools/lib/src/commands/create.dart

+  /// Adding argument is optional if the command does not require the user to explicitly state what platforms the project supports.
+  void _addPlatformsOptions() {
+    argParser.addMultiOption('platforms',
+        help: 'the platforms supported by this project.',


updated, see if you like this.

jonahwilliams · 2020-06-18T22:48:11Z

packages/flutter_tools/lib/src/commands/create.dart

+    if (argResults.wasParsed('platforms') && (generateModule || generatePackage)) {
+        final String template = generateModule?'module':'package';
+        throwToolExit(
+          'The "--platforms" argument is not supported in $template template. please remove the argument and try again.',


nit, remove : please remove the argument and try again

jonahwilliams · 2020-06-18T22:48:21Z

packages/flutter_tools/lib/src/commands/create.dart

+    final List<String> platforms = stringsArg('platforms');
+    // `--platforms` does not support module and package.
+    if (argResults.wasParsed('platforms') && (generateModule || generatePackage)) {
+        final String template = generateModule?'module':'package';


jonahwilliams · 2020-06-18T22:49:11Z

packages/flutter_tools/lib/src/commands/create.dart

-
+        final List<String> platforms = _getSupportedPlatformsFromTemplateContext(templateContext);
+        String platformsString = '';
+        for (int i = 0; i < platforms.length; i++) {


platforms.join(',') ?

done. Good call!

jonahwilliams · 2020-06-18T22:49:55Z

packages/flutter_tools/lib/src/commands/create.dart

+    argParser.addMultiOption('platforms',
+        help: 'the platforms supported by this project.'
+          'This argument only works when the --template is set to app or plugin.'
+          'Respective folders will be generated in the target project.'


clarify "respective folders" - "platform folders (android/ ) ?

jonahwilliams · 2020-06-18T22:51:28Z

packages/flutter_tools/lib/src/commands/create.dart

+          'Respective folders will be generated in the target project.'
+          'When adding platforms to a plugin project, the pubspec.yaml will be updated with the requested platform.'
+          'When adding windows, linux or macos platforms, the platform feature has to be enable. Otherwise it would be a no-op.'
+          'To enable desktop features, run `flutter config --enable-<desktopName>-desktop.'


Maybe: Adding desktop platforms requires the corresponding desktop config setting to be enabled

jonahwilliams · 2020-06-18T22:52:38Z

packages/flutter_tools/lib/src/commands/create.dart

    generatedCount += await _renderTemplate('plugin', directory, templateContext, overwrite: overwrite);
+    if (!argResults.wasParsed('platforms')) {
+        globals.printError('''
+You have generated a new plugin project without


please move to the top of the file

jonahwilliams · 2020-06-18T22:53:26Z

packages/flutter_tools/lib/src/commands/create.dart

    if (boolArg('pub')) {
      await pub.get(context: PubContext.create, directory: directory.path, offline: boolArg('offline'));
-      await project.ensureReadyForPlatformSpecificTooling(checkProjects: false);
+      await project.ensureReadyForPlatformSpecificTooling(checkProjects: pluginExampleApp);


this is confusing, why do we need to only check projects if we're creating an example app

As a side effect, this method always generates the GeneratedPluginRegstraint files for iOS and Android unless the checkProject flag is true. When the plugin is not for mobile, this method still generate those files, including iOS and Android.

makes sense

Do you guys think pluginExampleApp is confusing? I am thinking it might be better just to call it checkProjects or checkProjectsWhileEnsuringTooling

I think the non specificity of "checkProjects" is what's throwing me off (I know you didn't name it). Like, check what? If that was a better name, pluginExampleApp might make sense.

By looking at the code of ensureReadyForPlatformSpecificTooling, I also not sure what that meant. @jonahwilliams do you have any idea? Maybe we can add a TODO comment on that method to either fix the param name or add some doc/comments on the checkProjects param?

yeah, TODO for now.

jonahwilliams · 2020-06-18T22:54:21Z

packages/flutter_tools/lib/src/commands/create.dart

    return generatedCount;
  }

+  void _updateTemplateContextWithPlatforms(Map<String, dynamic> context, List<String> platforms) {


This is confusing, everything gets set to false by default, then everything get sets to true, then some are conditional set to false.

you are right, I deleted this method and updated the caller a bit. Please take a look see if it makes more sense.

jonahwilliams · 2020-06-18T22:55:46Z

packages/flutter_tools/lib/src/plugins.dart

+  ///
+  /// Throws ToolExit if cannot find the `platforms:` map.
+  static YamlMap getPlatformsYamlMap(YamlMap pubspec) {
+    if (pubspec == null) {


Assuming that someone is migrating an old plugin and trying to use add-platform, it would be nice to at least link to page explaining the formatting

done. We only need to show this error if the platforms map is missing or not valid, other failing scenarios are just general invalid pubspec.

jonahwilliams · 2020-06-18T22:56:14Z

packages/flutter_tools/lib/src/plugins.dart

+      throwToolExit('Did not find valid `platforms` map in pubspec.yaml', exitCode: 2);
+    }
+
+    return pluginConfig['platforms'] as YamlMap;


missing a type check here, platforms could be anything

jonahwilliams · 2020-06-18T23:00:35Z

packages/flutter_tools/lib/src/plugins.dart

+      final List<String> platformsToAdd = List<String>.from(platforms);
+      final YamlMap platformsMap = getPlatformsYamlMap(pubspec);
+      final List<String> existingPlatforms = platformsMap.keys.cast<String>().toList();
+      platformsToAdd.removeWhere((String platform) => existingPlatforms.contains(platform));


platformsToAdd.removeWhere(existingPlatforms.contains);

jonahwilliams · 2020-06-18T23:02:27Z

packages/flutter_tools/lib/src/plugins.dart

+          frontSpaces = line.split('platforms:').first;
+          index = i + 1;
+        }
+        if (line.contains('some_platform:')) {


Does this rely on on having a fake platform key?

This logic doesn't. however it does remove the some_platform.
The reason that some_platform needs to be there is that we need to keep the pubspec.yaml valid. Otherwise, other steps of the create command would fail (pub get for example)

Another way is to perhaps not having the whole flutter map in the pubspec if there are no --platforms specified, then add the flutter map programmatically when --platforms is specified.

This seems pretty fragile, what if someone develops android/ios and then wants to use add-platform to add a new desktop or web implementation?

This should also work as the code is to look for the platforms: key and add an entry under it. i think I have a test covers this scenario: 'create a plugin with ios, then add macos'.
It only removes 'some_platform' map, which is an invalid platform.

jonahwilliams · 2020-06-18T23:06:03Z

packages/flutter_tools/lib/src/plugins.dart

+  /// ```
+  ///
+  /// This method doesn't support the legacy plugin pubspec format and will throw error if try to update a pubspec.yaml that uses the legacy format.
+  static Future<void> updatePubspecWithPlatforms(String projectDir, List<String> platforms, String pluginClass, String androidIdentifier) async {


It seems like there are a lot of ways for this method to fail, and it requires a sentinel data in the form of the comment. Is there a way this process could be simplified by ignoring comments? Is add-platforms going to be usable if it requires a very specific structure?

one way to improve this is to ignore the lines that starts with a #

Other than that, I don't find a perfect solution to update this without breaking the existing yaml comments. Which I think we need to preserve because those are great instructions for new users.

I would say ignore the comments and focus on the structured data.

Did you mean to overwrite all the comments when updating pubspec.yaml?
Something like
file.write(pubspecYamlMap.convertToString)?
(I'm making up APIs here)

Or did you mean to skip comments like

if (line.contains('#')) { continue; }

jmagman

LGTM

… not enabled

flutter-github-sync · 2020-06-24T00:02:26Z

Google testing passed!

flutter-github-sync · 2020-06-24T00:02:27Z

Google testing passed!

flutter-github-sync · 2020-06-24T00:02:28Z

Google testing passed!

flutter-github-sync · 2020-06-24T00:02:49Z

Google testing passed!

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Jun 15, 2020

googlebot added the cla: yes label Jun 15, 2020

cyanglaz force-pushed the add_platforms branch from 9767608 to 17f2cb0 Compare June 15, 2020 22:49

cyanglaz changed the title ~~Refactor create.dart so it can share code with add-platform~~ Add flutter add-platform command Jun 17, 2020

cyanglaz marked this pull request as ready for review June 17, 2020 20:47

Chris Yang added 2 commits June 17, 2020 13:50

add platform

13ff41c

rebase conflict resolve

4fd0af6

cyanglaz force-pushed the add_platforms branch from 1d2816c to 4fd0af6 Compare June 17, 2020 20:52

jonahwilliams reviewed Jun 17, 2020

View reviewed changes

Chris Yang added 6 commits June 17, 2020 14:41

removing create_mixin

edbe882

draft

442c455

draft

d6c6b7b

add tests

f170cd3

draft

3a76b86

tests

c13bab5

cyanglaz changed the title ~~Add flutter add-platform command~~ Add --platforms to flutter create -t plugin command Jun 18, 2020

jonahwilliams reviewed Jun 18, 2020

View reviewed changes

throw if --platforms is added for module or package

5901b4f

jonahwilliams reviewed Jun 18, 2020

View reviewed changes

packages/flutter_tools/test/commands.shard/permeable/create_test.dart Outdated Show resolved Hide resolved

Chris Yang added 2 commits June 18, 2020 10:53

clean up and update platforms help message

b762e06

some review fixes

ba296a5

cyanglaz commented Jun 18, 2020

View reviewed changes

cyanglaz removed the work in progress; do not review label Jun 18, 2020

Chris Yang added 2 commits June 18, 2020 15:21

review fixes

44217ab

cleanup

1001bdf

jonahwilliams reviewed Jun 18, 2020

View reviewed changes

formatting fix

1f5b607

jmagman approved these changes Jun 23, 2020

View reviewed changes

Chris Yang added 4 commits June 23, 2020 15:15

merge and resolve conflicts

b96c293

add web support

ec4aaf7

add tests for web

9f21a0b

fix typo

d9a79b0

cyanglaz added the waiting for tree to go green label Jun 23, 2020

fix a bug where a non-mobile platform is specified but the feature is…

a4a9701

… not enabled

fluttergithubbot merged commit 2e63b7d into flutter:master Jun 24, 2020

cyanglaz deleted the add_platforms branch June 24, 2020 00:54

liyuqian mentioned this pull request Jun 29, 2020

flutter ide-config is broken #60561

Closed

cyanglaz mentioned this pull request Jun 30, 2020

[flutter_tool] fix ide-config crash because of no android key #60617

Merged

13 tasks

cyanglaz mentioned this pull request Jul 15, 2020

Update the flutter create -t plugin document to reflect changes related to --platforms flutter/website#4344

Merged

jmagman mentioned this pull request Aug 18, 2020

Template plugin should not support platforms by default #48270

Closed

stevemessick mentioned this pull request Sep 2, 2020

Plugin template now requires --platforms flutter/flutter-intellij#4796

Closed

jmagman mentioned this pull request Oct 17, 2020

Generate only requested platform directories on create #68376

Merged

10 tasks

jmagman mentioned this pull request Jan 13, 2021

flutter create --template plugin should make a federated plugin by default #43284

Open

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021

Add --platforms to flutter create -t plugin command #59507

Add --platforms to flutter create -t plugin command #59507

Uh oh!

Conversation

cyanglaz commented Jun 15, 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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

cyanglaz left a comment

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!

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

Add `--platforms` to `flutter create -t plugin` command #59507

Add `--platforms` to `flutter create -t plugin` command #59507

cyanglaz commented Jun 15, 2020 •

edited

Loading