-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Reland automatic discovery of observatory port for iOS #27908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Generally LGTM - how do you feel about the refactor in #27455 ? think the attach command is growing a bit long, and the |
|
|
||
| import 'dart:async'; | ||
|
|
||
| import 'package:multicast_dns/multicast_dns.dart'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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. |
|
I don't understand how, but this apparently raised our consumer facing dependency count by 4 to 35. |
|
I think we have a bug in our dependency pinning script, it is automatically including pinned packages in the transitive closure |
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