Skip to content

Add option to stream logs to file for flutter logs and way to use it in devicelab runs#84008

Merged
dnfield merged 5 commits intoflutter:masterfrom
dnfield:dl_logs
Jun 7, 2021
Merged

Add option to stream logs to file for flutter logs and way to use it in devicelab runs#84008
dnfield merged 5 commits intoflutter:masterfrom
dnfield:dl_logs

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jun 4, 2021 •
edited
Loading

Fixes #84000

See also #83939, #77193

We'll need a recipe change to get this running on the recipes.

@dnfield dnfield requested review from jmagman and jonahwilliams June 4, 2021 18:13
@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 4, 2021
@google-cla google-cla bot added the cla: yes label Jun 4, 2021
@dnfield dnfield requested a review from godofredoc June 4, 2021 18:13
jonahwilliams
jonahwilliams approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

tool changes LGTM

@jmagman
Copy link
Member

jmagman commented Jun 4, 2021

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8845332628763500560/+/u/run_test.dart_for_framework_tests_shard_and_subshard_misc/stdout

  Unhandled exception:
  Invalid argument(s): Could not find an option named "device-logs-path".
  #0      ArgResults.[] (package:args/src/arg_results.dart:65:7)
  #1      main (file:///b/s/w/ir/x/w/flutter/dev/devicelab/bin/run.dart:104:35)
  #2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:281:32)
  #3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

jmagman
jmagman reviewed Jun 4, 2021
View reviewed changes
dev/devicelab/bin/run.dart Outdated
Copy link
Member

@jmagman jmagman Jun 4, 2021

Choose a reason for hiding this comment

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

The arg is device-log-path. Probably worth adding device-logs-path as an alias to the arg if you, the person who wrote it a few minutes ago, couldn't quite remember which it was 😄

Copy link
Contributor Author

@dnfield dnfield Jun 4, 2021

Choose a reason for hiding this comment

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

Yes...

Copy link
Contributor Author

@dnfield dnfield Jun 4, 2021

Choose a reason for hiding this comment

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

WDYT of doing this as an optional arg vs. detecting LocalPlatform().environment[FLUTTER_LOGS_DIR]?

I mentionedin the issue, I'd rather avoid the HostAgent logic since I don't want to do this by default on local dev machines.

Copy link
Contributor Author

@dnfield dnfield Jun 4, 2021

Choose a reason for hiding this comment

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

As discussed in chat, uploaded a new change to make hostAGent not fall back to a local temp dir (and updated the test to make sure that's the case), and now using hostAgent.

jmagman
jmagman approved these changes Jun 4, 2021
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM

dev/devicelab/lib/framework/host_agent.dart Outdated
Copy link
Member

@jmagman jmagman Jun 4, 2021

Choose a reason for hiding this comment

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

Thanks for doing this.

@dnfield
Copy link
Contributor Author

dnfield commented Jun 4, 2021

Argh. This doesn't work great. I'm going to instead directly interact with adb and idevicesyslog.

@dnfield
Copy link
Contributor Author

dnfield commented Jun 5, 2021

Ok, now this works.

I've renamed adb.dart to devices.dart since it contains .. everything about devices and little about adb.

I've added new methods to Device to start/stop logging with minimal filtering (--quiet on iOS, a bit more restrictive on Android but keeping all fatal logs).

I've wrapped task execution with actually collecting those logs.

@dnfield
Copy link
Contributor Author

dnfield commented Jun 5, 2021

Most of the changes here are simple file renames. a3bfe1e is the meat of this change.

@dnfield dnfield requested review from jmagman and jonahwilliams June 5, 2021 03:15
@dnfield
Copy link
Contributor Author

dnfield commented Jun 5, 2021

Fixed up the merge conflicts.

jmagman
jmagman approved these changes Jun 7, 2021
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for renaming adb.dart, that should have been done awhile ago!

@dnfield dnfield merged commit 5cc7b6c into flutter:master Jun 7, 2021
@chinmaygarde
Copy link
Contributor

chinmaygarde commented Jun 7, 2021

Since Dan is OOO this week, can we land this right away please?

@dnfield dnfield deleted the dl_logs branch June 7, 2021 17:33
@dnfield
Copy link
Contributor Author

dnfield commented Jun 7, 2021

Oh I landed it. Not waiting ror another merge conflict. I'm semi-around.

This was referenced Jun 14, 2021
Merged
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman approved these changes

@godofredoc godofredoc Awaiting requested review from godofredoc

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. 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.

Devicelab bots should always upload full attached device log

4 participants

@dnfield @jmagman @chinmaygarde @jonahwilliams