Use FlutterFeatures to configure web and desktop devices by jonahwilliams · Pull Request #36465 · flutter/flutter

jonahwilliams · 2019-07-18T16:56:40Z

Description

A follow up to #36138, this PR enables the usage of flutter config for web, Linux, macOS, and Windows features. This removes the on-by default behavior of these features on master branch and instead replaces it with a configurable flag. For backwards compatibility, the old environment variables ENABLE_FLUTER_DESKTOP and FLUTTER_WEB still exist. I would like to deprecate and remove these at some point in the future, and encourage user to provide config values.

Example:

> flutter config
Usage: flutter config [arguments]
-h, --help                           Print this usage information.
    --[no-]analytics                 Enable or disable reporting anonymously tool usage statistics and crash reports.
    --clear-ios-signing-cert         Clear the saved development certificate choice used to sign apps for iOS device deployment.
    --gradle-dir                     The gradle install directory.
    --android-sdk                    The Android SDK directory.
    --android-studio-dir             The Android Studio install directory.
    --[no-]enable-web                Enable or disable Flutter Web on master, dev channels.
    --[no-]enable-linux-desktop      Enable or disable Flutter Desktop for Linux on the master channel.
    --[no-]enable-macos-desktop      Enable or disable Flutter Desktop for macOS on the master channel.
    --[no-]enable-windows-desktop    Enable or disable Flutter Desktop for Windows on the master channel.
    --clear-features                 Remove all configured features and restore them to the default values.

Run "flutter help" to see global options.

codecov · 2019-07-18T18:32:03Z

Codecov Report

Merging #36465 into master will increase coverage by 0.72%.
The diff coverage is 72.09%.

@@            Coverage Diff             @@
##           master   #36465      +/-   ##
==========================================
+ Coverage   54.03%   54.75%   +0.72%     
==========================================
  Files         188      188              
  Lines       17344    17303      -41     
==========================================
+ Hits         9372     9475     +103     
+ Misses       7972     7828     -144

Flag	Coverage Δ
#flutter_tool	`54.75% <72.09%> (+0.72%)`	⬆️

Impacted Files	Coverage Δ
...ackages/flutter_tools/lib/src/commands/unpack.dart	`6.93% <ø> (+0.06%)`	⬆️
packages/flutter_tools/lib/src/desktop.dart	`86.95% <ø> (+8.38%)`	⬆️
...kages/flutter_tools/lib/src/commands/assemble.dart	`64.86% <ø> (-0.96%)`	⬇️
.../flutter_tools/lib/src/commands/build_fuchsia.dart	`81.81% <ø> (ø)`	⬆️
packages/flutter_tools/lib/src/device.dart	`65.06% <ø> (-2.56%)`	⬇️
...ackages/flutter_tools/lib/src/commands/create.dart	`67.54% <0%> (-5.25%)`	⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart	`82.43% <0%> (-0.24%)`	⬇️
...lutter_tools/lib/src/windows/windows_workflow.dart	`100% <100%> (ø)`	⬆️
packages/flutter_tools/lib/src/plugins.dart	`86.23% <100%> (ø)`	⬆️
...es/flutter_tools/lib/src/macos/macos_workflow.dart	`100% <100%> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578204d...14220b9. Read the comment docs.

zanderso

lgtm w/ nit

zanderso · 2019-07-19T02:18:06Z

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

@@ -101,10 +101,18 @@ class BuildBundleCommand extends BuildSubCommand {
    // Check for target platforms that are only allowed on unstable Flutter.


Does this comment need an update?

stuartmorgan-g · 2019-07-19T04:42:32Z

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

    // Check for target platforms that are only allowed on unstable Flutter.
    switch (platform) {
      case TargetPlatform.darwin_x64:
+        if (featureFlags.isMacOSEnabled) {


Aren't these all backwards?

(Follow-up: does that mean this codepath isn't tested?)

These were wrong, I've added positive/negative test cases for both

stuartmorgan-g · 2019-07-19T04:46:40Z

packages/flutter_tools/lib/src/linux/linux_workflow.dart

@@ -21,10 +21,10 @@ class LinuxWorkflow implements Workflow {
  bool get appliesToHostPlatform => platform.isLinux;


Shouldn't these be gated on it being enabled too? That's how web is done, and it seems odd to be inconsistent about it. That would also simplify other code, since, e.g., the doctor conditions wouldn't need to check the feature flag.

Yeah, moving the check here simplifies the rest of the code

stuartmorgan-g · 2019-07-19T04:47:54Z

packages/flutter_tools/lib/src/device.dart

-  /// Only add desktop devices if the flag is enabled.
-  static List<DeviceDiscovery> get _conditionalDesktopDevices {
-    return flutterDesktopEnabled ? <DeviceDiscovery>[
+    if (featureFlags.isMacOSEnabled)


Do we actually need any of these checks? The devices object will say it can't discover devices anyways, right? Having the checks consolidated in fewer places seems easier.

stuartmorgan-g · 2019-07-19T04:49:49Z

packages/flutter_tools/lib/src/doctor.dart

-            if (linuxWorkflow.appliesToHostPlatform) LinuxDoctorValidator(),
-            if (windowsWorkflow.appliesToHostPlatform) visualStudioValidator,
-          ],
+        if (featureFlags.isLinuxEnabled && linuxWorkflow.appliesToHostPlatform)


Linux and Windows are behind a check here, but macOS isn't; it should be consistent (even though in practice it doesn't matter, since iOS applying is the same as being on macOS). Per my later comment I think the better option is to remove them all here, but if you don't go that route you should add the check for macOS.

jonahwilliams · 2019-07-19T17:40:37Z

I've also removed the isExperimental command property, since I do not want to have two distinct ways to control feature visibility.

…o use_web_config

)" This reverts commit bd52a78.

)" (#36654) This reverts commit bd52a78.

…tter#36465)" (flutter#36654) This reverts commit bd52a78.

jonahwilliams added 3 commits July 17, 2019 10:59

initial pass at removing version check based visibility

bd20423

update web config library

2d9340b

Add test cases for doctor inclusion

275268e

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 18, 2019

googlebot added the cla: yes label Jul 18, 2019

remove deprecation for now

fb4abd7

jonahwilliams marked this pull request as ready for review July 18, 2019 20:18

jonahwilliams requested review from stuartmorgan-g and zanderso July 18, 2019 22:10

zanderso approved these changes Jul 19, 2019

View reviewed changes

stuartmorgan-g requested changes Jul 19, 2019

View reviewed changes

jonahwilliams added 2 commits July 19, 2019 09:40

testing and cleanup of feature flag

69e8d40

update test values

c56d356

Update flutter_command_test.dart

9c23899

stuartmorgan-g approved these changes Jul 19, 2019

View reviewed changes

jonahwilliams added 2 commits July 19, 2019 12:43

Add env variable for local test

a9141dc

Merge branch 'use_web_config' of github.com:jonahwilliams/flutter int…

14220b9

…o use_web_config

jonahwilliams merged commit bd52a78 into flutter:master Jul 22, 2019

jonahwilliams deleted the use_web_config branch July 22, 2019 01:21

jonahwilliams pushed a commit that referenced this pull request Jul 22, 2019

Revert "Use FlutterFeatures to configure web and desktop devices (#36465

cd4e24c

)" This reverts commit bd52a78.

jonahwilliams mentioned this pull request Jul 22, 2019

Revert "Use FlutterFeatures to configure web and desktop devices" #36654

Merged

jonahwilliams restored the use_web_config branch July 22, 2019 04:47

jonahwilliams pushed a commit that referenced this pull request Jul 22, 2019

Revert "Use FlutterFeatures to configure web and desktop devices (#36465

8eae2df

)" (#36654) This reverts commit bd52a78.

jonahwilliams mentioned this pull request Jul 22, 2019

Reland: use flutter features for web and desktop #36699

Merged

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Use FlutterFeatures to configure web and desktop devices (flutter#36465)

a3e0a84

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Revert "Use FlutterFeatures to configure web and desktop devices (flu…

b3910f7

…tter#36465)" (flutter#36654) This reverts commit bd52a78.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FlutterFeatures to configure web and desktop devices#36465

Use FlutterFeatures to configure web and desktop devices#36465
jonahwilliams merged 9 commits intoflutter:masterfrom
jonahwilliams:use_web_config

jonahwilliams commented Jul 18, 2019 •

edited

Loading

Uh oh!

codecov bot commented Jul 18, 2019 •

edited

Loading

Uh oh!

zanderso left a comment

Uh oh!

zanderso Jul 19, 2019

Uh oh!

stuartmorgan-g Jul 19, 2019

Uh oh!

jonahwilliams Jul 19, 2019

Uh oh!

stuartmorgan-g Jul 19, 2019

Uh oh!

jonahwilliams Jul 19, 2019

Uh oh!

stuartmorgan-g Jul 19, 2019

Uh oh!

jonahwilliams Jul 19, 2019

Uh oh!

stuartmorgan-g Jul 19, 2019

Uh oh!

jonahwilliams Jul 19, 2019

Uh oh!

jonahwilliams commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -101,10 +101,18 @@ class BuildBundleCommand extends BuildSubCommand {
		// Check for target platforms that are only allowed on unstable Flutter.

		@@ -21,10 +21,10 @@ class LinuxWorkflow implements Workflow {
		bool get appliesToHostPlatform => platform.isLinux;

Conversation

jonahwilliams commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zanderso 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!

jonahwilliams commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonahwilliams commented Jul 18, 2019 •

edited

Loading

codecov bot commented Jul 18, 2019 •

edited

Loading