Stream logs to file using process.start. #227
Conversation
Fuchsia_ctl currently uses process.run to run commands this forcs the tool to wait until the process completes to collect the results.
godofredoc
requested review from
dnfield,
iskakaushik,
zanderso and
chaselatta
packages/fuchsia_ctl/bin/main.dart
Outdated
| @@ -186,7 +189,8 @@ Future<OperationResult> ssh( | |||
| identityFilePath: identityFile, | |||
| command: args['command'].split(' '), | |||
| timeoutMs: | |||
| Duration(milliseconds: int.parse(args['timeout-seconds']) * 1000)); | |||
| Duration(milliseconds: int.parse(args['timeout-seconds']) * 1000), | |||
| logFilePath: outputFile); | |||
There was a problem hiding this comment.
nit: trailing comma will make this format a bit more nicely
| /// level and message. | ||
| String toLogString(String message, {LogLevel level}) { | ||
| final StringBuffer buffer = StringBuffer(); | ||
| buffer.write(DateTime.now().toIso8601String()); |
There was a problem hiding this comment.
| buffer.write(DateTime.now().toIso8601String()); | |
| buffer.write(DateTime.now().toUtc().toIso8601String()); |
| Duration timeoutMs = defaultSshTimeoutMs}) async { | ||
| Duration timeoutMs = defaultSshTimeoutMs, | ||
| String logFilePath, | ||
| FileSystem fs}) async { |
There was a problem hiding this comment.
nit: trailing comma for arg list
| if (!logToFile) { | ||
| logFile.writeln(log); | ||
| } else { | ||
| logger.info(log); | ||
| } |
There was a problem hiding this comment.
Why can't we just do logger.info(log) here? Won't it write to the file if it's attached to the IOSink, and otherwise jut write to stdout?
There was a problem hiding this comment.
Done. It was implemented like that because logfile adds datetime and log level and all the ssh commands in fuchsia_ctl expects output with no additional prepended.
| if (!logToFile) { | ||
| logFile.writeln(log); | ||
| } else { | ||
| logger.warning(log); |
There was a problem hiding this comment.
Updated to error.
| fileSystem = MemoryFileSystem(); | ||
| fileSystem.file('logs')..createSync(); | ||
| logFile = fileSystem.file('logs').openWrite(); |
There was a problem hiding this comment.
This looks like it's not used below and the output will just be an empty string if no log file is specified.
I think we might be better off having some kind of BufferedLogger class that would allow us to retrieve the buffer as well as writing it to a file if needed.
Alternatively, if we're logging this to a file/stdout, do we really need to return stdout/stderr back to the caller?
There was a problem hiding this comment.
Removed from here and moved the functionality to the logger class.
packages/fuchsia_ctl/pubspec.yaml
Outdated
| @@ -14,10 +14,11 @@ dependencies: | |||
| file: ^5.0.10 | |||
| meta: ^1.1.7 | |||
| path: ^1.6.4 | |||
| pedantic: ^1.9.2 | |||
There was a problem hiding this comment.
Please avoid having pedantic as a direct dependency - use it as a dev dependency instead. Although this package is not consumed by anyone, this can cause problems if we later need to use some other package that's also using pedantic with an incompatible constraint. Pedantic's semver constraints are really more about the analysis option rules anyway, rather than the unawaited function.
We should just write our own unawaited function, which is literally one line of code, where we need it.
| void main() { | ||
| test('', () async {}); | ||
| } |
There was a problem hiding this comment.
Maybe forgot to save before committing? :)
There was a problem hiding this comment.
Yep uploaded the version with the tests.
| /// LogLevel for messages instended for debugging. | ||
| static const LogLevel debug = LogLevel._(0, 'DEBUG'); | ||
|
|
||
| /// LogLevel for messages instended to provide information about the exection. |
#227) * oops * Fix scientific notation parsing * Update packages/vector_graphics_compiler/test/parsers_test.dart Co-authored-by: Jonah Williams <jonahwilliams@google.com> * Fix up analysis issues and half implemented test --------- Co-authored-by: Jonah Williams <jonahwilliams@google.com>
Fuchsia_ctl currently uses process.run to run commands this forces the tool to wait until the process
completes to collect the results.