Generate only requested platform directories on create by jmagman · Pull Request #68376 · flutter/flutter

jmagman · 2020-10-17T02:15:36Z

Description

The android (and ios) directories are incorrectly created during the plugin registrant generation step. After a flutter create the only thing in android is GeneratedPluginRegistrant.java and the only thing in ios is GeneratedPluginRegistrant.{h,m}.

flutter/packages/flutter_tools/lib/src/project.dart

Lines 860 to 862 in 345188d

    
           if (appManifestFile == null || !appManifestFile.existsSync()) { 
        
             return AndroidEmbeddingVersion.v1; 
        
           }

is guarded by a check if the android directory exists (it did, but only contained GeneratedPluginRegistrant.java) and returned v1 because the app manifest didn't exist, and caused a confusing warning.

Avoid creating platform directories during the plugin registrant generation step, which bypasses the Android embedding version warning.

For flutter create -t app, generate the directories/plugins only for the requested platforms.
For other commands that regenerate various files (plugins, create missing template files, etc), only do so in existing platform directories.
Replace the checkProject concept by explicitly requesting platforms based on user flags, existing directories, and whether features are on, as appropriate.

Related Issues

Fixes #67346
Fixes #60023

Related:
#31342
#59507

Tests

Added --platform app create tests. Update existing plugin, project, cocoapods tests which already exercise these code paths.

Checklist

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

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.

jmagman · 2020-10-17T02:17:48Z

packages/flutter_tools/lib/src/plugins.dart

checkProjects was never actually passed in, so was always false.

jmagman · 2020-10-17T02:39:25Z

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

These templateContext values are only true if the corresponding feature is on.

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

Lines 423 to 426 in 73301a3

web: featureFlags.isWebEnabled && platforms.contains('web'),

linux: featureFlags.isLinuxEnabled && platforms.contains('linux'),

macos: featureFlags.isMacOSEnabled && platforms.contains('macos'),

windows: featureFlags.isWindowsEnabled && platforms.contains('windows'),

jmagman · 2020-10-17T18:39:27Z

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

Moved to app does not include desktop or web by default

jmagman · 2020-10-17T18:39:37Z

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

Moved to app does not include desktop or web by default

jmagman · 2020-10-17T18:39:44Z

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

Moved to app does not include desktop or web by default

jmagman · 2020-10-17T18:40:11Z

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

Moved to plugin does not include desktop or web by default

jmagman · 2020-10-17T18:40:17Z

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

Moved to plugin does not include desktop or web by default

jmagman · 2020-10-17T18:40:23Z

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

Moved to plugin does not include desktop or web by default

jmagman · 2020-10-19T16:09:28Z

packages/flutter_tools/lib/src/project.dart

@stuartmorgan I moved the TODOs scattered around to reexamine the logic to this spot. Let me know if you want it somewhere else, or need it reworded.

The TODO was really about figuring out what checkProjects was for and why we would ever want the checkProjects = false behavior since it seemed really bizarre, so it looks like this PR obsoletes it. But I can clean it up later.

jonahwilliams

Only small nits, seems like a good change.

Why were we getting the warning about the android embedding though? even if the project wasn't there or was partially created, we should probably default to not showing a warning

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

jonahwilliams · 2020-10-19T16:29:05Z

packages/flutter_tools/lib/src/project.dart

Is this comment misleading now? it also re-generates for desktop+web projects if they exist

Good call, reworded.

jmagman · 2020-10-19T18:02:25Z

Why were we getting the warning about the android embedding though? even if the project wasn't there or was partially created, we should probably default to not showing a warning

Before this PR the android (and ios directories) were incorrectly created during the plugin registrant generation step. After a flutter create the only thing in android was GeneratedPluginRegistrant.java and the only thing in ios was GeneratedPluginRegistrant.{h,m}.

flutter/packages/flutter_tools/lib/src/project.dart

Lines 860 to 862 in 345188d

    
           if (appManifestFile == null || !appManifestFile.existsSync()) { 
        
             return AndroidEmbeddingVersion.v1; 
        
           }

is guarded by a check if the android directory exists (it did, but only contained GeneratedPluginRegistrant.java) and returned v1 because the app manifest didn't exist.

After this PR, the android directory is not created during the during the plugin registrant generation step, so the embedding version is never checked.

jonahwilliams

LGTM with nits

cyanglaz

LGTM!

jmagman self-assigned this Oct 17, 2020

flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Oct 17, 2020

google-cla bot added the cla: yes label Oct 17, 2020

jmagman commented Oct 17, 2020

View reviewed changes

packages/flutter_tools/lib/src/plugins.dart Outdated

Copy link

Member Author

jmagman Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkProjects was never actually passed in, so was always false.

jmagman force-pushed the platform-generation branch 2 times, most recently from 74728a1 to 40b62e1 Compare October 17, 2020 02:37

jmagman commented Oct 17, 2020

View reviewed changes

jmagman force-pushed the platform-generation branch 3 times, most recently from ae86c5c to 97a6cac Compare October 17, 2020 18:34

jmagman commented Oct 17, 2020

View reviewed changes

jmagman requested review from cyanglaz, jonahwilliams and stuartmorgan-g October 19, 2020 16:08

jmagman commented Oct 19, 2020

View reviewed changes

jmagman marked this pull request as ready for review October 19, 2020 16:18

jmagman removed the work in progress; do not review label Oct 19, 2020

jonahwilliams reviewed Oct 19, 2020

View reviewed changes

jonahwilliams approved these changes Oct 19, 2020

View reviewed changes

jmagman added 2 commits October 19, 2020 11:41

Generate only requested platform directories on create

65af7cd

Review edits

ae8f1b9

jmagman force-pushed the platform-generation branch from 97a6cac to ae8f1b9 Compare October 19, 2020 19:05

cyanglaz approved these changes Oct 19, 2020

View reviewed changes

jmagman merged commit 5e17a24 into flutter:master Oct 19, 2020

jmagman deleted the platform-generation branch October 19, 2020 21:17

	if (appManifestFile == null \|\| !appManifestFile.existsSync()) {
	return AndroidEmbeddingVersion.v1;
	}

	web: featureFlags.isWebEnabled && platforms.contains('web'),
	linux: featureFlags.isLinuxEnabled && platforms.contains('linux'),
	macos: featureFlags.isMacOSEnabled && platforms.contains('macos'),
	windows: featureFlags.isWindowsEnabled && platforms.contains('windows'),

Conversation

jmagman commented Oct 17, 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!

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Oct 19, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmagman commented Oct 17, 2020 •

edited

Loading