Skip to content

[flutter_tools] Add support for custom devices#78113

Merged
fluttergithubbot merged 23 commits intoflutter:masterfrom
ardera:master
Apr 1, 2021
Merged

[flutter_tools] Add support for custom devices#78113
fluttergithubbot merged 23 commits intoflutter:masterfrom
ardera:master

Conversation

@ardera
Copy link
Contributor

@ardera ardera commented Mar 13, 2021 •
edited
Loading

  • added a feature flag that allows enabling / disabling the custom devices feature

  • add a config file in the default flutter config location (for windows, %APPDATA%/.flutter_custom_devices.json).

  • the config file is created with the $schema tag and some (disabled) example device configured each time any flutter command is run when the custom devices feature is enabled.

  • the schema tag points to https://raw.githubusercontent.com/flutter/flutter/stable/packages/flutter_tools/static/custom-devices.schema.json so we get autocompletion. (EDIT: Changed it to stable. Probably better that way, it'll probably confuse people if they get autocorrection for features that aren't yet implemented in stable. If people do want to get autocompletion for master channel features they can just change the URL)

  • added a CustomDevicesConfig class for querying the currently configured custom devices (with CustomDeviceConfig representing a specific device config)

  • added string interpolation to utils.dart (used for replacing things like ${hostPort} in the custom device commands)

  • added some testing

  • did some manual testing and it seems at least the VS Code plugin needs the linux-desktop flag (plus the custom devices flag) enabled to even list any custom devices. not sure whats up with that

  • also made protocol discovery not assert a loopback address since some people may not use port forwarding

  • by default the target platform is linux_arm64 (doesn't matter anyway since we're only building platform-agnostic stuff rn), didn't want to add a new target platform since that's probably a bigger breaking change

  • there's a TODO item in the code to make the target platform configurable in the future

  • resolves Custom device types as a solution for custom embedder support for flutter run #65067

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

It's probably better to add some more testing, for example for CustomDevicePortForwarder. Is there anything else I didn't notice I should also test?

Some tests don't pass on my local machine but they seem unrelated to anything I've written. For example the analyze_once test fails, because the error message isn't the expected one, or commands.shard\hermetic\run_test.dart bc it tries to access the flutter artifacts dir at C:\C:\Users\hanne\devel\flutter_sdk\bin\cache\artifacts which clearly isn't the right syntax. Maybe it's because I'm on Windows?

EDIT: Added all the tests. Edited schema URL.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 13, 2021
@google-cla google-cla bot added the cla: yes label Mar 13, 2021
@ardera ardera changed the title Add support for custom devices [WIP] Add support for custom devices Mar 13, 2021
@ardera ardera mentioned this pull request Mar 14, 2021
Closed
@ardera ardera changed the title [WIP] Add support for custom devices Add support for custom devices Mar 16, 2021
@jonahwilliams jonahwilliams self-requested a review March 18, 2021 20:35
@ardera
Copy link
Contributor Author

ardera commented Mar 19, 2021

In hindsight, I think it may be better to just define a CustomApp and some custom_arm, custom_arm64, custom_x86, custom_x64 target platforms instead of reusing the linux ones, even if that may require additional support from any IDE plugins

jonahwilliams
jonahwilliams reviewed Mar 19, 2021
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.

Just took a first pass - overall looks good. We might need to think a bit about stabilization too. Not sure what your thoughts are there

@ardera
Copy link
Contributor Author

ardera commented Mar 19, 2021

okay, should all be resolved now.

Just took a first pass - overall looks good. We might need to think a bit about stabilization too. Not sure what your thoughts are there

With stabilization, do you mean like API (or generally, interface) stability or making sure everything runs nice and stable?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 19, 2021

Sort of both. I think we can say initially that we can change this at any version to give time to shake things out, and in the meantime keep it enabled only for master/dev. Any concerns with that?

jonahwilliams
jonahwilliams reviewed Mar 19, 2021
View reviewed changes
@ardera
Copy link
Contributor Author

ardera commented Mar 19, 2021 •
edited
Loading

Sort of both. I think we can say initially that we can change this at any version to give time to shake things out, and in the meantime keep it enabled only for master/dev. Any concerns with that?

Sure. Yeah it'd be nice to have release support too. Maybe when implementing that we'll have to make some other breaking changes. (Though it'd also require official engine binaries for linux ARMv7, so it may take some time till that can be implemented)

I'm still probably going to create a fork and backport it to stable just so I can use it on stable myself

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 19, 2021

It would help if we had a list of features that are incomplete, then maybe that could be the guidelines for a stable feature?

@ardera
Copy link
Contributor Author

ardera commented Mar 19, 2021

It would help if we had a list of features that are incomplete, then maybe that could be the guidelines for a stable feature?

Yep that's a good idea. So should we create a new GH project for that? Or a google doc?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 19, 2021

Either works for me, GH project might be a bit easier to manage though

@ardera ardera mentioned this pull request Mar 19, 2021
Closed
@ardera
Copy link
Contributor Author

ardera commented Mar 19, 2021

Either works for me, GH project might be a bit easier to manage though

GH project sounds good to me. Can create some issues that you can add to the project (can't create the project myself of course). First one is here already.

In hindsight, I think it may be better to just define a CustomApp and some custom_arm, custom_arm64, custom_x86, custom_x64 target platforms instead of reusing the linux ones, even if that may require additional support from any IDE plugins

Created an issue for this: #78644 (Seems like you've already seen it). Though this doesn't need to be implemented in this PR since it's not blocking anything and can be easily changed later

@ardera
Copy link
Contributor Author

ardera commented Mar 20, 2021 •
edited
Loading

This also resolves #78644 now

@jonahwilliams jonahwilliams self-requested a review March 22, 2021 20:55
jonahwilliams
jonahwilliams previously approved these changes Mar 22, 2021
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.

Took another pass, this looks really good so far

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 22, 2021

My bad I didn't mean to approve yet, but its close!

@ardera
Copy link
Contributor Author

ardera commented Mar 22, 2021

My bad I didn't mean to approve yet, but its close!

nice! 😄

ardera and others added 21 commits March 31, 2021 23:26
@ardera
prefer relative imports
b8ded65
@ardera
- add tests, make non-null-safe
9781be8 
- add tests for custom device workflow
- add tests for custom device port forwarding
- add tests for custom device app starting / stopping
- add tests for custom device app installing / uninstalling
- split config into custom_devices_config.dart and custom_device_config.dart, so at least the CustomDeviceConfig can stay null-safe
- make CustomDevicesConfig non-null-safe since it relies on other non-null-safe stuff
- change JSON schema URI to refer to the stable branch
- fix other warnings
- use processManager.killPid instead of process.kill, since thats not compatible with testing
@ardera
- eliminate trailing U+0020 space character in custom_device_test.dart
760c70d
@ardera
- add missing license header
deffe02
@ardera
fix custom device workflow test
704085c
@ardera
Update packages/flutter_tools/lib/src/custom_devices/custom_device.dart
750038f 
Co-authored-by: Jonah Williams <[email protected]>
@ardera
fix some issues
c416235 
- formatting
- custom devices feature flag details
- set custom_devices.schema.json schema id to the stable one
- add "regex" format to the forwardPortSuccessRegex property in the schema
@ardera
fix some documentation in the JSON schema
814d9c6
@ardera
use custom platform type
959c37b 
- add `PlatformType.custom`
- make CustomDevices use `PlatformType.custom`
- make daemon report custom devices are always supported if custom devices feature is enabled
@ardera
Update packages/flutter_tools/lib/src/base/utils.dart
b3ba3b4 
Co-authored-by: Jonah Williams <[email protected]>
@ardera
some style improvements
d2b0e6f 
- use @VisibleForTesting
- use whenComplete
- dont use `else` when the if returns unconditionally
- improve spacing
- add a comment explaining some order of things
@ardera
document CustomDevices constructors
e7d46f8
@ardera
some fixes, improvements
9d4eb4c 
- don't store custom devices config in context
- add optional ping success regex in config (by default will look for 'ms TTL=' in ping command output)
- add end-to-end test for custom devices
- make sure ports are unforwarded when app is terminated
- implicitly ensure the custom devices config file exists in the constructor rather than when invoking a separate method
- fix some formatting
@ardera
fix import for device port forwarding
577b5df
@ardera
fix double indent
1d136aa
@ardera
use own MockBundleBuilder in test instead of importing the one from a…
edf996b 
…nother test
@ardera
Update packages/flutter_tools/test/general.shard/custom_devices/custo…
0602d48 
…m_device_test.dart

Co-authored-by: Jonah Williams <[email protected]>
@ardera
Update packages/flutter_tools/test/general.shard/custom_devices/custo…
5b941da 
…m_device_workflow_test.dart

Co-authored-by: Jonah Williams <[email protected]>
@ardera
add some more documentation
6b8adef 
- create FakeBundleBuilder and use that in custom_device_test.dart instead of a MockBundleBuilder
- make default JSON schema for custom devices config a file URI, pointing to the schema inside the flutter install
- use += 1 instead of ++
- await unforwarding in CustomDevicePortForwarder.dispose
@ardera
prefer relative imports
7d6279d
@ardera
add CustomDevices discovery to flutter device manager again
fd5d484
@ardera
Copy link
Contributor Author

ardera commented Mar 31, 2021

LGTM

You'll need to resolve the merge conflicts - I moved most of the concrete DeviceDiscovery types out of devices.dart and into a separate library - you should do the same with the custom device code

Nice! fixed the conflicts

@jonahwilliams jonahwilliams changed the title Add support for custom devices [flutter_tools] Add support for custom devices Mar 31, 2021
@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 31, 2021

Awesome, will land once tests pass and the build is green again

@fluttergithubbot fluttergithubbot merged commit e8ad030 into flutter:master Apr 1, 2021
@jmagman jmagman mentioned this pull request Apr 1, 2021
Merged
@HidenoriMatsubayashi HidenoriMatsubayashi mentioned this pull request May 17, 2021
Closed
@mbriand mbriand mentioned this pull request Feb 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman left review comments

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Custom device types as a solution for custom embedder support for flutter run

4 participants

@ardera @jonahwilliams @jmagman @fluttergithubbot