Replace ideviceinfo and idevice_id with xcdevice #49854

jmagman · 2020-01-31T03:32:57Z

Description

Xcode ships with a command line tool called xcdevice that produces a json array of all available devices, including iOS devices that can be deployed via Xcode over the network (Connect via network checkbox). This PR filters out these network devices, but it's relevant to someday fixing #15072.

I checked to make sure xcdevice is available in Xcode 10.2 (minimum Xcode required by Flutter) and later.

Example output:

{
  "simulator" : false,
  "operatingSystemVersion" : "13.3 (17C54)",
  "interface" : "usb",
  "available" : true,
  "platform" : "com.apple.platform.iphoneos",
  "modelCode" : "iPhone8,1",
  "identifier" : "d83d5bc53967baa0ee18626ba87b6254b2ab5418",
  "architecture" : "arm64",
  "modelName" : "iPhone 6s",
  "name" : "iPhone"
}

Since this provides the identifier, name, operating system, and architecture, we can get rid of all usages of idevice_id and ideviceinfo (cleanup happening in the next PR since this one was already getting too big).

Also, show any connection errors in doctor instead of crashing. Example:

[!] Connected device                          
    • Error: iPhone is not paired with your computer. To use iPhone with Xcode, unlock it and choose to trust this computer when prompted. (code -9)

Remove 2 libimobiledevice dependencies.
Remove all codepaths that resulted in cryptic lockdown errors and instead return Xcode's nice errors and recovery suggestions, like "To use iPhone with Xcode, unlock it and choose to trust this computer when prompted."

Related Issues

Part of #6118
Precursor to #15072 (will make networked devices discoverable when we stop filtering "network" interfaces).

Fixes #49375
Fixes #36524
Fixes #47727
Fixes #43398
Fixes #48057

Supersedes #49626
Supersedes #45657
Supersedes part of #36902?

Tests

xcode_test, devices_test

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-01-31T03:35:21Z

packages/flutter_tools/lib/src/doctor.dart

It was more like 5 seconds in my tests, but let's be conservative...

christopherfujino · 2020-01-31T17:03:26Z

packages/flutter_tools/lib/src/doctor.dart

sp: "after with" -> "after which"

zanderso · 2020-01-31T16:42:52Z

packages/flutter_tools/lib/src/macos/xcode.dart

Redundant with the import of '../build_info.dart' below.

Dang I thought I fixed that. Android Studio loves automatically importing package:flutter_tools.

zanderso · 2020-01-31T17:33:12Z

packages/flutter_tools/lib/src/macos/xcode.dart

Why is this a Stream?

async* / yield requires this to be a stream. I can change it to async and to just return a List, if we prefer that pattern...

This could return a Stream if we're actually getting results bit-by-bit from xcdevice, or if this is implementing an interface that requires it to be a Stream, but otherwise we should probably prefer Future<List>, or Future<Iterable>.

