flutter_tools: iOS wireless debugging support #58185

anirudhb · 2020-05-28T19:04:27Z

Description

This adds proper support for debugging wirelessly-connected devices on iOS. The toolchain seems to support deploying and connecting to devices over WiFi, so I've just removed the guards against wireless devices.

iOS devices now set the observatory to listen on all interfaces if they are network connected.

This is my first time contributing, please let me know if there's anything I need to fix!

Related Issues

Fixes #15072

Tests

None

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

anirudhb · 2020-05-29T02:29:10Z

Tests should be passing now.

jmagman

Thanks for your interest in this! The nslookup part is particularly clever.

I've been working up to this since:
#49854
#51015
#51678

and all the fallout thereof:
#50248
#53184
#53203
#58137

I had the device USB/network detection exposed on the iOS device part done on a branch, and I just put it out for review at #58257. It's pretty close to what you have (I even renamed the method getAvailableIOSDevices). The biggest difference is that it will avoid regressing #8309 for the 99% of users not on a wirelessly paired device. If you don't mind, I'd like to land that one, then you can rebase on it and we can review your change (plus it narrows the scope of your change).

My concerns so far are:

We would like to avoid regressing device discovery time for the 99% of users that are not using wireless debugging (2 seconds -> 5 seconds, even that 2 seconds increase was contentious). Once thought I had for this was to require specifying a --device-id for flutter run/install/screenshot/etc, trying the fast USB path first, and then dropping back to a slower discovery if it's not found.
This needs tests for the nslookup / port forwarding changes.
A change to the default --observatory-host behavior in flutter/engine#18651 could have all kinds of unexpected consequences for consumers of the observatory that aren't just this flutter tool. Why exactly did you need that change?

anirudhb · 2020-05-29T21:37:45Z

Thanks for leaving a review! To address your points:

In my personal tests, a 2-second timeout consistently resulted in the tool not detecting wirelessly-connected devices. I'm hoping your other PR (Change iOS device discovery from polling to long-running observation #58137) using xcdevice observe could help with this, as a fallback to wait for the device to connect (only for network connected devices, and grafted into the tool itself).
Not sure where to start about those tests, should I just copy the existing ones and adapt them?
The engine PR is required because by default the engine only listened on localhost, therefore making it only accessible via USB port forwarding. After looking into the iproxy source and conducting tests, I've determined you can only connect to the observatory on a wireless device if the observatory is exposed on all interfaces (i.e. you can directly connect with the IP, this had also contributed to my decision to just directly connect to the device instead of using iproxy). ~~There was absolutely no other way.~~ Edit: Maybe just the tool could pass a different observatory address, making that PR obsolete and keeping other consumers happy as well.

Additionally, that PR would really clean up my code a lot so I'd be interested in rebasing off it.

jmagman · 2020-05-29T23:42:40Z

In my personal tests, a 2-second timeout consistently resulted in the tool not detecting wirelessly-connected devices. I'm hoping your other PR (Change iOS device discovery from polling to long-running observation #58137) using xcdevice observe could help with this, as a fallback to wait for the device to connect (only for network connected devices, and grafted into the tool itself).

For sure, the timeout would have to be increased to detect wireless devices. Adding more seconds on discovery which would add that number of seconds to every flutter run command for the vast majority of users who aren't using wireless debugging. I was suggesting that, if the user passed in a -d device ID, then it would first try the faster path, but then if it's not found, try a longer timeout. This means

flutter run would not work with wirelessly paired devices, only flutter run -d <UDID>. That would have to be communicated somewhere (flutter run -h and maybe flutter devices).
flutter run would remain the same speed for most users that aren't using wirelessly paired devices.
There would be an additional cost to running with a wireless device since you'd always sit through the first failing device discovery.

This could be mitigated by more device property caching between flutter commands.

Not sure where to start about those tests, should I just copy the existing ones and adapt them?

I can look at this more after your rebase. We have plenty of technical debt to pay down in the tool testing category.

Edit: Maybe just the tool could pass a different observatory address, making that PR obsolete and keeping other consumers happy as well.

That's what I was thinking too. Check out locations where --observatory-port is already being passed in.

Additionally, that PR would really clean up my code a lot so I'd be interested in rebasing off it.

It's merged! Please rebase I can do a more thorough review.

anirudhb · 2020-05-30T00:19:23Z

Just finished rebasing off your PR. The engine PR is also now unnecessary (and has been closed) - resulting in me not needing to change a lot of tests.

If xcdevice times out trying to connect to a network device, it still gets returned in the list with a "network" type. If that's the only deployable device, would it be worth adding a xcdevice wait call to wait for the device to be connected? This would have no effect on USB users, and only happens when network devices can't connect. Additionally, this is also smoother for network users if it takes a little bit longer for their device to connect (past the timeout).

anirudhb · 2020-05-30T01:47:07Z

packages/flutter_tools/lib/src/protocol_discovery.dart

Nit: This feels kinda hacky. Is there any way to do this better?

anirudhb · 2020-06-03T20:19:56Z

@jmagman Rolled back 5 second timeout back to 2. I'm planning to address the disconnection issue using xcdevice wait in a later PR, after this is merged. Review?

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

Tests (should) be updated properly. Not sure if this is the best way to do it.

anirudhb · 2020-06-05T19:44:08Z

Added error handling by checking for Non-authoritative answer: which, from what I can tell, means that the device is not on the same WiFi network.

How would I write tests for this?

jmagman · 2020-06-25T20:28:18Z

Hi @anirudhb, sorry for the delay with this review. I'll try to look at it today!

jmagman · 2020-06-26T01:28:59Z

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

+      return null;
+    }
+    // 1st match is IPv6, last match is IPv4
+    final String ip = (ipv6 ? re.firstMatch(out) : re.allMatches(out).last).group(1).trim();


I'm still concerned about the unreliability of parsing this, cleaning the name up, etc. There's no tests for this parsing, and we don't have confidence the nslookup formatting won't change, or that we have a complete understanding of what else this output can look like.

I started working on #54781 to get the logs and observatory URL from the lldb logs (which will also give us crash-on-launch logs and anything else that was lost in the iOS 13 universal logging transition). Then we should be able to delete the FallbackDiscovery, and the iOS mDNS lookup code paths.

jmagman · 2020-06-26T01:30:32Z

Thank you so much for this contribution, we definitely know the community wants this feature.
I'm going to close this for now since I think we want to go the lldb attach path (if that all works out). If we wind up using the nslookup trick I'll definitely loop you back in!

anirudhb · 2020-06-26T01:41:29Z

nslookup is still necessary since the observatory URL grabbed from lldb logs will have a host of 0.0.0.0. It does get rid of the need for mDNS discovery though.

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 28, 2020

googlebot added the cla: yes label May 28, 2020

anirudhb mentioned this pull request May 28, 2020

observatory: Listen on all interfaces, not just localhost flutter/engine#18651

Closed

anirudhb force-pushed the wireless-debugging branch 3 times, most recently from b473550 to c8f12fd Compare May 28, 2020 20:46

zanderso requested a review from jmagman May 28, 2020 20:49

anirudhb force-pushed the wireless-debugging branch 7 times, most recently from 6b4d084 to 0497a67 Compare May 29, 2020 02:28

anirudhb force-pushed the wireless-debugging branch from 0497a67 to 0741a6b Compare May 29, 2020 02:43

jmagman requested changes May 29, 2020

View reviewed changes

anirudhb force-pushed the wireless-debugging branch from 0741a6b to f580bc5 Compare May 30, 2020 00:16

anirudhb requested a review from jmagman May 30, 2020 00:20

anirudhb force-pushed the wireless-debugging branch from f580bc5 to 4a9fc0b Compare May 30, 2020 00:23

anirudhb commented May 30, 2020

View reviewed changes

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

Copy link

Author

anirudhb May 30, 2020

Choose a reason for hiding this comment

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

Nit: This feels kinda hacky. Is there any way to do this better?

anirudhb force-pushed the wireless-debugging branch 3 times, most recently from 48e4060 to 06e5636 Compare June 3, 2020 20:18

jmagman reviewed Jun 4, 2020

View reviewed changes

packages/flutter_tools/lib/src/ios/devices.dart Outdated Show resolved Hide resolved

flutter_tools: Wireless debugging support

9a9d781

Tests (should) be updated properly. Not sure if this is the best way to do it.

anirudhb force-pushed the wireless-debugging branch from 06e5636 to 9a9d781 Compare June 5, 2020 19:38

anirudhb requested a review from jmagman June 5, 2020 19:44

jmagman reviewed Jun 26, 2020

View reviewed changes

jmagman closed this Jun 26, 2020

jmagman mentioned this pull request Jun 27, 2020

Turn on iOS wireless debugging, connect to remote observatory #60408

Closed

10 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021

flutter_tools: iOS wireless debugging support #58185

flutter_tools: iOS wireless debugging support #58185

Uh oh!

Conversation

anirudhb commented May 28, 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!

anirudhb commented May 29, 2020

Uh oh!

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

anirudhb commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented May 29, 2020

Uh oh!

anirudhb commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anirudhb May 30, 2020

Choose a reason for hiding this comment

Uh oh!

anirudhb commented Jun 3, 2020

Uh oh!

Uh oh!

anirudhb commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Jun 25, 2020

Uh oh!

jmagman Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

jmagman commented Jun 26, 2020

Uh oh!

anirudhb commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anirudhb commented May 28, 2020 •

edited

Loading

jmagman left a comment •

edited

Loading

anirudhb commented May 29, 2020 •

edited

Loading

anirudhb commented May 30, 2020 •

edited

Loading

anirudhb commented Jun 5, 2020 •

edited

Loading