Skip to content

support NO_COLOR env var for disabling ANSI #11398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

support NO_COLOR env var for disabling ANSI #11398

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion crates/bevy_log/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ impl Plugin for LogPlugin {
#[cfg(feature = "tracing-tracy")]
let tracy_layer = tracing_tracy::TracyLayer::new();

let fmt_layer = tracing_subscriber::fmt::Layer::default().with_writer(std::io::stderr);
let use_color = std::env::var("NO_COLOR").map(|v| v != "1").unwrap_or(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the env variable is present, color should not be used regardless of its value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I initially thought the same thing. But if you carefully read the nocolor website, command line flags are supposed to override the environment variable. Because of this, the upstream change allows with_ansi() to override the environment variable, otherwise it wouldn’t be possible for command-line flags to override the environment variable.

Longer explanation in this comment in the upstream issue for the PR tokio-rs/tracing#2388 (comment)

Copy link
Contributor

@dmlary dmlary Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated: Ignore this comment; I found the reference. Leaving this incorrect info for posterity.

Bah, they literally just updated the nocolor website a week ago, and the ~~text about flag overriding has been removed. ~~

If you want to push for NO_COLOR to always override commandline flags, I’d open a new PR upstream to fix the behavior there. The reason can be that the NO_COLOR guidelines changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it in the code block on the no-color website. It’s the comment at the bottom of their example code that says flags can override environment.

int
main(int argc, char *argv[])
{
	char *no_color = getenv("NO_COLOR");
	bool color = true;

	if (no_color != NULL && no_color[0] != '\0')
		color = false;

	/* do getopt(3) and/or config-file parsing to possibly turn color back on */
	...
}

Copy link
Member

Choose a reason for hiding this comment

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

from my understanding the check should be on "not empty". if it's set but empty, keep the colours, if it's set and not empty, then remove the colours

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember reading the website a while back and wondering why they didn't say NO_COLOR="true"


let fmt_layer = tracing_subscriber::fmt::Layer::default()
.with_ansi(use_color)
.with_writer(std::io::stderr);

// bevy_render::renderer logs a `tracy.frame_mark` event every frame
// at Level::INFO. Formatted logs should omit it.
Expand Down