Add --create option to `flutter emulators` command #18235

DanTup · 2018-06-06T12:17:13Z

This is still a bit rough and has no tests, but I wanted to nail down the logic before I start adding tests against it. This is based on the logic described in #13379 (comment). I don't entirely like how we have to do it (for ex. trying to create on without the sdk ID because we can't get a list of these, only devices/targets) but (on my Macbook at least) it seems to work.

LMK what you think. We may also wish to review the device we pick (currently we try pixel and pixel_xl) and whether we want to lock to a specific Android API version or just take the highest (which is what this currently does).

(cc @devoncarew @mit-mit)

devoncarew

Thanks for starting this! I added some comments on the CLI UI.

devoncarew · 2018-06-11T14:50:40Z

packages/flutter_tools/lib/src/android/android_sdk.dart

use a named param instead?

This matches other functions in this file. Should we change them to keep them consistent?

String getAdbPath([AndroidSdk existingSdk]); String getEmulatorPath([AndroidSdk existingSdk]);

No, let's keep them consistent.

devoncarew · 2018-06-11T14:53:10Z

packages/flutter_tools/lib/src/android/android_sdk.dart

I suspect this fallback logic parallels other android tools lookups in the source base (adb?). We may want to just fail if we can't locate an android sdk however (not do the path based lookup for avdmanager).

Currently there are three possibilities in here (same for the other few in this file):

If it exists on the SDK passed in (which looks to come from context[AndroidSdk]), we use that

If we can locate an SDK, that's used

Otherwise we try os.which('emulator')?.path

I'm not sure which out 1 and 2 is the best one (or whether we need 3). WDYT? If going with 1 then we can just delete this function; but I'm not sure whether that's a good bet. There are some other paths that do seem to use that and bail out if it's not set without any fallback, eg.:

