flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Launch DDS from Dart SDK and prepare to serve DevTools from DDS

Open bkonyi opened this issue 1 year ago • 12 comments

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.serve endpoint
  • flutter drive's --profile-memory flag 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.

bkonyi avatar Apr 10 '24 18:04 bkonyi

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.

bkonyi avatar Apr 12 '24 20:04 bkonyi

I scheduled time next week Monday to take a look at this tome :)

christopherfujino avatar Apr 12 '24 21:04 christopherfujino

@bkonyi As I understand it, this PR is doing two different things:

  1. Changing the way the tool launches DDS from calling dds.DartDevelopmentService.startDartDevelopmentService from package:dds to spawning a sub-process via dart development-service
  2. 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?

christopherfujino avatar Apr 15 '24 20:04 christopherfujino

@bkonyi As I understand it, this PR is doing two different things:

  1. Changing the way the tool launches DDS from calling dds.DartDevelopmentService.startDartDevelopmentService from package:dds to spawning a sub-process via dart development-service
  2. 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 avatar Apr 17 '24 20:04 bkonyi

@bkonyi As I understand it, this PR is doing two different things:

  1. Changing the way the tool launches DDS from calling dds.DartDevelopmentService.startDartDevelopmentService from package:dds to spawning a sub-process via dart development-service
  2. 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.

Thanks. I'm pretty swamped today, but scheduled time tomorrow to review.

christopherfujino avatar Apr 18 '24 19:04 christopherfujino

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.

bkonyi avatar Apr 18 '24 19:04 bkonyi

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.

flutter-dashboard[bot] avatar May 08 '24 19:05 flutter-dashboard[bot]

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?

andrewkolos avatar May 30 '24 20:05 andrewkolos

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 :(

bkonyi avatar Jun 03 '24 15:06 bkonyi

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.

bkonyi avatar Jun 10 '24 20:06 bkonyi

LGTM from my side, but should wait for @christopherfujino's approval before landing.

kenzieschmoll avatar Jun 11 '24 18:06 kenzieschmoll

@christopherfujino friendly bump.

bkonyi avatar Jun 25 '24 18:06 bkonyi

The remaining two checks have completed successfully on LUCI even though GitHub doesn't reflect this, so I'm just going to land this.

bkonyi avatar Jul 15 '24 18:07 bkonyi

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

zanderso avatar Jul 15 '24 19:07 zanderso