Support mdns when attaching to proxied devices. #146021

chingjun · 2024-03-29T23:41:13Z

Also move the vm service discovery logic into platform-specific implementation of Devices. This is to avoid having platform-specific code in attach.dart.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Also move the vm service discovery logic into platform-specific implementation of `Device`s. This is to avoid having platform-specific code in attach.dart.

christopherfujino · 2024-04-08T19:27:29Z

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

+          },
+          onError: (Object e) {
+            // Daemon throws string types.
+            if (e is String && e.contains('command not understood')) {


Is it safe to ignore other errors here?

Good point. Forwarded the error to the stream instead.

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

vashworth · 2024-04-08T23:43:45Z

packages/flutter_tools/lib/src/commands/attach.dart

+        timeout: const Duration(seconds: 30),
+        slowWarningCallback: () {
+          // If relying on mDNS to find Dart VM Service, remind the user to allow local network permissions.
+          if (vmServiceDiscovery.usesMdns) {


This status message is only relevant to iOS devices.

Suggested change

if (vmServiceDiscovery.usesMdns) {

if (vmServiceDiscovery.usesMdns && _isIOSDevice(device)) {

You made me realize that I misunderstood the comment and the message. I thought it was asking me to click "Allow" on the mac, when the dart binary wants to access network. It makes a lot more sense now.

In that case, usesMdns is not actually needed, I've removed that field. I also updated the comment and the message slightly to be clear that the permission.

Also updated _isIOSDevice below to check for device.platformType == PlatformType.ios instead of checking the type, to better support proxied devices.

PTAL, thanks!

vashworth · 2024-04-09T00:28:22Z

packages/flutter_tools/lib/src/fuchsia/fuchsia_device.dart

+    } on Exception {
+      isolateDiscoveryProtocol?.dispose();
+      final List<ForwardedPort> ports = portForwarder.forwardedPorts.toList();
+      ports.forEach(portForwarder.unforward);


So previously, it was

flutter/packages/flutter_tools/lib/src/commands/attach.dart

Lines 309 to 311 in 137cb4b

for (final ForwardedPort port in ports) {

await device.portForwarder.unforward(port);

}

Does putting it in a forEach wait for each?

On re-reading the code, the try-catch was not added in the right place. I have moved it to the Stream<Uri> get uris getter below, and added the await back.

Thanks.

christopherfujino

This LGTM but I'll defer to Victoria for final approval (she knows this code the best)

Removed `usesMdns`, and restore platform checking in attach.dart when showing the message asking user to allow network access on device. Fix error handling in FuchsiaDevice.

vashworth

LGTM!

vashworth · 2024-04-09T14:21:27Z

Analyzer issue in test

Unused import: '../ios/simulators.dart' • packages/flutter_tools/lib/src/commands/attach.dart:26:8 • unused_import

chingjun · 2024-04-09T18:35:35Z

Analyzer issue in test

Unused import: '../ios/simulators.dart' • packages/flutter_tools/lib/src/commands/attach.dart:26:8 • unused_import

Oops, missed that one. Fixed, thanks for the review!

…ter#146021)

flutter/flutter@4967a94...97cd47a 2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541) 2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538) 2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531) 2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527) 2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528) 2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488) 2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524) 2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021) 2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570) 2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515) 2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Also move the vm service discovery logic into platform-specific implementation of `Device`s. This is to avoid having platform-specific code in attach.dart.

flutter/flutter@4967a94...97cd47a 2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541) 2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538) 2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531) 2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527) 2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528) 2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488) 2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524) 2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021) 2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570) 2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515) 2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Mar 29, 2024

Support mdns when attaching to proxied devices.

13d0145

Also move the vm service discovery logic into platform-specific implementation of `Device`s. This is to avoid having platform-specific code in attach.dart.

chingjun force-pushed the discovery-for-attach branch from 7f482ca to 13d0145 Compare April 2, 2024 00:18

chingjun requested a review from christopherfujino April 2, 2024 01:08

chingjun marked this pull request as ready for review April 2, 2024 01:08

vashworth self-requested a review April 3, 2024 21:31

christopherfujino reviewed Apr 8, 2024

View reviewed changes

packages/flutter_tools/lib/src/proxied_devices/devices.dart Show resolved Hide resolved

Add error handling based on reviewer comments

5541b8d

chingjun force-pushed the discovery-for-attach branch from 080de5a to 5541b8d Compare April 8, 2024 22:53

vashworth reviewed Apr 9, 2024

View reviewed changes

christopherfujino reviewed Apr 9, 2024

View reviewed changes

Updates based on review feedback.

b6e0e07

Removed `usesMdns`, and restore platform checking in attach.dart when showing the message asking user to allow network access on device. Fix error handling in FuchsiaDevice.

vashworth approved these changes Apr 9, 2024

View reviewed changes

Fix analyzer error

1fd9d3e

vashworth approved these changes Apr 9, 2024

View reviewed changes

chingjun added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024

auto-submit bot merged commit 8004212 into flutter:master Apr 9, 2024

engine-flutter-autoroll mentioned this pull request Apr 10, 2024

Roll Flutter from 4967a94cd907 to 74b65d4e4c61 (10 revisions) flutter/packages#6496

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024

8004212 Support mdns when attaching to proxied devices. (flutter/flut…

3b3eeac

…ter#146021)

engine-flutter-autoroll mentioned this pull request Apr 10, 2024

Roll Flutter from 4967a94cd907 to 97cd47a02e60 (11 revisions) flutter/packages#6497

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024

8004212 Support mdns when attaching to proxied devices. (flutter/flut…

f4b4859

…ter#146021)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support mdns when attaching to proxied devices. #146021

Support mdns when attaching to proxied devices. #146021

Uh oh!

chingjun commented Mar 29, 2024 •

edited

Loading

Uh oh!

christopherfujino Apr 8, 2024

Uh oh!

chingjun Apr 8, 2024

Uh oh!

Uh oh!

vashworth Apr 8, 2024

Uh oh!

chingjun Apr 9, 2024

Uh oh!

vashworth Apr 9, 2024

Uh oh!

chingjun Apr 9, 2024

Uh oh!

christopherfujino left a comment

Uh oh!

vashworth left a comment

Uh oh!

vashworth commented Apr 9, 2024

Uh oh!

chingjun commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (vmServiceDiscovery.usesMdns) {
	if (vmServiceDiscovery.usesMdns && _isIOSDevice(device)) {

	for (final ForwardedPort port in ports) {
	await device.portForwarder.unforward(port);
	}

Support mdns when attaching to proxied devices. #146021

Support mdns when attaching to proxied devices. #146021

Uh oh!

Conversation

chingjun commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

christopherfujino Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

chingjun Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vashworth Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

chingjun Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

vashworth Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

chingjun Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

vashworth left a comment

Choose a reason for hiding this comment

Uh oh!

vashworth commented Apr 9, 2024

Uh oh!

chingjun commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chingjun commented Mar 29, 2024 •

edited

Loading