Future<bool> _checkForSupportedAdbVersion() async { if (androidSdk == null) return false;

Though interestingly, it then calls getAdbPath which does the fallback; but in this case it would never trigger because of the return above.

Let's just delete the os.which('emulator')?.path option.

Though interestingly, it then calls getAdbPath which does the fallback; but in this case it would never trigger because of the return above.

Hmm, sounds like some logic was orphaned by a refactor. Falling back to 'which adb' was useful since the user could then have a little functionality - like listing devices - even if they didn't have the full functionality that configuring an android sdk would give. If this regressed at some point, and people didn't notice, than it's likely that we don't need the fallback even for adb.

Ok, I'll remove the which. Looks like there are still some code paths that don't do what I described above, including getAdbDevices, so my comment about was a little misleading. Whether or not _checkForSupportedAdbVersion is behaving as we'd like though, I'm not sure. LMK if you think this needs some investigation and I'll take a look outside of this PR.

devoncarew · 2018-06-11T14:55:35Z

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

Should --create be a flag (instead of an option)? We could auto-name the resulting image (pixel, pixel_xl-2, ...), and have an optional flag to name a created image.

create feels like a subcommand, even if the package:args api doesn't make it easy to have it as one.

Should --create be a flag (instead of an option)? We could auto-name the resulting image

I was a bit unsure about this. If we pick the name for the user then it feels weird if we tell them "an emulator with that name already exists" so I was going to make them always provide it.

Of course, we could do the check ourselves first and only prompt if it already exists, but that might be better handled in the editor than here so it has better control over how to present it.

I was wondering if we should expose an emulator.getDeviceProfiles. That way when the user chooses "create emulator" in the editor, we can fetch the possible profiles (and filter it to pixels, nexuses, whatever) and then let them pick a device. I think being able to pick between a phone and a tablet could be helpful?

If we pick the name for the user then it feels weird if we tell them "an emulator with that name already exists"

We could choose a name, see if it already exists, and suffix something to de-dup (my_name-2).

and then let them pick a device

I generally like to avoid asking the user for input on stdin. I think flutter emulators --create --name=my_device would be a reasonable UI, but am not fixed on it if you think there's a better way.

Ok, gone with this. We only add suffixes if we're generating the name, so if the user provides an explicit name we'll just give them an error:

dantup-macbookpro:flutter dantup$ flutter emulator --create Emulator 'flutter_emulator' created successfully. dantup-macbookpro:flutter dantup$ flutter emulator --create Emulator 'flutter_emulator_1' created successfully. dantup-macbookpro:flutter dantup$ flutter emulator --create --name flutter_emulator_1 Failed to create emulator 'flutter_emulator_1'. Error: Android Virtual Device 'flutter_emulator_1' already exists.

It feels a bit weird doing flutter emulator --create and not flutter emulator create but I think that's unavoidable as-is.

Do we have any convention for indicating when a flag is only usable in conjunction with another (eg. --name only makes sense with --create)? I couldn't see anything obvious.

Do we have any convention for indicating when a flag is only usable in conjunction with another (eg. --name only makes sense with --create)? I couldn't see anything obvious

Not that I know of; likely just something we should do w/ the help text for the flags.

I found some are prefixed with Used with flag --xxx. so I'll copy that.

devoncarew · 2018-06-11T15:00:38Z

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

We should print some text here about where they can find more info about creating emulator images (referencing flutter.io, or Android / Android Studio docs).

How's this? Seems a bit weird sending people to Android Studio or avdmanager docs but since the errors come from them I'm not sure if we can provide the info ourselves.

This looks great for error text. We'll probably also want something brief for flutter emulators -h help text as well - perhaps pointing to a flutter.io page?

@mit-mit, @mjohnsullivan - we may want to consider a brief landing page on flutter.io describing how to create android emulators, or at least just routing people to places to find for info about it.

I've tweaked this a little so the help text is output in more places (eg. at the bottom of the emulator list it gives you instruction for create and launch). If we add a page to the website we should swap out the URLs currently there for our own (and our web page can link to those others if required).

DanTup · 2018-06-12T16:25:13Z

This still badly needs some tests - assuming the general idea is ok I'll work on them this week.

devoncarew · 2018-06-13T23:14:05Z

This still badly needs some tests - assuming the general idea is ok I'll work on them this week.

Idea looks great!

DanTup · 2018-06-14T09:52:43Z

Ok, I've pushed some tests for this. It turned out to be a bit more involved than I expected - I had to make some changes to the existing code to use processManager (so it can be mocked) plus some other tweaks. I tried to split the commits up to make it easier to understand all the changes.

I don't know if this is all the best way to do this (I've not used Mockito before and I'm not sure of our preferences between mocked and real integration tests) but this basically mocks the process manager to return canned responses instead of calling real SDK commands (I wasn't sure how well that's work in the build environment). Please review carefully and nitpick away!

I'm going to do some additional manual testing on Windows shortly, but seemed worthwhile kicking the CI run off to get the results quicker.

(also rebased on master due to conflicts with the error changes merged yesterday)

This allows it to be tested in the EmulatorManager tests and also used by daemon later if desired.

Process calls are mocked to avoid needing a real SDK (and to be fast). Full integration tests may be useful, but may require ensuring all build environments etc. are set up correctly.

Previous changes were to emulatorPath!

reduce throws on an empty collection.

The name we attempted to use will now always be returned, even in the case of failure.

On Windows after installing Andriod Studio I didn't have any of these and got this message. Installing with sdkmanager fixed the issue.

runResult had a toString() but we moved to ProcessResult when switching to ProcessManager to this ended up throwing "Instance of ProcessResult".

There seemed to be issues using Lists in args with Mockito that I couldn't figure out (docs say to use typed() but I couldn't make this compile with these lists still)..

DanTup · 2018-06-14T12:59:54Z

Apparently this code had many issues! :(

I've fixed a bunch of bugs from testing on Windows and on the build servers (eg. due to the avd folder didn't exist) and improved some errors. We're touching a lot of stuff so I suspect we may find more edge cases at it runs in more places.

After this, I think it'd might be good to have a real end-to-end test that does something like:

create emulator
launch emulator
check a new device appeared in flutter devices
maybe even launch an app on it?

I'm not sure how fragile that might be (or how well the build infrastructure will support it - I'd guess the devicelab machines should be able to do it?) but it might be a good check.

DanTup · 2018-06-14T13:08:03Z

I seem to get a lot of issues like this on build servers:

00:18 +109: test/emulator_test.dart: EmulatorManager create emulator with an empty name does not fail
type 'MappedListIterable' is not a subtype of type 'List<int>' of 'availableApiVersions'

Yet get no warnings locally, even with --preview-dart-2 passed to VM and analyzer. Not sure if I'm doing something wrong?

devoncarew · 2018-06-26T16:11:07Z

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

    }
  }

