From 8fa10c9bbc9e56f6ce8426f976230f0737b1841d Mon Sep 17 00:00:00 2001 From: "David M. Lary" Date: Wed, 12 Jul 2023 19:24:50 -0500 Subject: [PATCH 1/6] fmt::Subscriber `NO_COLOR` support The `NO_COLOR` environment variable should disable all ANSI color output (https:://no-color.org). This commit updates `fmt::Subscriber` to check the `NO_COLOR` environment variable before enabling ANSI color output. As described in the spec, any non-empty value set for `NO_COLOR` will prevent `is_ansi` from being set to `true`. fixes #2388 --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 74 +++++++++++++++++++- 1 file changed, 72 insertions(+), 2 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index 366660fb82..ec14081188 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -6,7 +6,7 @@ use crate::{ }; use format::{FmtSpan, TimingDisplay}; use std::{ - any::TypeId, cell::RefCell, fmt, io, marker::PhantomData, ops::Deref, ptr::NonNull, + any::TypeId, cell::RefCell, env, fmt, io, marker::PhantomData, ops::Deref, ptr::NonNull, time::Instant, }; use tracing_core::{ @@ -234,9 +234,15 @@ impl Subscriber { /// This method is primarily expected to be used with the /// [`reload::Handle::modify`](crate::reload::Handle::modify) method when changing /// the writer. + /// + /// If the `NO_COLOR` environment variable is set to a non-empty value, + /// `with_ansi(true)` will have no effect. + /// + /// [`NO_COLOR` environment variable]: https://no-color.org/ #[cfg(feature = "ansi")] #[cfg_attr(docsrs, doc(cfg(feature = "ansi")))] pub fn set_ansi(&mut self, ansi: bool) { + let ansi = ansi & env::var("NO_COLOR").map_or(true, |v| v.is_empty()); self.is_ansi = ansi; } @@ -282,6 +288,9 @@ impl Subscriber { /// feature flag enabled will panic if debug assertions are enabled, or /// print a warning otherwise. /// + /// If the `NO_COLOR` environment variable is set to a non-empty value, + /// `with_ansi(true)` will have no effect. + /// /// This method itself is still available without the feature flag. This /// is to allow ANSI escape codes to be explicitly *disabled* without /// having to opt-in to the dependencies required to emit ANSI formatting. @@ -289,6 +298,8 @@ impl Subscriber { /// ANSI escape codes can ensure that they are not used, regardless of /// whether or not other crates in the dependency graph enable the "ansi" /// feature flag. + /// + /// [`NO_COLOR` environment variable]: https://no-color.org/ pub fn with_ansi(self, ansi: bool) -> Self { #[cfg(not(feature = "ansi"))] if ansi { @@ -300,6 +311,7 @@ impl Subscriber { eprintln!("{}", ERROR); } + let ansi = ansi & env::var("NO_COLOR").map_or(true, |v| v.is_empty()); Subscriber { is_ansi: ansi, ..self @@ -680,12 +692,16 @@ impl Subscriber { impl Default for Subscriber { fn default() -> Self { + // only enable ANSI when the feature is enabled, and the NO_COLOR + // environment variable is unset or empty. + let ansi = cfg!(feature = "ansi") && env::var("NO_COLOR").map_or(true, |v| v.is_empty()); + Subscriber { fmt_fields: format::DefaultFields::default(), fmt_event: format::Format::default(), fmt_span: format::FmtSpanConfig::default(), make_writer: io::stdout, - is_ansi: cfg!(feature = "ansi"), + is_ansi: ansi, log_internal_errors: false, _inner: PhantomData, } @@ -1505,4 +1521,58 @@ mod test { actual.as_str() ); } + + // Because we need to modify an environment variable for these test cases, + // we do them all in a single test. + #[cfg(feature = "ansi")] + #[test] + fn subscriber_no_color() { + // save off the existing value of NO_COLOR + const NO_COLOR: &str = "NO_COLOR"; + let saved_no_color = std::env::var(NO_COLOR); + + let cases: Vec<(Option<&str>, bool)> = vec![ + (Some("0"), false), // any non-empty value disables ansi + (Some("off"), false), // any non-empty value disables ansi + (Some("1"), false), + (Some(""), true), // empty value does not disable ansi + (None, true), + ]; + + for (var, ansi) in cases { + if let Some(value) = var { + env::set_var(NO_COLOR, value); + } else { + env::remove_var(NO_COLOR); + } + + let subscriber: Subscriber<()> = fmt::Subscriber::default(); + assert_eq!( + subscriber.is_ansi, ansi, + "NO_COLOR={:?}; Subscriber::default().is_ansi should be {}", + var, ansi + ); + + let subscriber: Subscriber<()> = fmt::Subscriber::default().with_ansi(true); + assert_eq!( + subscriber.is_ansi, ansi, + "NO_COLOR={:?}; Subscriber::default().with_ansi(true).is_ansi should be {}", + var, ansi + ); + + let mut subscriber: Subscriber<()> = fmt::Subscriber::default(); + subscriber.set_ansi(true); + assert_eq!( + subscriber.is_ansi, ansi, + "NO_COLOR={:?}; subscriber.set_ansi(true); subscriber.is_ansi should be {}", + var, ansi + ); + } + + if let Ok(value) = saved_no_color { + env::set_var(NO_COLOR, value); + } else { + env::remove_var(NO_COLOR); + } + } } From f9f3f8c45d3bbd800dff23d3551f611badf590e5 Mon Sep 17 00:00:00 2001 From: "David M. Lary" Date: Wed, 12 Jul 2023 19:55:07 -0500 Subject: [PATCH 2/6] update to allow `NO_COLOR` to be overridden Re-reading https://no-color.org showed that they do allow overriding of the setting with command-line flags. Similarly, I've removed the changes to `fmt::subscriber::with_ansi` and `fmt::subscriber::set_ansi`; they will once again set `is_ansi` regardless of `NO_COLOR`. --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 30 +++++++------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index ec14081188..4544481b3c 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -234,15 +234,9 @@ impl Subscriber { /// This method is primarily expected to be used with the /// [`reload::Handle::modify`](crate::reload::Handle::modify) method when changing /// the writer. - /// - /// If the `NO_COLOR` environment variable is set to a non-empty value, - /// `with_ansi(true)` will have no effect. - /// - /// [`NO_COLOR` environment variable]: https://no-color.org/ #[cfg(feature = "ansi")] #[cfg_attr(docsrs, doc(cfg(feature = "ansi")))] pub fn set_ansi(&mut self, ansi: bool) { - let ansi = ansi & env::var("NO_COLOR").map_or(true, |v| v.is_empty()); self.is_ansi = ansi; } @@ -288,9 +282,6 @@ impl Subscriber { /// feature flag enabled will panic if debug assertions are enabled, or /// print a warning otherwise. /// - /// If the `NO_COLOR` environment variable is set to a non-empty value, - /// `with_ansi(true)` will have no effect. - /// /// This method itself is still available without the feature flag. This /// is to allow ANSI escape codes to be explicitly *disabled* without /// having to opt-in to the dependencies required to emit ANSI formatting. @@ -298,8 +289,6 @@ impl Subscriber { /// ANSI escape codes can ensure that they are not used, regardless of /// whether or not other crates in the dependency graph enable the "ansi" /// feature flag. - /// - /// [`NO_COLOR` environment variable]: https://no-color.org/ pub fn with_ansi(self, ansi: bool) -> Self { #[cfg(not(feature = "ansi"))] if ansi { @@ -311,7 +300,6 @@ impl Subscriber { eprintln!("{}", ERROR); } - let ansi = ansi & env::var("NO_COLOR").map_or(true, |v| v.is_empty()); Subscriber { is_ansi: ansi, ..self @@ -1553,19 +1541,21 @@ mod test { var, ansi ); + // with_ansi should override any `NO_COLOR` value let subscriber: Subscriber<()> = fmt::Subscriber::default().with_ansi(true); - assert_eq!( - subscriber.is_ansi, ansi, - "NO_COLOR={:?}; Subscriber::default().with_ansi(true).is_ansi should be {}", - var, ansi + assert!( + subscriber.is_ansi, + "NO_COLOR={:?}; Subscriber::default().with_ansi(true).is_ansi should be true", + var ); + // set_ansi should override any `NO_COLOR` value let mut subscriber: Subscriber<()> = fmt::Subscriber::default(); subscriber.set_ansi(true); - assert_eq!( - subscriber.is_ansi, ansi, - "NO_COLOR={:?}; subscriber.set_ansi(true); subscriber.is_ansi should be {}", - var, ansi + assert!( + subscriber.is_ansi, + "NO_COLOR={:?}; subscriber.set_ansi(true); subscriber.is_ansi should be true", + var ); } From 9bb797c66bc54922d29c616f69118dc915b45707 Mon Sep 17 00:00:00 2001 From: "David M. Lary" Date: Tue, 15 Aug 2023 18:05:08 -0500 Subject: [PATCH 3/6] use a Drop guard in NO_COLOR tests Co-authored-by: Eliza Weisman --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 28 +++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index 4544481b3c..bf287143e7 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -1515,9 +1515,26 @@ mod test { #[cfg(feature = "ansi")] #[test] fn subscriber_no_color() { - // save off the existing value of NO_COLOR + const NO_COLOR: &str = "NO_COLOR"; - let saved_no_color = std::env::var(NO_COLOR); + + // Restores the previous value of the `NO_COLOR` env variable when + // dropped. + // + // This is done in a `Drop` implementation, rather than just resetting + // the value at the end of the test, so that the previous value is + // restored even if the test panics. + struct RestoreEnvVar(Result); + impl Drop for RestoreEnvVar { + fn drop(&mut self) { + match self.0 { + Ok(ref var) => env::set_var(NO_COLOR, var), + Err(_) => env::remove_var(NO_COLOR), + } + } + } + + let _saved_no_color = RestoreEnvVar(env::var(NO_COLOR)); let cases: Vec<(Option<&str>, bool)> = vec![ (Some("0"), false), // any non-empty value disables ansi @@ -1559,10 +1576,7 @@ mod test { ); } - if let Ok(value) = saved_no_color { - env::set_var(NO_COLOR, value); - } else { - env::remove_var(NO_COLOR); - } + // dropping `_saved_no_color` will restore the previous value of + // `NO_COLOR`. } } From e56544a0daa35e3dec0e09e937cd2258a4b0aa69 Mon Sep 17 00:00:00 2001 From: "David M. Lary" Date: Tue, 15 Aug 2023 18:06:35 -0500 Subject: [PATCH 4/6] Update ANSI behavior documentation for NO_COLOR --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index bf287143e7..ba81ac24af 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -277,6 +277,13 @@ impl Subscriber { /// Sets whether or not the formatter emits ANSI terminal escape codes /// for colors and other text formatting. /// + /// When the "ansi" crate feature flag is enabled, ANSI colors are enabled + /// by default unless the `NO_COLOR` environment variable is set to + /// a non-empty value. If the `NO_COLOR` environment variable is set to + /// any non-empty value, then ANSI colors will be suppressed by default. + /// The [`with_ansi`] and [`set_ansi`] methods can be used to forcibly + /// enable ANSI colors, overriding any `NO_COLOR` environment variable. + /// /// Enabling ANSI escapes (calling `with_ansi(true)`) requires the "ansi" /// crate feature flag. Calling `with_ansi(true)` without the "ansi" /// feature flag enabled will panic if debug assertions are enabled, or From b5c40b715a570e4d5dfe7ba2daa243a92652da9b Mon Sep 17 00:00:00 2001 From: "David M. Lary" Date: Wed, 16 Aug 2023 18:05:08 -0500 Subject: [PATCH 5/6] fix to NO_COLOR documentation --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index ba81ac24af..29e9418c4e 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -296,6 +296,9 @@ impl Subscriber { /// ANSI escape codes can ensure that they are not used, regardless of /// whether or not other crates in the dependency graph enable the "ansi" /// feature flag. + /// + /// [`with_ansi`]: Subscriber::with_ansi + /// [`set_ansi`]: Subscriber::set_ansi pub fn with_ansi(self, ansi: bool) -> Self { #[cfg(not(feature = "ansi"))] if ansi { @@ -1522,7 +1525,6 @@ mod test { #[cfg(feature = "ansi")] #[test] fn subscriber_no_color() { - const NO_COLOR: &str = "NO_COLOR"; // Restores the previous value of the `NO_COLOR` env variable when @@ -1583,7 +1585,7 @@ mod test { ); } - // dropping `_saved_no_color` will restore the previous value of - // `NO_COLOR`. + // dropping `_saved_no_color` will restore the previous value of + // `NO_COLOR`. } } From 218f9c07c387f14592cd4ffce70d0dfb602dc419 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 17 Aug 2023 09:39:11 -0700 Subject: [PATCH 6/6] Update tracing-subscriber/src/fmt/fmt_subscriber.rs --- tracing-subscriber/src/fmt/fmt_subscriber.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_subscriber.rs b/tracing-subscriber/src/fmt/fmt_subscriber.rs index 29e9418c4e..64f9dfc1bb 100644 --- a/tracing-subscriber/src/fmt/fmt_subscriber.rs +++ b/tracing-subscriber/src/fmt/fmt_subscriber.rs @@ -278,11 +278,13 @@ impl Subscriber { /// for colors and other text formatting. /// /// When the "ansi" crate feature flag is enabled, ANSI colors are enabled - /// by default unless the `NO_COLOR` environment variable is set to - /// a non-empty value. If the `NO_COLOR` environment variable is set to + /// by default unless the [`NO_COLOR`] environment variable is set to + /// a non-empty value. If the [`NO_COLOR`] environment variable is set to /// any non-empty value, then ANSI colors will be suppressed by default. /// The [`with_ansi`] and [`set_ansi`] methods can be used to forcibly - /// enable ANSI colors, overriding any `NO_COLOR` environment variable. + /// enable ANSI colors, overriding any [`NO_COLOR`] environment variable. + /// + /// [`NO_COLOR`]: https://no-color.org/ /// /// Enabling ANSI escapes (calling `with_ansi(true)`) requires the "ansi" /// crate feature flag. Calling `with_ansi(true)` without the "ansi"