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

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

Merged
merged 1 commit into from Apr 30, 2024

Conversation

andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Apr 28, 2024

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

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

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)

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?

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

@andrewkolos
Copy link
Contributor Author

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

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
130 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 1, 2024
flutter/flutter@b597dd2...d33bb8f

2024-05-01 sokolovskyi.konstantin@gmail.com Add tests for single_activator.0.dart API example. (flutter/flutter#147426)
2024-05-01 Dhankechakishan@gmail.com Added missing code block language in docs (flutter/flutter#147481)
2024-05-01 zanderso@users.noreply.github.com 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 47866232+chunhtai@users.noreply.github.com Revert "add a new PopScope.onPopWithResultInvoke widget to replace Po� (flutter/flutter#147597)
2024-04-30 andrewrkolos@gmail.com include exception details in tool exit displayed when adb call fails (flutter/flutter#147498)
2024-04-30 32538273+ValentinVignal@users.noreply.github.com 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 dit@google.com,rmistry@google.com,stuartmorgan@google.com 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
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.

Include more information about adb failure in flutter daemon exit messages
3 participants