+  Future<Null> _createEmulator(String name) async {


Shouldn't name be optional here?

I'll make it so, though it's not required since there's only one place that calls this and it pulls it from a map, so it's always passing an argument. It is valid for it to be null (and we'll auto-generate it) though, so I'll make it optional.

Cool, I think it just makes it a bit more explicit that it can be null (in the absence of non-null annotations).

devoncarew · 2018-06-26T16:13:14Z

packages/flutter_tools/lib/src/emulator.dart

+
+    final String device =  await _getPreferredAvailableDevice();
+    if (device == null)
+      return new CreateEmulatorResult(


nit: the formatting here looks off. 2 space indent? no trailing comma?

Since this is a newer file - added by you recently - you may just want to dartfmt it as a level-set.

Done (though I tweaked some of its changes where they seemed to make it less readable).

devoncarew · 2018-06-26T16:13:29Z

packages/flutter_tools/lib/src/emulator.dart

+        error: 'No device definitions are available'
+    );
+
+    final String sdkId =  await _getPreferredSdkId();


nit: two spaces before await.

devoncarew · 2018-06-26T16:14:40Z

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

    argParser.addOption('launch',
        help: 'The full or partial ID of the emulator to launch.');
+    argParser.addFlag('create',
+        help: 'Creates a new Android emulator.',


We may want to make this a bit longer; we could say that we'll try and create a pixel device, and perhaps include a goo.gl shortened link for more info (until we have a short link to a flutter.io page).

I added "based on a Pixel device" though not sure what more to add. Do we have somewhere reasonable to link to? The current links we have (in the help) go to pages on managing AVDs with the Android SDK which might not be totally appropriate here?

devoncarew · 2018-06-27T14:20:22Z

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

  }

-  Future<Null> _createEmulator(String name) async {
+  Future<Null> _createEmulator([String name]) async {


As a nit, I generally use named params.

googlebot added the cla: yes label Jun 6, 2018

DanTup mentioned this pull request Jun 6, 2018

Help new developers get a Flutter-compatible android emulator AVD created #13379

Closed

devoncarew reviewed Jun 11, 2018

View reviewed changes

DanTup added 24 commits June 14, 2018 10:54

Add --create option to flutter emulators

7598f99

Tweaks to error message

a893e4a

Simplify emulator search logic

85b462b

Make name optional

99f561c

Add a note about this option being used with --create

f5fc987

Tweaks to help information

528f847

Switch to processManager for easier testing

81f6d74

Don't crash on missing files or missing properties in Android Emulator

dde52e1

Move name suffixing into emulator manager

55a0a68

This allows it to be tested in the EmulatorManager tests and also used by daemon later if desired.

Pass the context's android SDK through so it can be mocked by tests

e6d0ff0

Misc fixes

e52d3a7

Add tests around emulator creation

929c931

Process calls are mocked to avoid needing a real SDK (and to be fast). Full integration tests may be useful, but may require ensuring all build environments etc. are set up correctly.

Simplify avdManagerPath

c3de637

Previous changes were to emulatorPath!

Fix lint errors

28fbdfd

Fix incorrect file exgtension for Windows

719772e

Fix an issue where no system images would crash

5e2cbaf

reduce throws on an empty collection.

Fix "null" appearing in error messages

a00c1b3

The name we attempted to use will now always be returned, even in the case of failure.

Add additional info to missing-system-image failure message

bab1b29

On Windows after installing Andriod Studio I didn't have any of these and got this message. Installing with sdkmanager fixed the issue.

Fix thrown errors

889681f

runResult had a toString() but we moved to ProcessResult when switching to ProcessManager to this ended up throwing "Instance of ProcessResult".

Fix package import

edc825f

Fix more package imports

80581cc

Move mock implementation into Mock class

c9a2102

There seemed to be issues using Lists in args with Mockito that I couldn't figure out (docs say to use typed() but I couldn't make this compile with these lists still)..

Rename method that's ambigious now we have create

b7ec8d1

Handle where there's no avd path

7dcaa9e

DanTup added 3 commits June 14, 2018 14:08

Add another toList() :(

a2a3bea

Remove comment that was rewritten

cf45403

Fix forbidden import

d8a06e6

devoncarew reviewed Jun 26, 2018

View reviewed changes

DanTup added 3 commits June 27, 2018 11:25

Make optional arg more obviously optional

dd4c6de

Reformat doc

361f29b

Note that we create a pixel device in help text

004187a

devoncarew reviewed Jun 27, 2018

View reviewed changes

devoncarew approved these changes Jun 27, 2018

View reviewed changes

Make this a named arg

0e1db73

DanTup merged commit cdb0118 into flutter:master Jun 28, 2018

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

Add --create option to flutter emulators command #18235

Add --create option to flutter emulators command #18235

Uh oh!

Conversation

DanTup commented Jun 6, 2018

Uh oh!

devoncarew 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanTup commented Jun 12, 2018

Uh oh!

devoncarew commented Jun 13, 2018

Uh oh!

DanTup commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanTup commented Jun 14, 2018

Uh oh!

DanTup commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Add --create option to `flutter emulators` command #18235

Add --create option to `flutter emulators` command #18235

DanTup commented Jun 14, 2018 •

edited

Loading

DanTup commented Jun 14, 2018 •

edited

Loading