Launch DDS from Dart SDK and prepare to serve DevTools from DDS #146593
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 removal of instances of DevTools being served manually by the flutter_tool, instead relying on DDS to serve DevTools. This is consistent with how the standalone Dart VM serves DevTools, tying the DevTools lifecycle to a live DDS instance. This allows for the removal of much of the logic needed to properly manage the lifecycle of the DevTools server. 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 preventing 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.
github-actions
bot
added
platform-ios
bkonyi
removed
platform-ios
github-actions
bot
added
platform-ios
Contributor
Author
|
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. |
Contributor
|
I scheduled time next week Monday to take a look at this tome :) |
| enableImpeller: ImpellerStatus.fromBool(argResults!['enable-impeller'] as bool?), | ||
| debugLogsDirectoryPath: debugLogsDirectoryPath, | ||
| webRenderer: webRenderer, | ||
| printDtd: boolArg(FlutterGlobalOptions.kPrintDtd, global: true), |
Member
There was a problem hiding this comment.
we seem to repeat a lot of this arg parsing logic throughout flutter tools. Could be nice if there were some common getters to use
bool get printDtdFlag => boolArg(FlutterGlobalOptions.kPrintDtd, global: true);
bool get enableDevToolsFlag => boolArg(FlutterCommand.kEnableDevTools);
bool get disableServiceAuthCodesFlag => boolArg('disable-service-auth-codes');
but likely out of scope for this PR as this would be a larger cleanup
packages/flutter_tools/test/general.shard/runner/flutter_command_runner_test.dart
Show resolved
Hide resolved
Contributor
|
@bkonyi As I understand it, this PR is doing two different things:
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
changed the title
Contributor
Author
I've gone ahead and reverted to using the There may be some tests still failing, but I think this is ready for a first pass. |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 19, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 19, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 19, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 19, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 19, 2024
auto-submit bot
pushed a commit
that referenced
this pull request
Jul 26, 2024
…DS (#146593)" (#152386) This reverts commit 7cdc23b. The failure in the `native_assets_test` integration test on Windows was caused by the DevTools process not being shutdown by the `ColdRunner` when running the profile mode portion of the test. This resulted in the test being unable to clean up the project created by the test as DevTools was still holding onto a handle within the directory. This PR adds back the mistakenly removed DevTools shutdown logic in the `ColdRunner`.
TytaniumDev
pushed a commit
to TytaniumDev/flutter
that referenced
this pull request
Aug 7, 2024
…ter#146593) 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. This code is currently commented out and will be enabled in a future PR. 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.
TytaniumDev
pushed a commit
to TytaniumDev/flutter
that referenced
this pull request
Aug 7, 2024
…DDS (flutter#146593)" (flutter#151781) Reverts: flutter#146593 Initiated by: zanderso Reason for reverting: 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 Original PR Author: bkonyi Reviewed By: {christopherfujino, kenzieschmoll} This change reverts the following previous change: 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.
TytaniumDev
pushed a commit
to TytaniumDev/flutter
that referenced
this pull request
Aug 7, 2024
…DS (flutter#146593)" (flutter#152386) This reverts commit 7cdc23b. The failure in the `native_assets_test` integration test on Windows was caused by the DevTools process not being shutdown by the `ColdRunner` when running the profile mode portion of the test. This resulted in the test being unable to clean up the project created by the test as DevTools was still holding onto a handle within the directory. This PR adds back the mistakenly removed DevTools shutdown logic in the `ColdRunner`.
Buchimi
pushed a commit
to Buchimi/flutter
that referenced
this pull request
Sep 2, 2024
…DS (flutter#146593)" (flutter#152386) This reverts commit 7cdc23b. The failure in the `native_assets_test` integration test on Windows was caused by the DevTools process not being shutdown by the `ColdRunner` when running the profile mode portion of the test. This resulted in the test being unable to clean up the project created by the test as DevTools was still holding onto a handle within the directory. This PR adds back the mistakenly removed DevTools shutdown logic in the `ColdRunner`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-servicecommand 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:
devtools.serveendpointflutter drive's--profile-memoryflag used for recording memory profilesThis PR also includes some refactoring around
DebuggingOptionsto reduce the number of debugging related arguments being passed as parameters adjacent to aDebuggingOptionsinstance.