-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Turn on iOS wireless debugging, connect to remote observatory #60408
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
Conversation
|
\cc @zanderso @DanTup @dnfield @jonahwilliams Any insight on this one? Edit: @anirudhb figured it out and I updated with the suggested change. Please ignore. 🙂 |
|
@anirudhb Were you ever actually able to get #58185 working and attaching to the observatory? (Thank you again for that contribution! Fortunately the newest version of If you did have it working on your PR, would you mind testing this PR with the same setup? Did you have to set up anything special to get it working? |
|
I didn’t try anything yet but glancing at the observatory logs it seems you forgot to pass |
@anirudhb Good eye, that was totally it. Thank you so much! |
4c8e1fe to
6dc3f9b
Compare
|
Regarding the performance of this, it seems that install always copies all files to the device (including Flutter.framework) which is unreasonably large and slow. Have you looked into making Flutter's build work with app deltas? (This doesn't work currently since the build script seems to be touching Flutter.framework, forcing it to be copied.) |
The version of ios-deploy we're embedding doesn't support app-deltas yet (we're back on |
6dc3f9b to
5e97165
Compare
|
fwiw I did try the new ios-deploy with app deltas, but it seems that Flutter builds the app in such a way that app deltas copies the entire app again (including Flutter.framework, which shouldn't've even changed.) The app install time is extremely noticeable over wireless, whereas over USB it's not as significant. |
That doesn't surprise me, we scorch earth that framework on every build because people kept submitting the debug version to the App Store, which gets at best gets a rejection and at worst causes a 100% crash on launch. I've hopefully plugged enough holes with that logic that maybe we can be less aggressive, but we'll have to investigate further. I'm comfortable turning this feature on even if it's painfully slow, and we can improve later. |
|
I did some investigation on this, and the gist of it is that Xcode only uses modification time to determine whether to copy a file or not. Some things in the build that change modification time include code signing and framework thinning. Cocoapods additionally does this as well (this could be an upstream issue). After modifying the scripts to preserve modification time across codesign/thin, A proper implementation of this would probably involve hash comparisons, since Xcode doesn't do this for some reason when computing app deltas. |
5e97165 to
7536906
Compare
|
I removed the longer timeouts in favor of a new flag #64834. With both this change and that one, it's working on iOS 13 and Xcode 11. Unfortunately I'm hitting debug attach issues on Xcode 12/iOS 14. |
|
This pull request is not suitable for automatic merging in its current state.
|
7536906 to
3e43366
Compare
| '--disable-service-auth-codes', | ||
| '--observatory-port=$assumedObservatoryPort', | ||
| if (interfaceType == IOSDeviceInterface.network) | ||
| '--observatory-host=${ipv6 ? '::/0' : '0.0.0.0'}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment for reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see flutter/engine#9485.
| @required String id, | ||
| @required IProxy iproxy, | ||
| @required OperatingSystemUtils operatingSystemUtils, | ||
| @required IOSDeviceInterface interfaceType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultra nit / feel free to ignore: IOSDeviceConnectionInterface? Just interface is a bit vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
xster
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
christopherfujino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Not working on iOS 14 again... |
|
I guess re-requesting a review doesn't remove an approval, as far as GitHub is concerned? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to clear out my previous approval...in case the mergebot goes crazy again and tries to merge it.
|
This pull request has not been updated in a while. Please update this pull request to receive results from Gold, or close it. |
|
This pull request is not suitable for automatic merging in its current state.
|
I look like a genius now :) |
|
This is well out of date, closing. |
Description
Use new
--networklibimobiledevice flags.flutter run,flutter attach --host-vmservice-port,flutter install,flutter screenshotwork.Related issues
#15072
Steps to test
(If you don't get that little network icon next to the device name it probably didn't work, regardless of checkbox state.)
Tests
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change