From aa44576964ca61b00a284be413e6945a1cb13691 Mon Sep 17 00:00:00 2001 From: Max Carr Date: Thu, 18 Apr 2024 16:16:59 -0700 Subject: [PATCH] switch from `termcolor` to `anstream` and `anstyle` (#737) * ready for some initial feedback * soliciting feedback here * switch to `colorchoice-clap` crate for color handling it still breaks tests, i think it's an issue with how we handle the `check-release` subcommand * store stdout and stderr in config * add some tests Squashed commit of the following: commit 59a3a7f717224019303b3d0c084499f3e4747dc1 Author: m Date: Fri Apr 12 15:15:42 2024 -0700 add some simple tests commit 03b2494662cf2ba36bbc841c29df48abca226bad Author: m Date: Fri Apr 12 14:16:55 2024 -0700 current work * fix clippy * (re-)add ColorChoice::global & CARGO_TERM_COLOR - document that `ColorChoice::write_global` must be called before `GlobalConfig::new` or `set_std{err,out}` - add tests for `GlobalConfig` - add parsing for `CARGO_TERM_COLOR` env var in binary part of crate * Apply suggestions from code review Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> * make `configure_color` more clear * clarify rustdoc child process color setting * handle env vars with flags we had to make our own colorchoice clap handler because the mixin doesn't work with Option<_> * make `Check::check_release` take &mut GlobalConfig * respect our color preference in `rustdoc` * minor: save config.stderr() when we use it a lot this also cleans up lines that get unnecessarily changed in the diff --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> --- Cargo.lock | 29 +--- Cargo.toml | 4 +- src/check_release.rs | 173 ++++++++++----------- src/config.rs | 361 +++++++++++++++++++++++++++++++++++-------- src/lib.rs | 54 +------ src/main.rs | 73 ++++++--- src/rustdoc_cmd.rs | 22 +-- tests/lib_tests.rs | 7 +- 8 files changed, 451 insertions(+), 272 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ea28d41..cfa77ab5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -285,6 +285,8 @@ dependencies = [ name = "cargo-semver-checks" version = "0.30.0" dependencies = [ + "anstream", + "anstyle", "anyhow", "assert_cmd", "atty", @@ -311,8 +313,6 @@ dependencies = [ "sha2", "similar-asserts", "tame-index", - "termcolor", - "termcolor_output", "toml", "trustfall", "trustfall_rustdoc", @@ -2654,31 +2654,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "termcolor" -version = "1.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "06794f8f6c5c898b3275aebefa6b8a1cb24cd2c6c79397ab15774837a0bc5755" -dependencies = [ - "winapi-util", -] - -[[package]] -name = "termcolor_output" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0363afbf20990ea53a69c03b71800480aaf90e8f49f6fd5385ecc302062895ff" -dependencies = [ - "termcolor", - "termcolor_output_impl", -] - -[[package]] -name = "termcolor_output_impl" -version = "1.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f34dde0bb841eb3762b42bdff8db11bbdbc0a3bd7b32012955f5ce1d081f86c1" - [[package]] name = "termtree" version = "0.4.1" diff --git a/Cargo.toml b/Cargo.toml index 975a7047..af01b256 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,8 +24,6 @@ serde = { version = "1.0.185", features = ["derive"] } semver = "1.0.12" handlebars = "5.1.0" atty = "0.2.14" -termcolor = "1.1.3" -termcolor_output = "1.0.1" cargo_metadata = "0.18.1" clap-cargo = { version = "0.14.0", features = ["cargo_metadata"] } ignore = "0.4.18" @@ -45,6 +43,8 @@ directories = "5.0.1" sha2 = "0.10.6" rustc_version = "0.4.0" rayon = "1.8.0" +anstyle = "1.0.6" +anstream = "0.6.13" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/check_release.rs b/src/check_release.rs index d3652e9b..1d6ddccf 100644 --- a/src/check_release.rs +++ b/src/check_release.rs @@ -1,11 +1,12 @@ +use std::io::Write as _; use std::{collections::BTreeMap, sync::Arc, time::Instant}; +use anstyle::{AnsiColor, Color, Reset, Style}; + use anyhow::Context; use clap::crate_version; use itertools::Itertools; use rayon::prelude::*; -use termcolor::Color; -use termcolor_output::{colored, colored_ln}; use trustfall::TransparentValue; use trustfall_rustdoc::{VersionedCrate, VersionedIndexedCrate, VersionedRustdocAdapter}; @@ -163,33 +164,31 @@ pub(super) fn run_check_release( RequiredSemverUpdate::Minor => "minor", }; if results.is_empty() { - colored_ln(config.stderr(), |w| { - colored!( - w, - "{}{}{:>12}{} [{:>8.3}s] {:^18} {}", - fg!(Some(Color::Green)), - bold!(true), - "PASS", - reset!(), - time_to_decide.as_secs_f32(), - category, - semver_query.id, - ) - })?; + writeln!( + config.stderr(), + "{}{:>12}{} [{:8.3}s] {:^18} {}", + Style::new() + .fg_color(Some(Color::Ansi(AnsiColor::Green))) + .bold(), + "PASS", + Reset, + time_to_decide.as_secs_f32(), + category, + semver_query.id + )?; } else { - colored_ln(config.stderr(), |w| { - colored!( - w, - "{}{}{:>12}{} [{:>8.3}s] {:^18} {}", - fg!(Some(Color::Red)), - bold!(true), - "FAIL", - reset!(), - time_to_decide.as_secs_f32(), - category, - semver_query.id, - ) - })?; + writeln!( + config.stderr(), + "{}{:>12}{} [{:>8.3}s] {:^18} {}", + Style::new() + .fg_color(Some(Color::Ansi(AnsiColor::Red))) + .bold(), + "FAIL", + Reset, + time_to_decide.as_secs_f32(), + category, + semver_query.id + )?; } Ok(()) }) @@ -211,7 +210,7 @@ pub(super) fn run_check_release( results_with_errors.len(), skipped_queries, ), - Color::Red, + Color::Ansi(AnsiColor::Red), true, ) .expect("print failed"); @@ -222,57 +221,42 @@ pub(super) fn run_check_release( required_versions.push(semver_query.required_update); config .log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!( - w, - "\n--- failure {}: {} ---\n", - &semver_query.id, - &semver_query.human_readable_name, - ) - })?; + writeln!( + config.stdout(), + "\n--- failure {}: {} ---\n", + &semver_query.id, + &semver_query.human_readable_name + )?; Ok(()) }) .expect("print failed"); if let Some(ref_link) = semver_query.reference_link.as_deref() { config.log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!( - w, - "{}Description:{}\n{}\n{:>12} {}\n{:>12} {}\n", - bold!(true), - reset!(), - &semver_query.error_message, - "ref:", - ref_link, - "impl:", - format!( - "https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", - crate_version!(), - semver_query.id, - ) - ) - })?; + writeln!(config.stdout(), "{}Description:{}\n{}\n{:>12} {}\n{:>12} https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron\n", + Style::new().bold(), Reset, + &semver_query.error_message, + "ref:", + ref_link, + "impl:", + crate_version!(), + semver_query.id, + )?; Ok(()) }) .expect("print failed"); } else { config.log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!( - w, - "{}Description:{}\n{}\n{:>12} {}\n", - bold!(true), - reset!(), - &semver_query.error_message, - "impl:", - format!( - "https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", - crate_version!(), - semver_query.id, - ) - ) - })?; + writeln!( + config.stdout(), + "{}Description:{}\n{}\n{:>12} https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", + Style::new().bold(), + Reset, + &semver_query.error_message, + "impl:", + crate_version!(), + semver_query.id, + )?; Ok(()) }) .expect("print failed"); @@ -280,9 +264,12 @@ pub(super) fn run_check_release( config .log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!(w, "{}Failed in:{}", bold!(true), reset!()) - })?; + writeln!( + config.stdout(), + "{}Failed in:{}", + Style::new().bold(), + Reset + )?; Ok(()) }) .expect("print failed"); @@ -301,37 +288,35 @@ pub(super) fn run_check_release( .expect("could not materialize template"); config .log_info(|config| { - colored_ln(config.stdout(), |w| colored!(w, " {}", message,))?; + writeln!(config.stdout(), " {}", message)?; Ok(()) }) .expect("print failed"); config .log_extra_verbose(|config| { - colored_ln(config.stdout(), |w| { - let serde_pretty = serde_json::to_string_pretty(&pretty_result) - .expect("serde failed"); - let indented_serde = serde_pretty - .split('\n') - .map(|line| format!(" {line}")) - .join("\n"); - colored!(w, " lint rule output values:\n{}", indented_serde) - }) - .map_err(|e| e.into()) + let serde_pretty = + serde_json::to_string_pretty(&pretty_result).expect("serde failed"); + let indented_serde = serde_pretty + .split('\n') + .map(|line| format!(" {line}")) + .join("\n"); + writeln!( + config.stdout(), + "\tlint rule output values:\n{}", + indented_serde + )?; + Ok(()) }) .expect("print failed"); } else { config .log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!( - w, - "{}\n", - serde_json::to_string_pretty(&pretty_result) - .expect("serde failed"), - ) - }) - .expect("print failed"); + writeln!( + config.stdout(), + "{}\n", + serde_json::to_string_pretty(&pretty_result)? + )?; Ok(()) }) .expect("print failed"); @@ -362,7 +347,7 @@ pub(super) fn run_check_release( .filter(|x| *x == &RequiredSemverUpdate::Minor) .count(), ), - Color::Red, + Color::Ansi(AnsiColor::Red), true, ) .expect("print failed"); @@ -382,7 +367,7 @@ pub(super) fn run_check_release( queries_to_run.len(), skipped_queries, ), - Color::Green, + Color::Ansi(AnsiColor::Green), true, ) .expect("print failed"); diff --git a/src/config.rs b/src/config.rs index c12743eb..aa9de2fb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,18 +1,19 @@ -use termcolor::{ColorChoice, StandardStream}; +use anstream::{AutoStream, ColorChoice}; +use anstyle::{AnsiColor, Color, Reset, Style}; +use std::io::Write; use crate::templating::make_handlebars_registry; #[allow(dead_code)] pub struct GlobalConfig { level: Option, - is_stderr_tty: bool, - stdout: StandardStream, - stderr: StandardStream, handlebars: handlebars::Handlebars<'static>, /// Minimum rustc version supported. /// /// This will be used to print an error if the user's rustc version is not high enough. minimum_rustc_version: semver::Version, + stdout: AutoStream>, + stderr: AutoStream>, } impl Default for GlobalConfig { @@ -22,37 +23,21 @@ impl Default for GlobalConfig { } impl GlobalConfig { + /// Creates a new `GlobalConfig` instance. + /// + /// Reads color choice from the value set by [`ColorChoice::write_global`] at the time + /// of creation; see [`GlobalConfig::set_color_choice`] for finer-grained control over + /// `cargo-semver-checks`'s color output pub fn new() -> Self { - let is_stdout_tty = atty::is(atty::Stream::Stdout); - let is_stderr_tty = atty::is(atty::Stream::Stderr); - - let color_choice = match std::env::var("CARGO_TERM_COLOR").as_deref() { - Ok("always") => Some(ColorChoice::Always), - Ok("alwaysansi") => Some(ColorChoice::AlwaysAnsi), - Ok("auto") => Some(ColorChoice::Auto), - Ok("never") => Some(ColorChoice::Never), - Ok(_) | Err(..) => None, - }; + let stdout_choice = anstream::stdout().current_choice(); + let stderr_choice = anstream::stdout().current_choice(); Self { level: None, - is_stderr_tty, - stdout: StandardStream::stdout(color_choice.unwrap_or({ - if is_stdout_tty { - ColorChoice::Auto - } else { - ColorChoice::Never - } - })), - stderr: StandardStream::stderr(color_choice.unwrap_or({ - if is_stderr_tty { - ColorChoice::Auto - } else { - ColorChoice::Never - } - })), handlebars: make_handlebars_registry(), minimum_rustc_version: semver::Version::new(1, 74, 0), + stdout: AutoStream::new(Box::new(std::io::stdout()), stdout_choice), + stderr: AutoStream::new(Box::new(std::io::stderr()), stderr_choice), } } @@ -125,52 +110,23 @@ impl GlobalConfig { Ok(()) } - pub fn is_stderr_tty(&self) -> bool { - self.is_stderr_tty - } - - pub fn stdout(&mut self) -> &mut StandardStream { - &mut self.stdout - } - - pub fn stderr(&mut self) -> &mut StandardStream { - &mut self.stderr - } - - pub fn set_color_choice(mut self, choice: ColorChoice) -> Self { - self.stdout = StandardStream::stdout(choice); - self.stderr = StandardStream::stderr(choice); - self - } - /// Print a message with a colored title in the style of Cargo shell messages. pub fn shell_print( &mut self, status: impl std::fmt::Display, message: impl std::fmt::Display, - color: termcolor::Color, + color: anstyle::Color, justified: bool, ) -> anyhow::Result<()> { if self.is_info() { - use std::io::Write; - use termcolor::WriteColor; - - self.stderr().set_color( - termcolor::ColorSpec::new() - .set_fg(Some(color)) - .set_bold(true), - )?; + write!(self.stderr, "{}", Style::new().fg_color(Some(color)).bold())?; if justified { - write!(self.stderr(), "{status:>12}")?; + write!(self.stderr, "{status:>12}")?; } else { - write!(self.stderr(), "{status}")?; - self.stderr() - .set_color(termcolor::ColorSpec::new().set_bold(true))?; - write!(self.stderr(), ":")?; + write!(self.stderr, "{status}{}{}:", Reset, Style::new().bold())?; } - self.stderr().reset()?; - writeln!(self.stderr(), " {message}")?; + writeln!(self.stderr, "{Reset} {message}")?; } Ok(()) @@ -182,22 +138,224 @@ impl GlobalConfig { action: impl std::fmt::Display, message: impl std::fmt::Display, ) -> anyhow::Result<()> { - self.shell_print(action, message, termcolor::Color::Green, true) + self.shell_print(action, message, Color::Ansi(AnsiColor::Green), true) } pub fn shell_note(&mut self, message: impl std::fmt::Display) -> anyhow::Result<()> { - self.shell_print("note", message, termcolor::Color::Cyan, false) + self.shell_print("note", message, Color::Ansi(AnsiColor::Cyan), false) } pub fn shell_warn(&mut self, message: impl std::fmt::Display) -> anyhow::Result<()> { - self.shell_print("warning", message, termcolor::Color::Yellow, false) + self.shell_print("warning", message, Color::Ansi(AnsiColor::Yellow), false) + } + + /// Gets the color-supporting `stdout` that the crate will use. + /// + /// See [`GlobalConfig::set_stdout`] and [`GlobalConfig::set_out_color_choice`] to + /// configure this stream + #[must_use] + #[inline] + pub fn stdout(&mut self) -> impl Write + '_ { + &mut self.stdout + } + + /// Gets the color-supporting `stderr` that the crate will use. + /// + /// See [`GlobalConfig::set_stderr`] and [`GlobalConfig::set_err_color_choice`] to + /// configure this stream + #[must_use] + #[inline] + pub fn stderr(&mut self) -> impl Write + '_ { + &mut self.stderr + } + + /// Sets the stderr output stream + /// + /// Defaults to the global color choice setting set by [`ColorChoice::write_global`] + /// *at the time of calling `set_stderr`*. + /// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice after if needed. + pub fn set_stderr(&mut self, err: Box) { + self.stderr = AutoStream::auto(err); + } + + /// Sets the stdout output stream + /// + /// Defaults to the global color choice setting set by [`ColorChoice::write_global`]. + /// *at the time of calling `set_stdout`*. + /// Call [`GlobalConfig::set_out_color_choice`] to customize the color choice after if needed. + pub fn set_stdout(&mut self, out: Box) { + self.stdout = AutoStream::auto(out); + } + + /// Individually set the color choice setting for [`GlobalConfig::stderr`] + /// + /// Defaults to the global color choice in [`ColorChoice::global`], which can be set + /// in [`ColorChoice::write_global`] if you are using the `anstream` crate. + /// + /// See also [`GlobalConfig::set_out_color_choice`] and [`GlobalConfig::set_color_choice`] + pub fn set_err_color_choice(&mut self, use_color: bool) { + // `anstream` doesn't have a good mechanism to set color choice (on one stream) + // without making a new object, so we have to make a new autostream, but since we need + // to move the `RawStream` inner, we temporarily replace it with /dev/null + let stderr = std::mem::replace( + &mut self.stderr, + AutoStream::never(Box::new(std::io::sink())), + ); + self.stderr = AutoStream::new( + stderr.into_inner(), + if use_color { + ColorChoice::Always + } else { + ColorChoice::Never + }, + ); + } + + /// Individually set the color choice setting for [`GlobalConfig::stdout`] + /// + /// Defaults to the global color choice in [`ColorChoice::global`], which can be set + /// in [`ColorChoice::write_global`] if you are using the `anstream` crate. + /// + /// See also [`GlobalConfig::set_err_color_choice`] and [`GlobalConfig::set_color_choice`] + pub fn set_out_color_choice(&mut self, use_color: bool) { + // `anstream` doesn't have a good mechanism to set color choice (on one stream) + // without making a new object, so we have to make a new autostream, but since we need + // to move the `RawStream` inner, we temporarily replace it with /dev/null + let stdout = std::mem::replace( + &mut self.stdout, + AutoStream::never(Box::new(std::io::sink())), + ); + self.stdout = AutoStream::new( + stdout.into_inner(), + if use_color { + ColorChoice::Always + } else { + ColorChoice::Never + }, + ); + } + + /// Sets the color choice for both [`GlobalConfig::stderr`] and [`GlobalConfig::stdout`] + /// + /// If not set, defaults to the value in [`ColorChoice::global`] at the time the streams + /// are set using [`GlobalConfig::set_stdout`] and `err`, which can be set beforehand + /// + /// See also [`GlobalConfig::set_err_color_choice`] and [`GlobalConfig::set_out_color_choice`] + pub fn set_color_choice(&mut self, use_color: bool) { + self.set_err_color_choice(use_color); + self.set_out_color_choice(use_color); + } + + /// Gets the color choice (i.e., whether to output colors) for the configured stderr + /// + /// See also [`GlobalConfig::set_err_color_choice`] + #[must_use] + #[inline] + pub fn err_color_choice(&self) -> bool { + match &self.stderr.current_choice() { + ColorChoice::Always | ColorChoice::AlwaysAnsi => true, + // note: the `auto` branch is unreachable, as [`AutoStream::current_choice`] + // returns the *currently active* choice, not the initially-configured choice + // so an initial choice of `Auto` would be converted into either `Always` or `Never`. + ColorChoice::Never | ColorChoice::Auto => false, + } + } + + /// Gets the color choice (i.e., whether to output colors) for the configured stdout + /// + /// See also [`GlobalConfig::set_out_color_choice`] + #[must_use] + #[inline] + pub fn out_color_choice(&self) -> bool { + match &self.stdout.current_choice() { + ColorChoice::Always | ColorChoice::AlwaysAnsi => true, + // note: the `auto` branch is unreachable, as [`AutoStream::current_choice`] + // returns the *currently active* choice, not the initially-configured choice + // so an initial choice of `Auto` would be converted into either `Always` or `Never`. + ColorChoice::Never | ColorChoice::Auto => false, + } } } #[cfg(test)] mod tests { + use std::{ + io::{Cursor, Read, Seek}, + rc::Rc, + sync::Mutex, + }; + use super::*; + /// helper struct to implement `Write + 'static` while keeping + /// view access to an underlying buffer + /// + /// Uses [`Mutex::try_lock`] so no calls should block even though it is a mutex + #[derive(Debug, Clone, Default)] + struct SharedBuffer(Rc>>>); + + impl SharedBuffer { + fn new() -> Self { + Self::default() + } + } + + impl Write for SharedBuffer { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.try_lock().expect("mutex locked").write(buf) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.0.try_lock().expect("mutex locked").flush() + } + } + + /// asserts that there must be color/no color, based on the truth of `color`, + /// in the given stream, given a copy of the buffer it links to + fn expect_color(mut stream: impl Write, buf: SharedBuffer, color: bool) { + let expected: &[u8] = if color { + b"\x1b[1mcolor!\x1b[0m" + } else { + b"color!" + }; + + write!(stream, "{}color!{}", Style::new().bold(), Reset).expect("error writing"); + let mut grd = buf.0.try_lock().expect("mutex locked"); + + grd.rewind().expect("error rewinding"); + let mut data = Vec::new(); + grd.read_to_end(&mut data).expect("error reading"); + + assert_eq!( + data, expected, + "expected color: {}; found color: {}", + color, !color + ); + } + + fn assert_color_choice( + make_choice: impl Fn(&mut GlobalConfig), + stdout_color: Option, + stderr_color: Option, + ) { + let mut config = GlobalConfig::new(); + + let out = SharedBuffer::new(); + let err = SharedBuffer::new(); + config.set_stdout(Box::new(out.clone())); + config.set_stderr(Box::new(err.clone())); + + make_choice(&mut config); + + if let Some(stdout_color) = stdout_color { + expect_color(config.stdout(), out, stdout_color); + } + + if let Some(stderr_color) = stderr_color { + expect_color(config.stderr(), err, stderr_color); + } + } + #[test] fn test_log_level_info() { let mut config = GlobalConfig::new(); @@ -237,4 +395,75 @@ mod tests { assert!(!config.is_verbose()); assert!(!config.is_extra_verbose()); } + + #[test] + fn test_set_color_choice() { + assert_color_choice( + |config| config.set_color_choice(false), + Some(false), + Some(false), + ); + assert_color_choice( + |config| config.set_color_choice(true), + Some(true), + Some(true), + ); + } + + #[test] + fn test_set_out_color_choice() { + assert_color_choice( + |config| config.set_out_color_choice(false), + Some(false), + None, + ); + assert_color_choice(|config| config.set_out_color_choice(true), Some(true), None); + } + + #[test] + fn test_set_err_color_choice() { + assert_color_choice( + |config| config.set_err_color_choice(false), + None, + Some(false), + ); + assert_color_choice(|config| config.set_err_color_choice(true), None, Some(true)); + } + + #[test] + fn test_set_global_color_choice() { + ColorChoice::Always.write_global(); + assert_color_choice(|_| (), Some(true), Some(true)); + + ColorChoice::AlwaysAnsi.write_global(); + assert_color_choice(|_| (), Some(true), Some(true)); + + ColorChoice::Never.write_global(); + assert_color_choice(|_| (), Some(false), Some(false)); + + // We don't test `ColorChoice::Auto` because it depends on the tty status of the output, + // which could lead to a flaky test depending on where and how the test is executed. + } + + #[test] + fn test_get_color_choice() { + let mut config = GlobalConfig::new(); + config.set_color_choice(true); + assert!(config.err_color_choice()); + assert!(config.out_color_choice()); + + config.set_out_color_choice(false); + assert!(!config.out_color_choice()); + + config.set_color_choice(false); + config.set_err_color_choice(true); + assert!(config.err_color_choice()); + + ColorChoice::AlwaysAnsi.write_global(); + // we have to instantiate a new GlobalConfig here for it to read + // the color choice + config = GlobalConfig::new(); + assert!(config.err_color_choice()); + assert!(config.out_color_choice()); + } } diff --git a/src/lib.rs b/src/lib.rs index a8a4fd84..9020ce09 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,8 +35,6 @@ pub struct Check { scope: Scope, current: Rustdoc, baseline: Rustdoc, - log_level: Option, - color_choice: Option, release_type: Option, current_feature_config: rustdoc_gen::FeatureConfig, baseline_feature_config: rustdoc_gen::FeatureConfig, @@ -251,8 +249,6 @@ impl Check { scope: Scope::default(), current, baseline: Rustdoc::from_registry_latest_crate_version(), - log_level: Default::default(), - color_choice: None, release_type: None, current_feature_config: rustdoc_gen::FeatureConfig::default_for_current(), baseline_feature_config: rustdoc_gen::FeatureConfig::default_for_baseline(), @@ -275,35 +271,6 @@ impl Check { self } - /// Set the log level. - /// If not set or set to `None`, logging is disabled. - pub fn with_log_level(&mut self, log_level: Option) -> &mut Self { - self.log_level = log_level; - self - } - - /// Get the current log level. - /// If set to `None`, logging is disabled. - #[inline] - pub fn log_level(&self) -> Option<&log::Level> { - self.log_level.as_ref() - } - - /// Set the termcolor [color choice](termcolor::ColorChoice). - /// If `None`, the use of colors will be determined automatically by - /// the `CARGO_TERM_COLOR` env var and tty type of output. - pub fn with_color_choice(&mut self, choice: Option) -> &mut Self { - self.color_choice = choice; - self - } - - /// Get the current color choice. If `None`, the use of colors is determined - /// by the `CARGO_TERM_COLOR` env var and whether the output is a tty - #[inline] - pub fn color_choice(&self) -> Option<&termcolor::ColorChoice> { - self.color_choice.as_ref() - } - pub fn with_release_type(&mut self, release_type: ReleaseType) -> &mut Self { self.release_type = Some(release_type); self @@ -405,14 +372,7 @@ impl Check { }) } - pub fn check_release(&self) -> anyhow::Result { - let mut config = GlobalConfig::new().set_level(self.log_level); - // we don't want to set auto if the choice is not set because it would overwrite - // the value if the CARGO_TERM_COLOR env var read in GlobalConfig::new() if it is set - if let Some(choice) = self.color_choice { - config = config.set_color_choice(choice); - } - + pub fn check_release(&self, config: &mut GlobalConfig) -> anyhow::Result { let rustdoc_cmd = RustdocCommand::new().deps(false).silence(config.is_info()); // If both the current and baseline rustdoc are given explicitly as a file path, @@ -437,8 +397,8 @@ impl Check { }; } - let current_loader = self.get_rustdoc_generator(&mut config, &self.current.source)?; - let baseline_loader = self.get_rustdoc_generator(&mut config, &self.baseline.source)?; + let current_loader = self.get_rustdoc_generator(config, &self.current.source)?; + let baseline_loader = self.get_rustdoc_generator(config, &self.baseline.source)?; // Create a report for each crate. // We want to run all the checks, even if one returns `Err`. @@ -467,7 +427,7 @@ impl Check { let start = std::time::Instant::now(); let version = None; let (current_crate, baseline_crate) = generate_versioned_crates( - &mut config, + config, &rustdoc_cmd, &*current_loader, &*baseline_loader, @@ -488,7 +448,7 @@ impl Check { )?; let report = run_check_release( - &mut config, + config, &name, current_crate, baseline_crate, @@ -545,7 +505,7 @@ note: skipped the following crates since they have no library target: {skipped}" } else { let start = std::time::Instant::now(); let (current_crate, baseline_crate) = generate_versioned_crates( - &mut config, + config, &rustdoc_cmd, &*current_loader, &*baseline_loader, @@ -568,7 +528,7 @@ note: skipped the following crates since they have no library target: {skipped}" let result = Ok(( crate_name.clone(), Some(run_check_release( - &mut config, + config, crate_name, current_crate, baseline_crate, diff --git a/src/main.rs b/src/main.rs index 21b0e695..331d196b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,6 @@ #![forbid(unsafe_code)] -use std::path::PathBuf; +use std::{env, path::PathBuf}; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, @@ -11,6 +11,9 @@ fn main() { human_panic::setup_panic!(); let Cargo::SemverChecks(args) = Cargo::parse(); + + configure_color(args.color_choice); + if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; bugreport!() @@ -26,12 +29,6 @@ fn main() { let mut config = GlobalConfig::new().set_level(args.check_release.verbosity.log_level()); - // we don't want to set auto if the choice is not set because it would overwrite - // the value if the CARGO_TERM_COLOR env var read in GlobalConfig::new() if it is set - if let Some(choice) = args.check_release.color_choice { - config = config.set_color_choice(choice); - } - let queries = SemverQuery::all_queries(); let mut rows = vec![["id", "type", "description"], ["==", "====", "==========="]]; for query in queries.values() { @@ -88,11 +85,18 @@ fn main() { std::process::exit(0); } - let check: cargo_semver_checks::Check = match args.command { - Some(SemverChecksCommands::CheckRelease(args)) => args.into(), - None => args.check_release.into(), + let check_release = match args.command { + Some(SemverChecksCommands::CheckRelease(c)) => c, + None => args.check_release, }; - let report = exit_on_error(check.log_level().is_some(), || check.check_release()); + + let mut config = GlobalConfig::new(); + + config = config.set_level(check_release.verbosity.log_level()); + + let check: cargo_semver_checks::Check = check_release.into(); + + let report = exit_on_error(config.is_error(), || check.check_release(&mut config)); if report.success() { std::process::exit(0); } else { @@ -100,7 +104,7 @@ fn main() { } } -fn exit_on_error(log_errors: bool, inner: impl Fn() -> anyhow::Result) -> T { +fn exit_on_error(log_errors: bool, mut inner: impl FnMut() -> anyhow::Result) -> T { match inner() { Ok(x) => x, Err(err) => { @@ -112,6 +116,33 @@ fn exit_on_error(log_errors: bool, inner: impl Fn() -> anyhow::Result) -> } } +/// helper function to determine whether to use colors based on the (passed) `--color` flag +/// and the value of the `CARGO_TERM_COLOR` variable. +/// +/// If the `--color` flag is set to something valid, it overrides anything in +/// the `CARGO_TERM_COLOR` environment variable +fn configure_color(cli_choice: Option) { + use anstream::ColorChoice as AnstreamChoice; + use clap::ColorChoice as ClapChoice; + let choice = match cli_choice { + Some(ClapChoice::Always) => AnstreamChoice::Always, + Some(ClapChoice::Auto) => AnstreamChoice::Auto, + Some(ClapChoice::Never) => AnstreamChoice::Never, + // we match the behavior of cargo in + // https://doc.rust-lang.org/cargo/reference/config.html#termcolor + // note that [`ColorChoice::AlwaysAnsi`] is not supported by cargo. + None => match env::var("CARGO_TERM_COLOR").as_deref() { + Ok("always") => AnstreamChoice::Always, + Ok("never") => AnstreamChoice::Never, + // if `auto` is set, or the env var is invalid + // or both the env var and flag are not set, we set the choice to auto + _ => AnstreamChoice::Auto, + }, + }; + + choice.write_global(); +} + #[derive(Debug, Parser)] #[command(name = "cargo")] #[command(bin_name = "cargo")] @@ -137,6 +168,12 @@ struct SemverChecks { #[command(subcommand)] command: Option, + + // we need to use clap::ColorChoice instead of anstream::ColorChoice + // because ValueEnum is implemented for it. + /// Choose whether to output colors + #[arg(long = "color", global = true, value_name = "WHEN", value_enum)] + color_choice: Option, } /// Check your crate for semver violations. @@ -287,16 +324,9 @@ struct CheckRelease { #[arg(long = "target")] build_target: Option, + // docstring for help is on the `clap_verbosity_flag::Verbosity` struct itself #[command(flatten)] verbosity: clap_verbosity_flag::Verbosity, - - /// Whether to print colors to the terminal: - /// always, alwaysansi (always use only ANSI color codes), - /// auto (based on whether output is a tty), and never - /// - /// Default is auto (use colors if output is a TTY, otherwise don't use colors) - #[arg(value_enum, long = "color")] - color_choice: Option, } impl From for cargo_semver_checks::Check { @@ -358,9 +388,6 @@ impl From for cargo_semver_checks::Check { check.with_baseline(baseline); } - check.with_log_level(value.verbosity.log_level()); - check.with_color_choice(value.color_choice); - if let Some(release_type) = value.release_type { check.with_release_type(release_type); } diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index 702dcf97..39047ea9 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -129,15 +129,19 @@ impl RustdocCommand { if !self.deps { cmd.arg("--no-deps"); } - if config.is_stderr_tty() { - cmd.arg("--color=always"); - } + + // respect our configured color choice + cmd.arg(if config.err_color_choice() { + "--color=always" + } else { + "--color=never" + }); let output = cmd.output()?; if !output.status.success() { if self.silence { config.log_error(|config| { - let stderr = config.stderr(); + let mut stderr = config.stderr(); let delimiter = "-----"; writeln!( stderr, @@ -156,9 +160,8 @@ impl RustdocCommand { })?; } else { config.log_error(|config| { - let stderr = config.stderr(); writeln!( - stderr, + config.stderr(), "error: running cargo-doc on crate {crate_name} v{version} failed, see stderr output above" )?; Ok(()) @@ -167,7 +170,7 @@ impl RustdocCommand { config.log_error(|config| { let features = crate_source.feature_list_from_config(config, crate_data.feature_config); - let stderr = config.stderr(); + let mut stderr = config.stderr(); writeln!( stderr, "note: this is usually due to a compilation error in the crate," @@ -242,8 +245,8 @@ cargo new --lib example && None } else { config.log_error(|config| { - let stderr = config.stderr(); let delimiter = "-----"; + let mut stderr = config.stderr(); writeln!( stderr, "error: running cargo-config on crate {crate_name} failed with output:" @@ -258,8 +261,7 @@ cargo new --lib example && writeln!(stderr, "note: this may be a bug in cargo, or a bug in cargo-semver-checks;")?; writeln!(stderr, " if unsure, feel free to open a GitHub issue on cargo-semver-checks")?; writeln!(stderr, "note: running the following command on the crate should reproduce the error:")?; - writeln!( - stderr, + writeln!(stderr, " cargo config -Zunstable-options get --format=json-value build.target\n", )?; Ok(()) diff --git a/tests/lib_tests.rs b/tests/lib_tests.rs index cee04bdf..24071774 100644 --- a/tests/lib_tests.rs +++ b/tests/lib_tests.rs @@ -1,12 +1,13 @@ -use cargo_semver_checks::{ActualSemverUpdate, Check, ReleaseType, Rustdoc}; +use cargo_semver_checks::{ActualSemverUpdate, Check, GlobalConfig, ReleaseType, Rustdoc}; #[test] fn major_required_bump_if_breaking_change() { let current = Rustdoc::from_root("test_crates/trait_missing/old/"); let baseline = Rustdoc::from_root("test_crates/trait_missing/new/"); + let mut config = GlobalConfig::new(); let mut check = Check::new(current); let check = check.with_baseline(baseline); - let report = check.check_release().unwrap(); + let report = check.check_release(&mut config).unwrap(); assert!(!report.success()); let (_crate_name, crate_report) = report.crate_reports().iter().next().unwrap(); let required_bump = crate_report.required_bump().unwrap(); @@ -20,7 +21,7 @@ fn major_required_bump_if_breaking_change_and_major_bump_detected() { let baseline = Rustdoc::from_root("test_crates/trait_missing_with_major_bump/new/"); let mut check = Check::new(current); let check = check.with_baseline(baseline); - let report = check.check_release().unwrap(); + let report = check.check_release(&mut GlobalConfig::new()).unwrap(); // semver is successful because the new crate has a major bump version assert!(report.success()); let (_crate_name, crate_report) = report.crate_reports().iter().next().unwrap();