Skip to content

Allow specifying device-vmservice-port and host-vmservice-port#44027

Merged
jonahwilliams merged 9 commits intoflutter:masterfrom
jonahwilliams:use_port_checking
Nov 13, 2019
Merged

Allow specifying device-vmservice-port and host-vmservice-port#44027
jonahwilliams merged 9 commits intoflutter:masterfrom
jonahwilliams:use_port_checking

Conversation

@jonahwilliams
Copy link
Contributor

Description

--host-vmservice-port supersedes --observatory-port which is now soft-deprecated. --device-vmservice-port applies a filter to the observatory discovery process.

WIP due to phrasing and tests

@fluttergithubbot fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Nov 1, 2019
@jonahwilliams
Copy link
Contributor Author

Still not ready I guess, my bad!

/// If no port is set, returns null.
int get hostVmservicePort {
if (!_usesPortOption ||
argResults['observatory-port'] == null ||
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is the right place, but a check that only one of --observatory-port or --host-vmservice-port is set would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'discovered.',
);
argParser.addOption('host-vmservice-port',
help: 'Listen to the given port for a vmservice connection.\n'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe: 'When a device-side vmservice port is forwarded to a host-side port, use this value as the host port.'

this,
ipv6,
debuggingOptions.observatoryPort,
debuggingOptions.hostVmservicePort,
Copy link
Member

Choose a reason for hiding this comment

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

The device port could also be used to filter results from an mdns query. It looks like currently MDnsObservatoryDiscovery will throw a tool exit if there is more than one result, asking for an appId so that it can refine the mdns query, but another way to distinguish would be to pass the device port to the engine and then filter on it. Maybe add a TODO.

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

getLogReader(),
portForwarder: portForwarder,
hostPort: debuggingOptions.observatoryPort,
hostPort: debuggingOptions.hostVmservicePort,
Copy link
Member

Choose a reason for hiding this comment

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

How do we format Vmservice in other places? Should we do VMService, VmService, or keep Vmservice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to consistently use VmService


Future<Uri> getObservatoryUri(String applicationId, Device device, [bool usesIpv6 = false, int observatoryPort]) async {
final MDnsObservatoryDiscoveryResult result = await query(applicationId: applicationId);
Future<Uri> getObservatoryUri(String applicationId, Device device, {bool usesIpv6 = false, int hostVmservicePort, int deviceVmservicePort}) async {
Copy link
Member

Choose a reason for hiding this comment

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

nit: long line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Future<Uri> getObservatoryUri(String applicationId, Device device, [bool usesIpv6 = false, int observatoryPort]) async {
final MDnsObservatoryDiscoveryResult result = await query(applicationId: applicationId);
Future<Uri> getObservatoryUri(String applicationId, Device device, {bool usesIpv6 = false, int hostVmservicePort, int deviceVmservicePort}) async {
final MDnsObservatoryDiscoveryResult result = await query(applicationId: applicationId, deviceVmservicePort: deviceVmservicePort);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
final int localPort = observatoryPort
?? await device.portForwarder.forward(devicePort);
final int localPort = await device.portForwarder.forward(devicePort, hostPort: hostVmservicePort);
Copy link
Member

Choose a reason for hiding this comment

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

In some other places, the name actualHostPort is used, which I think might be a little more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams jonahwilliams merged commit c0af77b into flutter:master Nov 13, 2019
@jonahwilliams jonahwilliams deleted the use_port_checking branch November 13, 2019 21:01
jonahwilliams pushed a commit that referenced this pull request Nov 13, 2019
jonahwilliams pushed a commit that referenced this pull request Nov 13, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants