flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
162.18k stars 26.63k forks source link

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

Closed andrewkolos closed 2 weeks ago

andrewkolos commented 2 weeks ago

Fixes #125971

Pre-launch Checklist

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

andrewkolos commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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) https://github.com/flutter/flutter/blob/be0eb721d5a0668a32878a16da7a699a8998b7c4/packages/flutter_tools/lib/runner.dart#L176-L178

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?

andrewkolos commented 2 weeks ago

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 commented 2 weeks ago

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