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 into
flutter: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 into
flutter:masterfrom
dnfield:dl_logs

Conversation

@dnfield

@dnfield dnfield commented Jun 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed Jun 4, 2021
View reviewed changes
Comment thread packages/flutter_tools/test/commands.shard/hermetic/logs_test.dart Outdated
jonahwilliams
jonahwilliams approved these changes Jun 4, 2021
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tool changes LGTM

@jmagman

jmagman commented Jun 4, 2021

Copy link
Copy Markdown
Member

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
Comment thread dev/devicelab/bin/run.dart Outdated

@jmagman jmagman Jun 4, 2021

Copy link
Copy Markdown
Member

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 😄

@dnfield dnfield Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes...

@dnfield dnfield Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

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.

@dnfield dnfield Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

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

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread dev/devicelab/lib/framework/host_agent.dart Outdated

@jmagman jmagman Jun 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for doing this.

Comment thread packages/flutter_tools/test/commands.shard/hermetic/logs_test.dart Outdated
@dnfield

dnfield commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

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

@dnfield

dnfield commented Jun 5, 2021

Copy link
Copy Markdown
Contributor Author

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

dnfield commented Jun 5, 2021

Copy link
Copy Markdown
Contributor Author

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

dnfield commented Jun 5, 2021

Copy link
Copy Markdown
Contributor Author

Fixed up the merge conflicts.

jmagman
jmagman approved these changes Jun 7, 2021
View reviewed changes

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

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

chinmaygarde commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

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

dnfield commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

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