Implement mdns for `flutter run` by christopherfujino · Pull Request #40447 · flutter/flutter

christopherfujino · 2019-09-13T18:25:29Z

Description

Fixes the flutter tool not attaching to app observatory services on ios13 by implementing mDNS for flutter run (previously the tool would discover the observatory URI via reading the logs). If mDNS fails, the tool will fallback to the old method.

Related Issues

Fixes #40448

Tests

I added the following tests:

Testing both mDNS discovery, and the fallback log scanning.

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-09-13T18:25:30Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

codecov · 2019-09-17T20:47:52Z

Codecov Report

Merging #40447 into master will decrease coverage by 0.76%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##           master   #40447      +/-   ##
==========================================
- Coverage   59.46%    58.7%   -0.77%     
==========================================
  Files         192      194       +2     
  Lines       18697    18735      +38     
==========================================
- Hits        11119    10998     -121     
- Misses       7578     7737     +159

Flag	Coverage Δ
#flutter_tool	`58.7% <70.73%> (-0.77%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/context_runner.dart	`70.9% <0%> (-1.32%)`	⬇️
...ackages/flutter_tools/lib/src/commands/attach.dart	`73.68% <62.5%> (-0.79%)`	⬇️
packages/flutter_tools/lib/src/mdns_discovery.dart	`69.84% <69.84%> (ø)`
packages/flutter_tools/lib/src/ios/devices.dart	`63.24% <90%> (+0.63%)`	⬆️
packages/flutter_tools/lib/src/bundle.dart	`31.81% <0%> (-43.94%)`	⬇️
...kages/flutter_tools/lib/src/commands/precache.dart	`50% <0%> (-41.67%)`	⬇️
...s/flutter_tools/lib/src/tester/flutter_tester.dart	`32.98% <0%> (-38.15%)`	⬇️
...flutter_tools/lib/src/android/android_builder.dart	`12.5% <0%> (-34.38%)`	⬇️
packages/flutter_tools/lib/src/web/web_device.dart	`20.51% <0%> (-25.65%)`	⬇️
...ackages/flutter_tools/lib/src/commands/config.dart	`72% <0%> (-14.67%)`	⬇️
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dd8c3a...9333fab. Read the comment docs.

…tocol discovery fail

zanderso

lgtm w/ nits, but also wait for dnfield's review.

zanderso · 2019-09-17T22:18:54Z

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

+          package.id,
+          this,
+          ipv6,
+          debuggingOptions.observatoryPort


nit: missing comma

added comma

zanderso · 2019-09-17T22:21:45Z

packages/flutter_tools/lib/src/mdns_discovery.dart

+  /// The [MDnsClient] used to do a lookup.
+  final MDnsClient client;
+
+  static const String dartObservatoryName = '_dartobservatory._tcp.local';


Does this need to be public? Could it be @visibleForTesting?

added @visibleForTesting

dnfield

This LGTM, would defer to @zanderso for final approval - in particular, do we want any analytics going on in here? How often mDNS fails, how long it takes?

Also, can you update the pull request message template? :)

dnfield · 2019-09-17T22:23:18Z

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

+      }
+
+      // Fallback to manual protocol discovery
+      printTrace('mDNS lookup failed, attempting fallback protocol discovery.');


nit: Maybe just clarify that this is going to try to scan logs in the message.

zanderso

still lgtm

Bump version of just package:multicast_dns (#41207) Implement mdns for flutter run (#40447)

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 13, 2019

googlebot added the cla: yes label Sep 13, 2019

christopherfujino mentioned this pull request Sep 13, 2019

flutter run doesn't attach to observatory service on ios13 #40448

Closed

christopherfujino added 4 commits September 16, 2019 13:04

get run working on ios13, with a lot of TODOs left

6741896

remove unneccessary comment

af6eafb

remove debugging echo

2f5a61d

refactor attach to use same file

f872afe

christopherfujino force-pushed the implement-mdns-for-run branch from b879271 to f872afe Compare September 16, 2019 20:05

christopherfujino added 11 commits September 16, 2019 13:40

respect observatory-port command line arg

237fb23

add missing import

5d6a29b

add fallback manual observatory discovery

81a56f3

comment out obsolete tests

c5d2aed

rename file observatory_discovery -> mdns_discovery

7538595

remove obsolete import

c027c0e

start log ready from beginning, before mDNS attempt

00d5daa

move MDnsObservatoryDiscovery to context and override for testing

d7e7fa9

fix import

35dbfca

move tests of MDnsObservatoryDiscovery to its own file

cebde63

add a test to attach_test.dart to verify mdns lookup works

5d75a09

christopherfujino added 3 commits September 17, 2019 13:54

fix unused import

d7d790c

add test to verify failure LaunchResult when both mDNS and manual pro…

0c31c92

…tocol discovery fail

remove bad test

e29c6f1

christopherfujino marked this pull request as ready for review September 17, 2019 22:12

christopherfujino requested review from dnfield and zanderso September 17, 2019 22:12

remove unused mock

6526ba8

dnfield added the platform-ios iOS applications specifically label Sep 17, 2019

zanderso approved these changes Sep 17, 2019

View reviewed changes

dnfield approved these changes Sep 17, 2019

View reviewed changes

code review

9333fab

zanderso approved these changes Sep 18, 2019

View reviewed changes

christopherfujino merged commit 0b24a5a into flutter:master Sep 18, 2019

christopherfujino deleted the implement-mdns-for-run branch September 18, 2019 18:01

christopherfujino mentioned this pull request Sep 19, 2019

ios-deploy will not launch to ios13 device with "Device doesn't support wireless sync" #37699

Closed

zanderso mentioned this pull request Sep 23, 2019

UTF8 format exception from mDNS #41140

Closed

tvolkert mentioned this pull request Sep 24, 2019

V1.9.1 hotfixes #41229

Merged

tvolkert added a commit that referenced this pull request Sep 25, 2019

V1.9.1 hotfixes (#41229)

a72edc2

Bump version of just package:multicast_dns (#41207) Implement mdns for flutter run (#40447)

christopherfujino mentioned this pull request Sep 30, 2019

change logging in mDNS discovery to verbose-mode only #41621

Merged

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Implement mdns for flutter run (flutter#40447)

b4d37e0

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mdns for `flutter run`#40447

Implement mdns for `flutter run`#40447
christopherfujino merged 20 commits intoflutter:masterfrom
chris-forks:implement-mdns-for-run

christopherfujino commented Sep 13, 2019 •

edited

Loading

Uh oh!

fluttergithubbot commented Sep 13, 2019

Uh oh!

codecov bot commented Sep 17, 2019 •

edited

Loading

Uh oh!

zanderso left a comment

Uh oh!

zanderso Sep 17, 2019

Uh oh!

christopherfujino Sep 17, 2019

Uh oh!

zanderso Sep 17, 2019

Uh oh!

christopherfujino Sep 17, 2019

Uh oh!

dnfield left a comment

Uh oh!

dnfield Sep 17, 2019

Uh oh!

christopherfujino Sep 17, 2019

Uh oh!

zanderso left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

christopherfujino commented Sep 13, 2019 • 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!

fluttergithubbot commented Sep 13, 2019

Uh oh!

codecov bot commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

christopherfujino Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

zanderso Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

christopherfujino Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

christopherfujino Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christopherfujino commented Sep 13, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading