flutter
flutter copied to clipboard
Launch DDS from Dart SDK and prepare to serve DevTools from DDS
This change is a major step towards moving away from shipping DDS via Pub.
The first component of this PR is the move away from importing package:dds to launch DDS. Instead, DDS is launched out of process using the dart development-service command shipped with the Dart SDK. This makes Flutter's handling of DDS consistent with the standalone Dart VM.
The second component of this PR is the initial work to prepare for the removal of instances of DevTools being served manually by the flutter_tool, instead relying on DDS to serve DevTools. This will be consistent with how the standalone Dart VM serves DevTools, tying the DevTools lifecycle to a live DDS instance. This will allow for the removal of much of the logic needed to properly manage the lifecycle of the DevTools server in a future PR. Also, by serving DevTools from DDS, users will no longer need to forward a secondary port in remote workflows as DevTools will be available on the DDS port.
There's two remaining circumstances that will prevent us from removing DevtoolsRunner completely:
- The daemon's
devtools.serveendpoint flutter drive's--profile-memoryflag used for recording memory profiles
This PR also includes some refactoring around DebuggingOptions to reduce the number of debugging related arguments being passed as parameters adjacent to a DebuggingOptions instance.
I think this is ready for a first pass. The failing integration test will be fixed once the Dart SDK roll picks up this CL. I still need to investigate what changes are needed for google3.
I scheduled time next week Monday to take a look at this tome :)
@bkonyi As I understand it, this PR is doing two different things:
- Changing the way the tool launches DDS from calling
dds.DartDevelopmentService.startDartDevelopmentServicefrom package:dds to spawning a sub-process viadart development-service - Changing the way the tool launches DevTools from directly to via DDS
Do I have this right? And if so, would it be possible to split this into two separate PRs, both to make it easier to review and to minimize the impact if either of these changes needs to be reverted?
@bkonyi As I understand it, this PR is doing two different things:
- Changing the way the tool launches DDS from calling
dds.DartDevelopmentService.startDartDevelopmentServicefrom package:dds to spawning a sub-process viadart development-service- Changing the way the tool launches DevTools from directly to via DDS
Do I have this right? And if so, would it be possible to split this into two separate PRs, both to make it easier to review and to minimize the impact if either of these changes needs to be reverted?
I've gone ahead and reverted to using the ResidentDevToolsHandler code to manage the DevTools instance to reduce the surface area of this change. Much of the code needed to move over to serving DevTools from DDS is still present but commented out with TODOs and can be reviewed in the follow up PR.
There may be some tests still failing, but I think this is ready for a first pass.
@bkonyi As I understand it, this PR is doing two different things:
- Changing the way the tool launches DDS from calling
dds.DartDevelopmentService.startDartDevelopmentServicefrom package:dds to spawning a sub-process viadart development-service- Changing the way the tool launches DevTools from directly to via DDS
Do I have this right? And if so, would it be possible to split this into two separate PRs, both to make it easier to review and to minimize the impact if either of these changes needs to be reverted?
I've gone ahead and reverted to using the
ResidentDevToolsHandlercode to manage the DevTools instance to reduce the surface area of this change. Much of the code needed to move over to serving DevTools from DDS is still present but commented out with TODOs and can be reviewed in the follow up PR.There may be some tests still failing, but I think this is ready for a first pass.
Thanks. I'm pretty swamped today, but scheduled time tomorrow to review.
Thanks. I'm pretty swamped today, but scheduled time tomorrow to review.
No rush, the google3 changes required for this to land are blocked on Dart CLI support in google3, so I'm not sure how long that'll take to clear up.
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.
For more guidance, visit Writing a golden file test for package:flutter.
Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.
No rush, the google3 changes required for this to land are blocked on Dart CLI support in google3, so I'm not sure how long that'll take to clear up.
@bkonyi is this still the case?
No rush, the google3 changes required for this to land are blocked on Dart CLI support in google3, so I'm not sure how long that'll take to clear up.
@bkonyi is this still the case?
Yeah, I've been fighting with some build rules to get all the right artifacts shipped with the Dart SDK packaged with the Flutter extracted and depended on properly. It's been a headache so far :(
FYI @christopherfujino @kenzieschmoll, this should be ready for review. This PR is currently blocked from landing as this CL in the Dart SDK needs to be rolled into google3, but otherwise this change is good to go.
LGTM from my side, but should wait for @christopherfujino's approval before landing.
@christopherfujino friendly bump.
The remaining two checks have completed successfully on LUCI even though GitHub doesn't reflect this, so I'm just going to land this.
Reason for revert: Consistently failing Windows_android native_assets_android as in https://ci.chromium.org/ui/p/flutter/builders/prod/Windows_android%20native_assets_android/2533/overview