Skip to content

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Jun 27, 2020

Description

Use new --network libimobiledevice flags.

flutter run, flutter attach --host-vmservice-port, flutter install, flutter screenshot work.

Related issues

#15072

Steps to test

  1. Attach a phone to your Mac. This works best when both are on the same Wi-Fi network (unrelated to Flutter).
  2. Make sure the device has a passcode set.
  3. Open Xcode > Window > Devices and Simulators > Choose your phone, then check Connect via Network
    Screen Shot 2020-06-26 at 7 40 35 PM
    (If you don't get that little network icon next to the device name it probably didn't work, regardless of checkbox state.)
  4. Unplug your device from USB.

Tests

  • mac_test.dart
  • xcode_test.dart

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

  • 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 jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 27, 2020
@jmagman jmagman self-assigned this Jun 27, 2020
@jmagman
Copy link
Member Author

jmagman commented Jun 27, 2020

\cc @zanderso @DanTup @dnfield @jonahwilliams Any insight on this one?

Edit: @anirudhb figured it out and I updated with the suggested change. Please ignore. 🙂

@jmagman
Copy link
Member Author

jmagman commented Jun 27, 2020

@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 iproxy handles the IP lookup for us, so we don't need to do any stdout parsing). I also got "Connection refused" issues when I ran your PR, though it may have been because the IP wasn't parsed correctly on my machine, and even when the host and target were on the same network I still get the Non-authoritative output (maybe a hint?).

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?

@anirudhb
Copy link

I didn’t try anything yet but glancing at the observatory logs it seems you forgot to pass —observatory-host 0.0.0.0 to the app arguments.

@jmagman
Copy link
Member Author

jmagman commented Jun 27, 2020

I didn’t try anything yet but glancing at the observatory logs it seems you forgot to pass —observatory-host 0.0.0.0 to the app arguments.

@anirudhb Good eye, that was totally it. Thank you so much!

@jmagman jmagman force-pushed the wireless-network-flag branch from 4c8e1fe to 6dc3f9b Compare June 30, 2020 20:32
@anirudhb
Copy link

anirudhb commented Jul 7, 2020

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.)

@jmagman
Copy link
Member Author

jmagman commented Jul 8, 2020

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 1.10.0-beta.3), but we'll need to upgrade soon to support Xcode 12 (see #60072 (comment)). We can explore that separately since it'll help non-wireless install times as well.

@jmagman jmagman force-pushed the wireless-network-flag branch from 6dc3f9b to 5e97165 Compare July 8, 2020 21:55
@anirudhb
Copy link

anirudhb commented Jul 11, 2020

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.

@jmagman
Copy link
Member Author

jmagman commented Jul 11, 2020

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.

@anirudhb
Copy link

anirudhb commented Jul 11, 2020

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, Xcode was successfully able to use app deltas! edit: Crazy run errors including a white screen on launch.

A proper implementation of this would probably involve hash comparisons, since Xcode doesn't do this for some reason when computing app deltas.

@jmagman jmagman force-pushed the wireless-network-flag branch from 5e97165 to 7536906 Compare August 28, 2020 23:40
@jmagman
Copy link
Member Author

jmagman commented Aug 29, 2020

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.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@jmagman jmagman marked this pull request as ready for review September 2, 2020 19:12
@jmagman jmagman force-pushed the wireless-network-flag branch from 7536906 to 3e43366 Compare September 2, 2020 22:40
'--disable-service-auth-codes',
'--observatory-port=$assumedObservatoryPort',
if (interfaceType == IOSDeviceInterface.network)
'--observatory-host=${ipv6 ? '::/0' : '0.0.0.0'}',
Copy link
Member

Choose a reason for hiding this comment

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

add comment for reader

Copy link
Member Author

Choose a reason for hiding this comment

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

@required String id,
@required IProxy iproxy,
@required OperatingSystemUtils operatingSystemUtils,
@required IOSDeviceInterface interfaceType,
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@jmagman
Copy link
Member Author

jmagman commented Sep 3, 2020

Not working on iOS 14 again...

@christopherfujino
Copy link
Contributor

I guess re-requesting a review doesn't remove an approval, as far as GitHub is concerned?

Copy link
Contributor

@christopherfujino christopherfujino left a 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.

@flutter-dashboard
Copy link

This pull request has not been updated in a while. Please update this pull request to receive results from Gold, or close it.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • This pull request has changes requested by @christopherfujino. Please resolve those before re-applying the label.
  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@christopherfujino
Copy link
Contributor

Just trying to clear out my previous approval...in case the mergebot goes crazy again and tries to merge it.

I look like a genius now :)

@jmagman
Copy link
Member Author

jmagman commented Apr 22, 2021

This is well out of date, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants