Wasmtime: omit ANSI color sequences in logging when not a terminal. #7436
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In bytecodealliance#7239 we added a `tracing-log` subscriber that prints logs to stderr if enabled with an environment variable. It included logic to add ANSI color sequences when stderr is a terminal, for legibility. Unfortunately it seems that while this logic *enabled* colors on a terminal, it did not *disable* colors on a non-terminal; so redirects of stderr to a file would result in ANSI color sequences being captured in that file. Specifically, the builder seems not to default to no-color; so rather than enable-or-nothing, we should explicitly enable or disable always. Fixes bytecodealliance#7435.
alexcrichton
approved these changes
Nov 1, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Thanks, I should have tested for this case instead of assuming it would be fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #7239 we added a
tracing-logsubscriber that prints logs to stderr if enabled with an environment variable. It included logic to add ANSI color sequences when stderr is a terminal, for legibility. Unfortunately it seems that while this logic enabled colors on a terminal, it did not disable colors on a non-terminal; so redirects of stderr to a file would result in ANSI color sequences being captured in that file. Specifically, the builder seems not to default to no-color; so rather than enable-or-nothing, we should explicitly enable or disable always.Fixes #7435.