Sitelet https://github.com/flutter/flutter/pull/27908
Skip to content

Reland automatic discovery of observatory port for iOS - #27908

Merged
dnfield merged 23 commits into
flutter:masterfrom
dnfield:ios_attach
Feb 16, 2019
Merged

dnfield merged 23 commits into
flutter:masterfrom
dnfield:ios_attach

Conversation

@dnfield

@dnfield dnfield commented Feb 13, 2019 •

Copy link
Copy Markdown
Contributor

Relands #26944 (was reverted in #27191)

Multicast_dns has landed internally, so this should no longer be an internal roll blocker.

/cc @zanderso who was cc'd on the revert for reasons I'm not fully aware of :)

fixes #23164

@jonahwilliams

Copy link
Copy Markdown
Contributor

Generally LGTM - how do you feel about the refactor in #27455 ? think the attach command is growing a bit long, and the is checks are a definite smell


import 'dart:async';

import 'package:multicast_dns/multicast_dns.dart';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this will need to be added to the BUILD.gn as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added - can you verify I did it right though?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @zanderso do we need to manually roll this into fuchsia?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, after the in-flight roll lands, this will cause the auto-roller to block until Fuchsia's third_party Dart packages are manually updated. I can help with that.

Also, it should go in this one https://github.com/flutter/flutter/blob/master/packages/flutter_tools/BUILD.gn, not the one for flutter_test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I've moved it to the correct BUILD.gn

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this good to land now, or were we waiting on confirmation for any of this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is okay to land from the Fuchsia perspective. An earlier flutter/flutter change also requires an update to the Fuchsia third_party Dart packages, so it would be convenient to handle this one and that one at the same time.

@zoechi zoechi added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 14, 2019
@dnfield dnfield added platform-ios iOS applications specifically platform-macos Building on or for macOS specifically labels Feb 14, 2019
@dnfield

dnfield commented Feb 15, 2019

Copy link
Copy Markdown
Contributor Author

Note: This commit is pinning json_schema to 1.0.10, since >=2.0.0 adds additional transitive dependencies we don't want in the tools. Opened #28021 to track.

@dnfield
dnfield merged commit c90c3a1 into flutter:master Feb 16, 2019
@dnfield
dnfield deleted the ios_attach branch February 16, 2019 01:45
@Hixie

Hixie commented Feb 19, 2019

Copy link
Copy Markdown
Contributor

I don't understand how, but this apparently raised our consumer facing dependency count by 4 to 35.
This seems problematic on two fronts. One, raising our dependency count by >10% is pretty serious. Two, why does the patch show no evidence of this?

@jonahwilliams

Copy link
Copy Markdown
Contributor

I think we have a bug in our dependency pinning script, it is automatically including pinned packages in the transitive closure

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-ios iOS applications specifically platform-macos Building on or for macOS 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.

Teach flutter_tools how to use mDNS to find the iOS observatory port

6 participants