(async* Streams also can have some surprising behavior. IIRC they don't interact well with FakeAsync for tests.)

zanderso · 2020-01-31T17:33:36Z

packages/flutter_tools/lib/src/macos/xcode.dart

Why is this a stream?

Same, async*.

zanderso · 2020-01-31T17:37:04Z

packages/flutter_tools/lib/src/macos/xcode.dart

Can we pass a smaller timeout if we know the user doesn't care about wireless debugging?

Said another way, if wireless device discovery was under a flag, could this number be smaller?

If the user doesn't care about networked devices and this is changed to --timeout 1 (faster than the default) , this returns the USB-tethered devices and none of the networked devices.

There's an xcdevice wait [--usb|--wifi|--both] [--timeout=sec] <udid> which will return as soon as the device is found but waits until the timeout if not, which would be fast for USB-tethered devices. That would make sense for flutter run -d <udid> hasSpecifiedDeviceId situations, though it would require PollingDeviceDiscovery to get refactored to take a "hint" identifier or something like that.

Maybe it would be okay for flutter doctor and flutter devices to take the full 5 seconds since we can then surface that it's networked and that they need to pass in -d <udid> to flutter run (attach, logs, screenshot, etc) on the device. And users would only be running those command infrequently, and they probably would prefer an accurate listing than for them to be fast.

Proposal:

Switch to xcdevice wait --both --timeout=5 for any command using -d <udid> (run, attach, logs, screenshot) to return fast if possible, but wait if needed.

When there's no -d <udid> or -d all assume USB-only (common case) and use the fastest timeout possible.

Keep the long timeout for flutter doctor and flutter devices and for "network" interface show a warning like "This device is available over the network and must be specified with `-d {}

Can the --timeout value be smaller than 1?

Can the --timeout value be smaller than 1?

1 seems to be the minimum allowed. 0 reverts to the longer default timeout.

christopherfujino

A few questions, but overall this is great!

packages/flutter_tools/lib/src/ios/devices.dart

christopherfujino · 2020-01-31T17:39:42Z

packages/flutter_tools/lib/src/macos/xcode.dart

Does this mean it will always take 5 seconds, even if all devices were found?

NVM, just read "cons" above. adding 5 seconds to the doctor seems costly, I wonder if we should make the extended timeout opt-in at first? Not sure how many users would be interested in this feature.

christopherfujino · 2020-01-31T18:25:22Z

Should we warn the user that connecting to the observatory is not supported at this time?

jonahwilliams · 2020-01-31T20:38:26Z

I would actually suggest not listing network devices until we have a way to connect to them

jmagman · 2020-01-31T20:47:28Z

Should we warn the user that connecting to the observatory is not supported at this time?

@christopherfujino This is a good idea, and detectible since we know it's a "network" interface.

I would actually suggest not listing network devices until we have a way to connect to them

@jonahwilliams That's what we do now, we just ignore them. I think it's better to at least let the user at least launch the app, even if they can't connect to the observatory, than to not ever be able to launch it at all. What about @christopherfujino's suggestion to print a message that networked devices don't support hot reload and just exit instead of crashing?

jmagman · 2020-01-31T20:50:36Z

I'm going to repurpose this issue to replace idevice_id and ideviceinfo with xcdevice for USB only, which is still a good direction, and address network devices discovery and port forwarding in future PRs instead of trying to do everything at once. I'll change this to purposely return fast and skip interfaces that aren't "usb".

zanderso · 2020-01-31T21:02:36Z

I'm going to repurpose this issue to replace idevice_id and ideviceinfo with xcdevice for USB only, ...

sgtm

jonahwilliams · 2020-01-31T21:03:25Z

I guess if we can still launch the app, that is definitely useful

jmagman · 2020-01-31T23:04:28Z

I'm going to repurpose this issue to replace idevice_id and ideviceinfo with xcdevice for USB only, which is still a good direction, and address network devices discovery and port forwarding in future PRs instead of trying to do everything at once. I'll change this to purposely return fast and skip interfaces that aren't "usb".

Done. PTAL!

zanderso · 2020-01-31T23:18:46Z

packages/flutter_tools/lib/src/macos/xcode.dart

Best practice is to hoist RegExp construction out of the loop.

zanderso · 2020-01-31T23:20:07Z

packages/flutter_tools/lib/src/macos/xcode.dart

Can the --timeout value be smaller than 1?

jmagman · 2020-02-01T02:18:22Z

Diagnostic messages I've seen while testing:

• Error: Jenn’s iPhone is busy: Preparing debugger support for iPhone Xcode will continue when Jenn’s iPhone is finished. (code -10)

• Error: Jenn’s iPhone has recently restarted Xcode will continue when Jenn’s iPhone is unlocked. (code -14)

cbracken

Overall looks pretty great -- I had no idea xcdevice existed!

Two quick questions:

do all versions of XCode we support include this tool? (if we only support 11, then we're set)
do they all emit compatible output?

cbracken · 2020-02-01T17:41:42Z

packages/flutter_tools/lib/src/macos/xcode.dart

This method is pretty long -- could a few smaller methods be factored out to break out? Something like some {sub,super,}set of:

fetch blob o' json (already done: _getAllDevices())

isIPhoneOSDevice(deviceProperties)

error message check (already done: _parseErrorMessage)

_isAvailable(...)

_isUSBDevice(...) (or alternatively _getConnectionType(...) == _DeviceConnectionType.usb)

get SDK version

get cpu arch

Good idea, done!

jmagman · 2020-02-03T18:53:06Z

Overall looks pretty great -- I had no idea xcdevice existed!

Two quick questions:

do all versions of XCode we support include this tool? (if we only support 11, then we're set)

do they all emit compatible output?

I tested on Xcode 10.1 and the output looked the same, though the error message text was sometimes different. I'll try on Xcode 10.2, which is our current minimum required version.

Of course there's nothing stopping Xcode from completely changing or even removing this tool in the next release...

zanderso · 2020-02-04T16:27:03Z

packages/flutter_tools/lib/src/macos/xcode.dart

Consider printing a trace message with the exception object so we can get some additional info from a --verbose run.

It looks like you may also want to pass throwOnError: true to runSync(). That will make runSync() throw a ProcessException if the xcrun exit code is non-zero.

zanderso · 2020-02-04T16:28:09Z

packages/flutter_tools/lib/src/macos/xcode.dart

Should this fail more loudly if the reason we can't find devices is that the tool isn't installed like we expect?

I've seen GitHub issues where people are trying to run -d chrome or for a particular Android device, and they are getting crashes because their personal iPhone is plugged in but isn't set up for development. I don't think we should make people have Xcode installed if they aren't doing iOS development, and we can't really tell what their intentions are at the point we're trying to find a particular device identifier.

Ah, okay, yeah that makes sense.

zanderso · 2020-02-04T16:34:44Z

packages/flutter_tools/lib/src/macos/xcode.dart

It looks like this is called shortly after an empty return from getAllConnectedDevices(). Would it make sense to cache the result of _getAllDevices() to avoid shelling out to xcdevice again?

jmagman · 2020-02-05T23:11:56Z

weird error on customer_testing tests:

Cloning into 'bin/cache/pkg/tests'...
dart --enable-asserts dev/customer_testing/run_tests.dart --skip-on-fetch-failure --skip-template bin/cache/pkg/tests/registry/*.test
ERROR: Could not parse: ./bin/cache/pkg/tests/registry/flutter_packages.test
First "fetch" directive does not match expected pattern (expected "git clone https://github.com/USERNAME/REPOSITORY.git tests").

Rebased onto master to resolve.

This reverts commit 3aa7a80.

…50243) This reverts commit 3aa7a80.

jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Jan 31, 2020

jmagman requested review from cbracken, christopherfujino and zanderso January 31, 2020 03:32

jmagman self-assigned this Jan 31, 2020

googlebot added the cla: yes label Jan 31, 2020

jmagman force-pushed the xcdevice-discovery branch from 0ee695b to 456e43b Compare January 31, 2020 03:34

jmagman commented Jan 31, 2020

View reviewed changes

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

Copy link

Member Author

jmagman Jan 31, 2020

Choose a reason for hiding this comment

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

It was more like 5 seconds in my tests, but let's be conservative...

christopherfujino reviewed Jan 31, 2020

View reviewed changes

zanderso reviewed Jan 31, 2020

View reviewed changes

christopherfujino reviewed Jan 31, 2020

View reviewed changes

jmagman force-pushed the xcdevice-discovery branch from 456e43b to 3b1ffbd Compare January 31, 2020 22:27

jmagman changed the title ~~Discover wirelessly paired iOS devices, replace ideviceinfo and idevice_id with xcdevice~~ Replace ideviceinfo and idevice_id with xcdevice Jan 31, 2020

zanderso reviewed Jan 31, 2020

View reviewed changes

cbracken reviewed Feb 1, 2020

View reviewed changes

jmagman force-pushed the xcdevice-discovery branch from 75f10c3 to 3ac412f Compare February 3, 2020 23:53

zanderso reviewed Feb 4, 2020

View reviewed changes

jmagman force-pushed the xcdevice-discovery branch from 3ac412f to 0e229d2 Compare February 4, 2020 20:57

dnfield mentioned this pull request Feb 5, 2020

WIP: Handle untrusted mac host #36902

Closed

9 tasks

jmagman added 7 commits February 5, 2020 15:08

Append period

f4af721

anonymize path

1bc591a

Refactor device property parsing

4b53d1a

Run xcrun xcdevice instead of xcdevice directly

d71a277

Add cache

c3926a6

throwOnError

813396d

Add FormatException

0aeb10a

jmagman force-pushed the xcdevice-discovery branch from eaf12b6 to 0aeb10a Compare February 5, 2020 23:08

Remove FormatException catch

261b285

jmagman added the waiting for tree to go green label Feb 6, 2020

jmagman merged commit 3aa7a80 into flutter:master Feb 6, 2020

jmagman deleted the xcdevice-discovery branch February 6, 2020 01:37

jmagman added a commit that referenced this pull request Feb 6, 2020

Revert "Replace ideviceinfo and idevice_id with xcdevice (#49854)"

f5c40e1

This reverts commit 3aa7a80.

jmagman mentioned this pull request Feb 6, 2020

Revert "Replace ideviceinfo and idevice_id with xcdevice" #50243

Merged

jmagman added a commit that referenced this pull request Feb 6, 2020

Revert "Replace ideviceinfo and idevice_id with xcdevice (#49854)" (#…

ce6fbf6

…50243) This reverts commit 3aa7a80.

jmagman mentioned this pull request Feb 6, 2020

Remove ideviceinfo, idevice_id artifacts #50248

Merged

10 tasks

jmagman restored the xcdevice-discovery branch February 6, 2020 02:22

This was referenced Feb 6, 2020

Reland Replace ideviceinfo and idevice_id with xcdevice #50252

Merged

Refactor IMobileDevice to be context free #49626

Closed

This was referenced Feb 19, 2020

Discover devices in parallel instead of serially waiting for each device type #51015

Merged

"iproxy cannot be opened because the developer cannot be verified" #42969

Closed

jmagman mentioned this pull request Mar 6, 2020

Can't run flutter app on iphone using IntelliJ nor flutter run on command line but works using Xcode build #29495

Closed

jmagman mentioned this pull request Mar 23, 2020

flutter doctor crashes with iOS simulator #53019

Closed

This was referenced May 29, 2020

Detect USB/network interface from iOS devices #58257

Merged

flutter run should support wireless debugging of iOS devices #15072

Closed

jmagman mentioned this pull request Jul 1, 2020

flutter devices doesn't show my iPhone after upgrade to flutter 1.17.4 #60556

Closed

jmagman mentioned this pull request Jul 16, 2020

Show device diagnostic messages in doctor #61585

Merged

10 tasks

jmagman mentioned this pull request Mar 3, 2021

Remove outdated lockdownd error note flutter/website#5388

Merged

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

Replace ideviceinfo and idevice_id with xcdevice #49854

Replace ideviceinfo and idevice_id with xcdevice #49854

Uh oh!

Conversation

jmagman commented Jan 31, 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!

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!

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

christopherfujino commented Jan 31, 2020

Uh oh!

jonahwilliams commented Jan 31, 2020

Uh oh!

jmagman commented Jan 31, 2020

Uh oh!

jmagman commented Jan 31, 2020

Uh oh!

zanderso commented Jan 31, 2020

Uh oh!

jonahwilliams commented Jan 31, 2020

Uh oh!

jmagman commented Jan 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Feb 1, 2020

Uh oh!

cbracken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jmagman commented Feb 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Jan 31, 2020 •

edited

Loading

cbracken left a comment •

edited

Loading

jmagman Feb 4, 2020 •

edited

Loading