Skip to content

Add publish-port flag to disable mDNS port discovery#67452

Merged
fluttergithubbot merged 6 commits intoflutter:masterfrom
jmagman:disable-observatory
Oct 7, 2020
Merged

Add publish-port flag to disable mDNS port discovery#67452
fluttergithubbot merged 6 commits intoflutter:masterfrom
jmagman:disable-observatory

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 6, 2020 •
edited
Loading

Description

Add a publish-port flag to run and drive. Default to on for run and off for drive.
This will tell the engine to not use mDNS to publish the observatory port, which means the local network permission popup won't be blocking the app on drive on iOS 14, which is particularly important in a CI environment.
As of #66092 log scanning should be sufficient to parse the port without the mDNS fallback, so remove it.

Related Issues

Framework part of #65207
Engine launch flag PR at flutter/engine#21632

Tests

drive_test, fallback_discovery_test, ios_device_start_prebuilt_test.
Manual testing showed flutter drive didn't show the permission prompt on iOS 14, and the tests passed.

Checklist

  • 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 the platform-ios iOS applications specifically label Oct 6, 2020
@jmagman jmagman self-assigned this Oct 6, 2020
@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 6, 2020
jonahwilliams
jonahwilliams reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

If the log scanning is sufficient, why do we need it for run? Removing the mDNS logic from everything but attach would be a tremendous simplification.

packages/flutter_tools/lib/src/runner/flutter_command.dart Outdated
argParser.addFlag('publish-observatory-port',
negatable: true,
hide: !verboseHelp,
help: 'Publish the observatory URL over mDNS. Disable to prevent the'
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 6, 2020

Choose a reason for hiding this comment

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

Observatory or VM Service ?

Copy link
Member Author

@jmagman jmagman Oct 6, 2020

Choose a reason for hiding this comment

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

Oh you. I'm still torn whether consistency or accuracy is better, considering in the engine it's called FlutterObservatoryPublisher etc. I'll reword.

@jmagman jmagman mentioned this pull request Oct 6, 2020
Merged
9 tasks
@jmagman
Copy link
Member Author

jmagman commented Oct 6, 2020

If the log scanning is sufficient, why do we need it for run? Removing the mDNS logic from everything but attach would be a tremendous simplification.

I'm willing to give it a try, let's see if we get more failure reports.

@jmagman
Copy link
Member Author

jmagman commented Oct 6, 2020

If the log scanning is sufficient, why do we need it for run?

I think we still want it to launch and publish over mDNS with run so you could detach and then attach later (even if the tool doesn't try to discover it on run). Particularly since you can't launch directly from the home screen on iOS 14 anymore.

@jmagman jmagman force-pushed the disable-observatory branch from ed6435f to 8c5cd34 Compare October 6, 2020 23:20
jonahwilliams
jonahwilliams reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Can you document the reasoning for leaving it enabled on run?

jonahwilliams
jonahwilliams approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams 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 jmagman changed the title Add publish-observatory-port flag to disable mDNS port discovery Add publish-port flag to disable mDNS port discovery Oct 6, 2020
@fluttergithubbot fluttergithubbot merged commit f8b1de3 into flutter:master Oct 7, 2020
zanderso
zanderso approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm w/ question

packages/flutter_tools/lib/src/ios/fallback_discovery.dart
flutterUsage: _flutterUsage,
).send();

try {
Copy link
Member

@zanderso zanderso Oct 7, 2020

Choose a reason for hiding this comment

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

To make sure I understand, this PR just removes mDNS discovery from fallback discovery, but it will still be used when needed from e.g. flutter attach?

Copy link
Member Author

@jmagman jmagman Oct 7, 2020

Choose a reason for hiding this comment

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

Yes, it's still used on attach, but no longer used for drive or run.

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

Line 263 in b8397f6

await MDnsObservatoryDiscovery.instance.getObservatoryUri(

@jmagman jmagman deleted the disable-observatory branch October 7, 2020 17:50
This was referenced Oct 15, 2020
Closed
Merged
@xster xster mentioned this pull request Nov 25, 2020
Closed
@jmagman jmagman mentioned this pull request Feb 1, 2021
Merged
@jmagman jmagman mentioned this pull request Jan 6, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanderso zanderso zanderso approved these changes

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jmagman @zanderso @jonahwilliams @googlebot @fluttergithubbot