Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore unreachable iOS devices in IOSDevice.getAttachedDevices #23776

Merged
merged 6 commits into from Nov 5, 2018
Merged

Ignore unreachable iOS devices in IOSDevice.getAttachedDevices #23776

merged 6 commits into from Nov 5, 2018

Conversation

mattijsf
Copy link
Contributor

@mattijsf mattijsf commented Oct 31, 2018

Fixes #23341

Related:

The usbmuxd ("USB multiplexer daemon") daemon manages the list of devices and facilitates communication with them. When Apple added support for WiFi backups and syncing, they did so by extending usbmuxd, so it is very much an intended behavior. I would like to see your proposed patch to usbmuxd_get_device_list as a configuration option, though. It would also be useful if the APIs in libimobiledevice allowed restricting connects to USB so it could selected per-connection.

In practice this means that idevice_id -l also lists udid's that are not actually attached via usb and have no debugging capabilities. When flutter tries to get device meta information using ideviceinfo -u UDID it returns an exit code 255 causing fluttor doctor and other flutter devices related API's to produce errors even though there are other attached devices / simulators that work fine.

This PR basically ignores devices for which ideviceinfo fails to receive meta information but still allow devices that do provide meta information to be included.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@mattijsf
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@raisezhang
Copy link

This is still not a solution to this problem ?

@mattijsf
Copy link
Contributor Author

mattijsf commented Nov 1, 2018

@raisezhang I'm not sure what you mean but once this PR is merged it should fix flutter doctor producing errors when there are paired iOS devices on the network that are not attached via a usb cable.

I wouldn't call it a solution but rather a workaround until idevice_id -l has a way to filter usb-only devices.

@Hixie
Copy link
Contributor

Hixie commented Nov 1, 2018

We should write a test for this. I believe the flutter_tools tests have the technology to mock processes which would help here.

@tvolkert
Copy link
Contributor

tvolkert commented Nov 1, 2018

We should write a test for this. I believe the flutter_tools tests have the technology to mock processes which would help here.

Yep. There's also some tests in

group('getAttachedDevices', () {
that make sue of these mocks too.

@mattijsf
Copy link
Contributor Author

mattijsf commented Nov 2, 2018

Thanks! I'll have a look. I'm somewhat new to Dart but it shouldn't be a problem to write some tests for this. I'll also have a look to catch the specific ideviceinfo's No device found with udid error instead of all error.

@mattijsf
Copy link
Contributor Author

mattijsf commented Nov 3, 2018

I added tests and specific exception handling. I chose to add a new IOSDeviceNotFoundError to keep the detection of this specific error closest to the invocation of the ideviceinfo command. Let me know if this is undesired as I could also catch the original ToolExit and put the logic inside getAttachedDevices() without a new Exception type.

Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM, pending one small change.

@tvolkert
Copy link
Contributor

tvolkert commented Nov 5, 2018

LGTM. Will merge on green.

@tvolkert tvolkert merged commit 54becbf into flutter:master Nov 5, 2018
@mattijsf mattijsf deleted the idevice_id-ignore-errored-devices branch November 9, 2018 07:30
@mattijsf mattijsf restored the idevice_id-ignore-errored-devices branch November 9, 2018 07:30
@mattijsf mattijsf deleted the idevice_id-ignore-errored-devices branch November 9, 2018 07:30
@mattijsf mattijsf restored the idevice_id-ignore-errored-devices branch November 9, 2018 07:30
@chunlea
Copy link

chunlea commented Nov 13, 2018

This PR finally saved me! 🍻

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter doctor crashes after updating ios-deploy and cocoapods
6 participants