Use a 'builder label' for log messages #1100
Conversation
Closes #1069 Replaces the builder `toString()` with a more relevant label, and changes AssetIds to something more reconizable. - Keep a `buildLabel` on `BuildAction`. Where possible this is a `builderKey`, but when that is missing or empty we fall back on the `toString` of the Builder. - Normalize AssetIds to `package:` URIs for non-root packages, and file path for root package. - Don't prefix newlines as messages are logged, these weren't working as implemented and would get swallowed by the StdIoLogger anyway. - Hardcode known `build_runner` Logger names and don't print their name with messages. - Always prefix messages from Builders (loggers with names we don't recognize) with a newline. This works well in the WARNING and SEVERE case but might not work as well for other messages - we can revist if necessary. - Don't print stack traces unless verbose logging is enabled.
|
The hardcoded logger names make me a littler nervous - some of the existing newline magic is also somewhat troubling, but I think it's still worth it to get the behavior we have. This might solve #981 since it hides stack traces by default, but we can still discuss a |
| @@ -60,15 +60,6 @@ class BuildForInputLogger implements Logger { | |||
| if (logLevel >= Level.SEVERE) { | |||
| _errorWasSeen = true; | |||
| } | |||
| if (logLevel >= Level.WARNING) { | |||
There was a problem hiding this comment.
I think we still want this - unless we stopped removing the asset that its running on from the description as well (and maybe even then).
There was a problem hiding this comment.
Header splitting happens where we print now. This code path isn't realistically getting hit now, but if we fixed this we'd end up with a black line before the header, not splitting the header like we intended.
| @@ -60,10 +60,8 @@ main(List<String> args) async { | |||
| var result = await runDart('a', 'tool/build.dart', | |||
| args: ['build', '--delete-conflicting-outputs']); | |||
| expect(result.exitCode, 0, reason: result.stderr as String); | |||
| expect( | |||
| result.stdout, | |||
| contains('BuildDefinition: Invalidating asset graph due to ' | |||
There was a problem hiding this comment.
are these names no longer present or you just wanted to simplify the tests? I think they do provide some value personally (the names being there, I am ambivalent about the tests).
There was a problem hiding this comment.
They are no longer present. I felt like they mainly added value for us. The end user probably didn't care about the particulars ways we split up functionality in this package or the labels we gave them.
There was a problem hiding this comment.
It gave you some indication of where the log was coming from but its probably true that for most users that didn't mean much.... I am ok with removing them.
|
Should have read the full description first :). The main thing I don't think we want is always prefixing messages with a newline - I believe that will break the existing logic that overwrites messages under WARNING level? |
| 'BuildDefinition', | ||
| 'Heartbeat' | ||
| ]; | ||
| return verbose || !knownNames.contains(record.loggerName) |
There was a problem hiding this comment.
looks like this is where we would only add the newline for >= WARNING?
There was a problem hiding this comment.
Yeah, I can add a level check here.
|
@jakemac53 - now only splitting on WARNING and above - any other concerns? |
Closes #1069
Replaces the builder
toString()with a more relevant label, andchanges AssetIds to something more reconizable.
buildLabelonBuildAction. Where possible this is abuilderKey, but when that is missing or empty we fall back on thetoStringof the Builder.package:URIs for non-root packages, and filepath for root package.
implemented and would get swallowed by the StdIoLogger anyway.
build_runnerLogger names and don't print their namewith messages.
recognize) with a newline. This works well in the WARNING and SEVERE
case but might not work as well for other messages - we can revist if
necessary.