Skip to content

include exception details in tool exit displayed when adb call fails#147498

Merged
auto-submit[bot] merged 1 commit intoflutter:masterfrom
andrewkolos:improve-adb-error
Apr 30, 2024
Merged

include exception details in tool exit displayed when adb call fails#147498
auto-submit[bot] merged 1 commit intoflutter:masterfrom
andrewkolos:improve-adb-error

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Apr 28, 2024 •
edited
Loading

Fixes #125971

Pre-launch Checklist

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

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 28, 2024
@andrewkolos andrewkolos marked this pull request as ready for review April 28, 2024 21:28
@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 29, 2024

In #125971, @DanTup proposes (as an example) including the stack trace, but I feel like this would be pretty noisy to include in a tool exit message. Perhaps we could include the stack trace in a printTrace call, but I am not sure that would appear in the flutter daemon log output surfaced by VS Code. @DanTup, do you know if printTrace output would appear in these logs (i.e. does Dart-Code invoke flutter daemon with the -v flag)?

@DanTup
Copy link
Contributor

DanTup commented Apr 29, 2024

i.e. does Dart-Code invoke flutter daemon with the -v flag?

It doesn't currently. We could change it, but it does mean there would generally be a lot more output (in normal operation when there are no errors) being sent over to the IDEs to be read/ignored.

Is there a way we could include the stack trace info in the call to throwToolExit that could be handled by the daemon mode and always printed? (flutter daemon is only intended for IDEs, so having error messages be verbose even without -v could be helpful).

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 29, 2024

i.e. does Dart-Code invoke flutter daemon with the -v flag?

It doesn't currently. We could change it, but it does mean there would generally be a lot more output (in normal operation when there are no errors) being sent over to the IDEs to be read/ignored.

Makes sense.

Is there a way we could include the stack trace info in the call to throwToolExit that could be handled by the daemon mode and always printed? (flutter daemon is only intended for IDEs, so having error messages be verbose even without -v could be helpful).

I think we could augment this conditional (excerpted from the code that handles ToolExits)

flutter/packages/flutter_tools/lib/runner.dart

Lines 176 to 178 in be0eb72

if (verbose) {
globals.printError('\n$stackTrace\n');
}

We could change if (verbose) to if (verbose || args.first == 'daemon') (in an actual implementation, I would try avoid the hard-coded dependency on the name of the command, but I'm keeping this simple). I'm pretty neutral on this since it's not clear how often this will be useful, but I'm down to implement this while I'm here. Any disagreement here, @christopherfujino?

christopherfujino
christopherfujino approved these changes Apr 29, 2024
View reviewed changes
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

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 30, 2024

I'm pretty neutral on this since it's not clear how often this will be useful, but I'm down to implement this while I'm here. Any disagreement here, @christopherfujino?

@christopherfujino 😛

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 30, 2024 •
edited
Loading

I'm pretty neutral on this since it's not clear how often this will be useful, but I'm down to implement this while I'm here. Any disagreement here, @christopherfujino?

@christopherfujino 😛

I'm also fine to just merge this as-is since this isn't high priority. If you agree, feel free to re-approve

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 30, 2024
@auto-submit auto-submit bot merged commit ce822ec into flutter:master Apr 30, 2024
@andrewkolos andrewkolos mentioned this pull request Apr 30, 2024
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 1, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2024
@engine-flutter-autoroll
ce822ec include exception details in tool exit displayed when adb cal…
07ba747 
…l fails (flutter/flutter#147498)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 1, 2024
@engine-flutter-autoroll
Roll Flutter from b597dd2 to d33bb8f (7 revisions) (#6633)
d500725 
flutter/flutter@b597dd2...d33bb8f

2024-05-01 [email protected] Add tests for single_activator.0.dart API example. (flutter/flutter#147426)
2024-05-01 [email protected] Added missing code block language in docs (flutter/flutter#147481)
2024-05-01 [email protected] Move docs_test and docs_publish to bringup (flutter/flutter#147645)
2024-05-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix memory leak in `TabPageSelector`  (#147403)" (flutter/flutter#147622)
2024-04-30 [email protected] Revert "add a new PopScope.onPopWithResultInvoke widget to replace Poâ�¦ (flutter/flutter#147597)
2024-04-30 [email protected] include exception details in tool exit displayed when adb call fails (flutter/flutter#147498)
2024-04-30 [email protected] Fix memory leak in `TabPageSelector`  (flutter/flutter#147403)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
@engine-flutter-autoroll @TecHaxter
Roll Flutter from b597dd2 to d33bb8f (7 revisions) (flutter#6633)
01dab88 
flutter/flutter@b597dd2...d33bb8f

2024-05-01 [email protected] Add tests for single_activator.0.dart API example. (flutter/flutter#147426)
2024-05-01 [email protected] Added missing code block language in docs (flutter/flutter#147481)
2024-05-01 [email protected] Move docs_test and docs_publish to bringup (flutter/flutter#147645)
2024-05-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix memory leak in `TabPageSelector`  (#147403)" (flutter/flutter#147622)
2024-04-30 [email protected] Revert "add a new PopScope.onPopWithResultInvoke widget to replace Poâ�¦ (flutter/flutter#147597)
2024-04-30 [email protected] include exception details in tool exit displayed when adb call fails (flutter/flutter#147498)
2024-04-30 [email protected] Fix memory leak in `TabPageSelector`  (flutter/flutter#147403)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
ce822ec include exception details in tool exit displayed when adb cal…
0b0c600 
…l fails (flutter/flutter#147498)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include more information about adb failure in flutter daemon exit messages

3 participants

@andrewkolos @DanTup @christopherfujino