Skip to content

Conversation

@DanTup
Copy link
Contributor

@DanTup DanTup commented Jun 21, 2022

This allows a DAP client to know when an app has finished launching even if there is no VM Service (such as noDebug runs) and also still get the VM Service URI in the case where the adapter doesn't connect to it itself (Profile mode - where we use VM Service for DevTools in the client, but don't provide any debugging).

Fixes Dart-Code/Dart-Code#3945 (with some client updates).

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 this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

This allows a DAP client to know when an app has finished launching even if there is no VM Service (such as noDebug runs).

Fixes Dart-Code/Dart-Code#3945 (with some client updates).
@DanTup DanTup requested a review from christopherfujino June 21, 2022 12:00
@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 21, 2022
@DanTup DanTup changed the title Pass app.started events to the DAP client Pass app.started events to the DAP client + dart.debuggerUris for Profile mode Jun 21, 2022
'vmServiceUri': serviceUri.toString(),
}),
eventType: 'dart.debuggerUris',
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a CL that adds a method to the base to avoid duplicating it here, but it needs a new DDS release (and then rolling into here). Assuming that doesn't happen before this is ready, I'll sort this in a separate PR once it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

are you saying the DDS change hasn't been published yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing it's (going to be) this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in DDS 2.2.3 published today, however that can't be rolled into Flutter yet due to a conflicting vm_service constraint with DWDS.

In case it isn't clear, the DDS change is not required for this code to function, only to replace lines 412-417 with a simpler base method call so that the Flutter adapter doesn't need to construct the dart.debuggerUris message itself.

Copy link
Contributor Author

@DanTup DanTup Jun 21, 2022

Choose a reason for hiding this comment

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

I'm guessing it's (going to be) this

Yup! The real change was this, which is just extracting a method so this Flutter adapter can call it:

dart-lang/sdk@fe8dd70

Copy link
Contributor

Choose a reason for hiding this comment

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

It's in DDS 2.2.3 published today, however that can't be rolled into Flutter yet due to a conflicting vm_service constraint with DWDS.

In case it isn't clear, the DDS change is not required for this code to function, only to replace lines 412-417 with a simpler base method call so that the Flutter adapter doesn't need to construct the dart.debuggerUris message itself.

Understood, I was hoping I could roll forward to simplify this change, but I understand we're blocked on transitive deps.

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit e2ef8cf into flutter:master Jun 21, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 22, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 22, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 22, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 22, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

Improve error message when trying to open DevTools for a debug session that has no VM Service

3 participants