Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flutter_tools] Add support for vmServiceFileInfo when attaching #128503

Merged

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Jun 8, 2023

When building the new SDK DAPs, this functionality was missed from the Flutter adapter (but added to the Dart CLI adapter).

As well as passing a VM Service URI directly, we support passing a file that can be polled for it.

This uses the same mechanism we use to obtain the VM Service URI from a Dart debug session (we run dart --write-service-info=foo.json my_file.dart and then poll that file which the VM will write) and is useful for users that have their own mechanism for launching an app (for example using custom Flutter embedders - see Dart-Code/Dart-Code#3353) to provide a VM Service URI once the app is up and running.

Fixes Dart-Code/Dart-Code#4577.

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.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 8, 2023
@DanTup DanTup force-pushed the add-vm-service-info-file-for-attach branch from 487e8d5 to 381c6f2 Compare June 8, 2023 12:04
When building the new SDK DAPs, this functionality was missed from the Flutter adapter (but added to the Dart CLI adapter).

As well as passing a VM Service URI directly, we support passing a file that can be polled for it.

This uses the same mechanism we use to obtain the VM Service URI from a Dart debug session (we run "dart --write-service-info=foo.json my_file.dart" and then poll that file) and is useful for users that have their own mechanism for launching an app (for example using custom Flutter embedders - see Dart-Code/Dart-Code#3353) to provide a VM Service URI once the app is up and running.

Fixes Dart-Code/Dart-Code#4577.
@DanTup DanTup force-pushed the add-vm-service-info-file-for-attach branch from 381c6f2 to e4ef0bc Compare June 8, 2023 13:21
@DanTup DanTup force-pushed the add-vm-service-info-file-for-attach branch from 3c02364 to 62534fb Compare June 8, 2023 13:54
@@ -40,7 +40,10 @@ Arguments specific to `launchRequest` are:

Arguments specific to `attachRequest` are:

- `String? vmServiceUri` - the VM Service URI to attach to (if not supplied, Flutter will try to discover it from the device)
- `String vmServiceInfoFile` - the file to read the VM Service info from \*
- `String vmServiceUri` - the VM Service URI to attach to \*
Copy link
Member

Choose a reason for hiding this comment

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

is this now non-nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, no - apparently I pasted the Dart version over this and didn't check it properly (it's wrong there, and I also lost the additional explanatory text). Fixed!

String? vmServiceUri = args.vmServiceUri;
final String? vmServiceInfoFile = args.vmServiceInfoFile;

if (vmServiceUri != null && vmServiceInfoFile != null) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if they're both null? Do we try to discover it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I've made the error text a bit clearer (and also reintroduced this explanation in the readme noted above).

@DanTup
Copy link
Contributor Author

DanTup commented Jun 16, 2023

Out of interest - does the autosubmit bot handle review status? Like if I add the tag now, would it just blindly merge when the build is green, or wait for you to approve? 🤔

@christopherfujino
Copy link
Member

Out of interest - does the autosubmit bot handle review status? Like if I add the tag now, would it just blindly merge when the build is green, or wait for you to approve? thinking

Unfortunately, I think it would remove the label since it's not approved.

Copy link
Member

@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

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2023
@christopherfujino
Copy link
Member

LGTM and added the label for you :)

@auto-submit auto-submit bot merged commit dc4541f into flutter:master Jun 16, 2023
123 checks passed
@DanTup
Copy link
Contributor Author

DanTup commented Jun 16, 2023

Unfortunately, I think it would remove the label since it's not approved.

Ah, yes. I wonder if it's worth a feature request to just delay until it's reviewed? I think that's what Gerrit does - you can add AutoSubmit to mean "when this is approved, land it without me needing to come back and do it"

added the label for you

Thanks! :)

@christopherfujino
Copy link
Member

Unfortunately, I think it would remove the label since it's not approved.

Ah, yes. I wonder if it's worth a feature request to just delay until it's reviewed? I think that's what Gerrit does - you can add AutoSubmit to mean "when this is approved, land it without me needing to come back and do it"

Good idea ;) #113638

@DanTup
Copy link
Contributor Author

DanTup commented Jun 17, 2023

Good idea ;) #113638

christopherfujino commented on Oct 18, 2022

LOL! Always one step ahead 😁

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 21, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vmServiceInfoFile does not work with Flutter SDK debug adapter
2 participants