From bdc8b943750979008f6cf7353642c39095e8e30c Mon Sep 17 00:00:00 2001 From: m Date: Wed, 3 Apr 2024 11:47:36 -0700 Subject: [PATCH 01/14] ready for some initial feedback --- Cargo.lock | 13 ++++ Cargo.toml | 3 + src/check_release.rs | 171 ++++++++++++++++++------------------------- src/config.rs | 77 +++---------------- src/lib.rs | 20 ----- src/main.rs | 47 +++++++++--- src/rustdoc_cmd.rs | 89 ++++++++++------------ 7 files changed, 173 insertions(+), 247 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cde2d047..58df8b81 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -64,6 +64,16 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "anstyle-termcolor" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "11c3d1411f1f4c8a7b177caec3c71b51290f9e8ad9f99124fd3fe9aa96e56834" +dependencies = [ + "anstyle", + "termcolor", +] + [[package]] name = "anstyle-wincon" version = "3.0.2" @@ -285,6 +295,9 @@ dependencies = [ name = "cargo-semver-checks" version = "0.30.0" dependencies = [ + "anstream", + "anstyle", + "anstyle-termcolor", "anyhow", "assert_cmd", "atty", diff --git a/Cargo.toml b/Cargo.toml index 975a7047..26007f13 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,9 @@ 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" +anstyle-termcolor = "1.1.0" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/check_release.rs b/src/check_release.rs index d3652e9b..e09c42f8 100644 --- a/src/check_release.rs +++ b/src/check_release.rs @@ -1,11 +1,12 @@ use std::{collections::BTreeMap, sync::Arc, time::Instant}; +use anstream::{eprintln, println}; +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,29 @@ 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, - ) - })?; + eprintln!( + "{}{:>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, - ) - })?; + eprintln!( + "{}{:>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 +208,7 @@ pub(super) fn run_check_release( results_with_errors.len(), skipped_queries, ), - Color::Red, + Color::Ansi(AnsiColor::Red), true, ) .expect("print failed"); @@ -221,68 +218,54 @@ pub(super) fn run_check_release( for (semver_query, results) in results_with_errors { 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, - ) - })?; + .log_info(|_| { + println!( + "\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, - ) + config.log_info(|_| { + println!("{}Description:{}\n{}\n{:>12} {}\n{:>12} {}\n", + Style::new().bold(), 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, ) - })?; + ); 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, - ) + config.log_info(|_| { + println!( + "{}Description:{}\n{}\n{:>12} {}\n", + Style::new().bold(), + Reset, + &semver_query.error_message, + "impl:", + format!( + "https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", + crate_version!(), + semver_query.id, ) - })?; + ); Ok(()) }) .expect("print failed"); } config - .log_info(|config| { - colored_ln(config.stdout(), |w| { - colored!(w, "{}Failed in:{}", bold!(true), reset!()) - })?; + .log_info(|_| { + println!("{}Failed in:{}", Style::new().bold(), Reset); Ok(()) }) .expect("print failed"); @@ -300,38 +283,28 @@ pub(super) fn run_check_release( .context("Error instantiating semver query template.") .expect("could not materialize template"); config - .log_info(|config| { - colored_ln(config.stdout(), |w| colored!(w, " {}", message,))?; + .log_info(|_| { + println!(" {}", 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()) + .log_extra_verbose(|_| { + 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"); + println!("\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"); + .log_info(|_| { + println!("{}\n", serde_json::to_string_pretty(&pretty_result)?); Ok(()) }) .expect("print failed"); @@ -362,7 +335,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 +355,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..d8046658 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,13 +1,11 @@ -use termcolor::{ColorChoice, StandardStream}; +use anstream::{eprint, eprintln}; +use anstyle::{AnsiColor, Color, Reset, Style}; 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. /// @@ -23,34 +21,8 @@ impl Default for GlobalConfig { impl GlobalConfig { 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, - }; - 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), } @@ -125,52 +97,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), - )?; + eprint!("{}", Style::new().fg_color(Some(color)).bold()); if justified { - write!(self.stderr(), "{status:>12}")?; + eprint!("{status:>12}"); } else { - write!(self.stderr(), "{status}")?; - self.stderr() - .set_color(termcolor::ColorSpec::new().set_bold(true))?; - write!(self.stderr(), ":")?; + eprint!("{status}{}{}:", Reset, Style::new().bold()); } - self.stderr().reset()?; - writeln!(self.stderr(), " {message}")?; + eprintln!("{} {message}", Reset); } Ok(()) @@ -182,15 +125,15 @@ 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) } } diff --git a/src/lib.rs b/src/lib.rs index e83922e4..bd22e46e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -285,21 +285,6 @@ impl Check { 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 @@ -403,11 +388,6 @@ 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); - } let rustdoc_cmd = RustdocCommand::new().deps(false).silence(config.is_info()); diff --git a/src/main.rs b/src/main.rs index 21b0e695..2ab35afa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,12 +5,17 @@ use std::path::PathBuf; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, }; -use clap::{Args, Parser, Subcommand}; +use clap::{Args, Parser, Subcommand, ValueEnum}; +use serde::Serialize; fn main() { human_panic::setup_panic!(); let Cargo::SemverChecks(args) = Cargo::parse(); + // TODO: handle CARGO_TERM_COLOR env var again + if let Some(color_choice) = args.check_release.color_choice { + anstream::ColorChoice::write_global(color_choice.into()); + } if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; bugreport!() @@ -26,12 +31,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() { @@ -112,6 +111,37 @@ fn exit_on_error(log_errors: bool, inner: impl Fn() -> anyhow::Result) -> } } +/// helper enum to derive [`clap::ValueEnum`] on [`anstream::ColorChoice`] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, ValueEnum)] +pub(crate) enum ColorChoice { + Auto, + AlwaysAnsi, + Always, + Never, +} + +impl From for ColorChoice { + fn from(value: anstream::ColorChoice) -> Self { + match value { + anstream::ColorChoice::Always => Self::Always, + anstream::ColorChoice::AlwaysAnsi => Self::AlwaysAnsi, + anstream::ColorChoice::Auto => Self::Auto, + anstream::ColorChoice::Never => Self::Never, + } + } +} + +impl From for anstream::ColorChoice { + fn from(value: ColorChoice) -> Self { + match value { + ColorChoice::Always => Self::Always, + ColorChoice::AlwaysAnsi => Self::AlwaysAnsi, + ColorChoice::Auto => Self::Auto, + ColorChoice::Never => Self::Never, + } + } +} + #[derive(Debug, Parser)] #[command(name = "cargo")] #[command(bin_name = "cargo")] @@ -296,7 +326,7 @@ struct CheckRelease { /// /// Default is auto (use colors if output is a TTY, otherwise don't use colors) #[arg(value_enum, long = "color")] - color_choice: Option, + color_choice: Option, } impl From for cargo_semver_checks::Check { @@ -359,7 +389,6 @@ impl From for cargo_semver_checks::Check { } 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..b553cd2a 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -1,6 +1,6 @@ -use std::io::Write as _; use std::path::{Path, PathBuf}; +use anstream::ColorChoice; use anyhow::Context; use itertools::Itertools as _; @@ -129,57 +129,48 @@ impl RustdocCommand { if !self.deps { cmd.arg("--no-deps"); } - if config.is_stderr_tty() { - cmd.arg("--color=always"); - } + + // TODO + let color_choice = match anstream::stderr().current_choice() { + ColorChoice::Always | ColorChoice::AlwaysAnsi => "--color=always", + ColorChoice::Auto if anstream::stderr().is_terminal() => "--color=always", + ColorChoice::Auto | ColorChoice::Never => "--color=never", + }; + + cmd.arg(color_choice); + // if config.is_stderr_tty() { + // cmd.arg("--color=always"); + // } let output = cmd.output()?; if !output.status.success() { if self.silence { config.log_error(|config| { - let stderr = config.stderr(); let delimiter = "-----"; - writeln!( - stderr, - "error: running cargo-doc on crate {crate_name} failed with output:" - )?; - writeln!( - stderr, + eprintln!("error: running cargo-doc on crate {crate_name} failed with output:"); + eprintln!( "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) - )?; - writeln!( - stderr, - "error: failed to build rustdoc for crate {crate_name} v{version}" - )?; + ); + eprintln!("error: failed to build rustdoc for crate {crate_name} v{version}"); Ok(()) })?; } else { config.log_error(|config| { - let stderr = config.stderr(); - writeln!( - stderr, + eprintln!( "error: running cargo-doc on crate {crate_name} v{version} failed, see stderr output above" - )?; + ); Ok(()) })?; } config.log_error(|config| { let features = crate_source.feature_list_from_config(config, crate_data.feature_config); - let stderr = config.stderr(); - writeln!( - stderr, - "note: this is usually due to a compilation error in the crate," - )?; - writeln!( - stderr, - " and is unlikely to be a bug in cargo-semver-checks" - )?; - writeln!( - stderr, + eprintln!("note: this is usually due to a compilation error in the crate,"); + eprintln!(" and is unlikely to be a bug in cargo-semver-checks"); + eprintln!( "note: the following command can be used to reproduce the compilation error:" - )?; + ); let selector = match crate_source { CrateSource::Registry { version, .. } => format!("{crate_name}@={version}"), CrateSource::ManifestPath { manifest } => format!( @@ -197,15 +188,14 @@ impl RustdocCommand { } else { format!("--features {} ", features.into_iter().join(",")) }; - writeln!( - stderr, + eprintln!( " \ cargo new --lib example && cd example && echo '[workspace]' >> Cargo.toml && cargo add {selector} --no-default-features {feature_list}&& cargo check\n" - )?; + ); Ok(()) })?; anyhow::bail!( @@ -242,26 +232,22 @@ cargo new --lib example && None } else { config.log_error(|config| { - let stderr = config.stderr(); let delimiter = "-----"; - writeln!( - stderr, + eprintln!( "error: running cargo-config on crate {crate_name} failed with output:" - )?; - writeln!( - stderr, + ); + eprintln!( "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) - )?; + ); - writeln!(stderr, "error: unexpected cargo config output for crate {crate_name} v{version}\n")?; - 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, + eprintln!("error: unexpected cargo config output for crate {crate_name} v{version}\n"); + eprintln!("note: this may be a bug in cargo, or a bug in cargo-semver-checks;"); + eprintln!(" if unsure, feel free to open a GitHub issue on cargo-semver-checks"); + eprintln!("note: running the following command on the crate should reproduce the error:"); + eprintln!( " cargo config -Zunstable-options get --format=json-value build.target\n", - )?; + ); Ok(()) })?; anyhow::bail!( @@ -413,11 +399,10 @@ fn create_placeholder_rustdoc_manifest( if project_with_features.features.is_empty() { return Ok(()); } - writeln!( - config.stderr(), + eprintln!( " Features: {}", project_with_features.features.join(","), - )?; + ); Ok(()) })?; let mut deps = DepsSet::new(); From a48d569947202457151f3e429313183c1c6dc1e4 Mon Sep 17 00:00:00 2001 From: m Date: Thu, 4 Apr 2024 18:34:03 -0700 Subject: [PATCH 02/14] soliciting feedback here --- Cargo.lock | 38 --------------------------------- Cargo.toml | 3 --- src/check_release.rs | 2 +- src/lib.rs | 2 -- src/main.rs | 50 +++++++++++++++++++------------------------- src/rustdoc_cmd.rs | 8 +++---- 6 files changed, 26 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58df8b81..2ef034a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -64,16 +64,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "anstyle-termcolor" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11c3d1411f1f4c8a7b177caec3c71b51290f9e8ad9f99124fd3fe9aa96e56834" -dependencies = [ - "anstyle", - "termcolor", -] - [[package]] name = "anstyle-wincon" version = "3.0.2" @@ -297,7 +287,6 @@ version = "0.30.0" dependencies = [ "anstream", "anstyle", - "anstyle-termcolor", "anyhow", "assert_cmd", "atty", @@ -324,8 +313,6 @@ dependencies = [ "sha2", "similar-asserts", "tame-index", - "termcolor", - "termcolor_output", "toml", "trustfall", "trustfall_rustdoc", @@ -2668,31 +2655,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 26007f13..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" @@ -47,7 +45,6 @@ rustc_version = "0.4.0" rayon = "1.8.0" anstyle = "1.0.6" anstream = "0.6.13" -anstyle-termcolor = "1.1.0" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/check_release.rs b/src/check_release.rs index e09c42f8..c0c13723 100644 --- a/src/check_release.rs +++ b/src/check_release.rs @@ -158,7 +158,7 @@ pub(super) fn run_check_release( let mut results_with_errors = vec![]; for (semver_query, time_to_decide, results) in all_results { config - .log_verbose(|config| { + .log_verbose(|_| { let category = match semver_query.required_update { RequiredSemverUpdate::Major => "major", RequiredSemverUpdate::Minor => "minor", diff --git a/src/lib.rs b/src/lib.rs index bd22e46e..cb9fc6fb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,7 +35,6 @@ pub struct Check { 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, @@ -248,7 +247,6 @@ impl Check { 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(), diff --git a/src/main.rs b/src/main.rs index 2ab35afa..84d976df 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,17 +5,17 @@ use std::path::PathBuf; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, }; -use clap::{Args, Parser, Subcommand, ValueEnum}; -use serde::Serialize; +use clap::{builder::PossibleValue, Args, Parser, Subcommand, ValueEnum}; fn main() { human_panic::setup_panic!(); let Cargo::SemverChecks(args) = Cargo::parse(); - // TODO: handle CARGO_TERM_COLOR env var again + if let Some(color_choice) = args.check_release.color_choice { - anstream::ColorChoice::write_global(color_choice.into()); + color_choice.0.write_global(); } + if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; bugreport!() @@ -112,33 +112,25 @@ fn exit_on_error(log_errors: bool, inner: impl Fn() -> anyhow::Result) -> } /// helper enum to derive [`clap::ValueEnum`] on [`anstream::ColorChoice`] -#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, ValueEnum)] -pub(crate) enum ColorChoice { - Auto, - AlwaysAnsi, - Always, - Never, -} +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub(crate) struct ColorChoice(pub(crate) anstream::ColorChoice); -impl From for ColorChoice { - fn from(value: anstream::ColorChoice) -> Self { - match value { - anstream::ColorChoice::Always => Self::Always, - anstream::ColorChoice::AlwaysAnsi => Self::AlwaysAnsi, - anstream::ColorChoice::Auto => Self::Auto, - anstream::ColorChoice::Never => Self::Never, - } +impl ValueEnum for ColorChoice { + fn value_variants<'a>() -> &'a [Self] { + use anstream::ColorChoice::*; + &[Self(Always), Self(AlwaysAnsi), Self(Auto), Self(Never)] } -} -impl From for anstream::ColorChoice { - fn from(value: ColorChoice) -> Self { - match value { - ColorChoice::Always => Self::Always, - ColorChoice::AlwaysAnsi => Self::AlwaysAnsi, - ColorChoice::Auto => Self::Auto, - ColorChoice::Never => Self::Never, - } + fn to_possible_value(&self) -> Option { + use anstream::ColorChoice::*; + let name = match self.0 { + Always => "always", + AlwaysAnsi => "always-ansi", + Auto => "auto", + Never => "never", + }; + + Some(PossibleValue::new(name)) } } @@ -321,7 +313,7 @@ struct CheckRelease { verbosity: clap_verbosity_flag::Verbosity, /// Whether to print colors to the terminal: - /// always, alwaysansi (always use only ANSI color codes), + /// always, always-ansi (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) diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index b553cd2a..1af3a474 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -145,7 +145,7 @@ impl RustdocCommand { let output = cmd.output()?; if !output.status.success() { if self.silence { - config.log_error(|config| { + config.log_error(|_| { let delimiter = "-----"; eprintln!("error: running cargo-doc on crate {crate_name} failed with output:"); eprintln!( @@ -156,7 +156,7 @@ impl RustdocCommand { Ok(()) })?; } else { - config.log_error(|config| { + config.log_error(|_| { eprintln!( "error: running cargo-doc on crate {crate_name} v{version} failed, see stderr output above" ); @@ -231,7 +231,7 @@ cargo new --lib example && { None } else { - config.log_error(|config| { + config.log_error(|_| { let delimiter = "-----"; eprintln!( "error: running cargo-config on crate {crate_name} failed with output:" @@ -395,7 +395,7 @@ fn create_placeholder_rustdoc_manifest( ..DependencyDetail::default() }, }; - config.log_verbose(|config| { + config.log_verbose(|_| { if project_with_features.features.is_empty() { return Ok(()); } From b21b96f5008fe374083545401088f1383d3c9829 Mon Sep 17 00:00:00 2001 From: m Date: Mon, 8 Apr 2024 13:39:23 -0700 Subject: [PATCH 03/14] 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 --- Cargo.lock | 11 +++++++++++ Cargo.toml | 1 + src/main.rs | 33 ++++----------------------------- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c470d5e9..4fdf9698 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -296,6 +296,7 @@ dependencies = [ "clap", "clap-cargo", "clap-verbosity-flag", + "colorchoice-clap", "directories", "gix", "handlebars", @@ -436,6 +437,16 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" +[[package]] +name = "colorchoice-clap" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6fe9ad5d064caf028aeb50818a5c7857c28f746ad04111652b85d9bca8b0d0be" +dependencies = [ + "clap", + "colorchoice", +] + [[package]] name = "console" version = "0.15.8" diff --git a/Cargo.toml b/Cargo.toml index af01b256..ed2a0220 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ rustc_version = "0.4.0" rayon = "1.8.0" anstyle = "1.0.6" anstream = "0.6.13" +colorchoice-clap = "1.0.3" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/main.rs b/src/main.rs index 84d976df..f48089bd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,16 +5,14 @@ use std::path::PathBuf; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, }; -use clap::{builder::PossibleValue, Args, Parser, Subcommand, ValueEnum}; +use clap::{Args, Parser, Subcommand}; fn main() { human_panic::setup_panic!(); let Cargo::SemverChecks(args) = Cargo::parse(); - if let Some(color_choice) = args.check_release.color_choice { - color_choice.0.write_global(); - } + args.check_release.color_choice.write_global(); if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; @@ -111,29 +109,6 @@ fn exit_on_error(log_errors: bool, inner: impl Fn() -> anyhow::Result) -> } } -/// helper enum to derive [`clap::ValueEnum`] on [`anstream::ColorChoice`] -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub(crate) struct ColorChoice(pub(crate) anstream::ColorChoice); - -impl ValueEnum for ColorChoice { - fn value_variants<'a>() -> &'a [Self] { - use anstream::ColorChoice::*; - &[Self(Always), Self(AlwaysAnsi), Self(Auto), Self(Never)] - } - - fn to_possible_value(&self) -> Option { - use anstream::ColorChoice::*; - let name = match self.0 { - Always => "always", - AlwaysAnsi => "always-ansi", - Auto => "auto", - Never => "never", - }; - - Some(PossibleValue::new(name)) - } -} - #[derive(Debug, Parser)] #[command(name = "cargo")] #[command(bin_name = "cargo")] @@ -317,8 +292,8 @@ struct CheckRelease { /// 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, + #[command(flatten)] + color_choice: colorchoice_clap::Color, } impl From for cargo_semver_checks::Check { From 08bb37255e6baffd42c8877892d1251fe5953995 Mon Sep 17 00:00:00 2001 From: m Date: Thu, 11 Apr 2024 11:24:14 -0700 Subject: [PATCH 04/14] store stdout and stderr in config --- src/check_release.rs | 66 ++++++++++++++++---------- src/config.rs | 107 +++++++++++++++++++++++++++++++++++++++++-- src/main.rs | 15 +++--- src/rustdoc_cmd.rs | 76 +++++++++++++++++++----------- 4 files changed, 198 insertions(+), 66 deletions(-) diff --git a/src/check_release.rs b/src/check_release.rs index c0c13723..c6a322e7 100644 --- a/src/check_release.rs +++ b/src/check_release.rs @@ -1,6 +1,6 @@ +use std::io::Write as _; use std::{collections::BTreeMap, sync::Arc, time::Instant}; -use anstream::{eprintln, println}; use anstyle::{AnsiColor, Color, Reset, Style}; use anyhow::Context; @@ -158,13 +158,14 @@ pub(super) fn run_check_release( let mut results_with_errors = vec![]; for (semver_query, time_to_decide, results) in all_results { config - .log_verbose(|_| { + .log_verbose(|config| { let category = match semver_query.required_update { RequiredSemverUpdate::Major => "major", RequiredSemverUpdate::Minor => "minor", }; if results.is_empty() { - eprintln!( + writeln!( + config.stderr(), "{}{:>12}{} [{:8.3}s] {:^18} {}", Style::new() .fg_color(Some(Color::Ansi(AnsiColor::Green))) @@ -174,9 +175,10 @@ pub(super) fn run_check_release( time_to_decide.as_secs_f32(), category, semver_query.id - ); + )?; } else { - eprintln!( + writeln!( + config.stderr(), "{}{:>12}{} [{:>8.3}s] {:^18} {}", Style::new() .fg_color(Some(Color::Ansi(AnsiColor::Red))) @@ -186,7 +188,7 @@ pub(super) fn run_check_release( time_to_decide.as_secs_f32(), category, semver_query.id - ); + )?; } Ok(()) }) @@ -218,18 +220,20 @@ pub(super) fn run_check_release( for (semver_query, results) in results_with_errors { required_versions.push(semver_query.required_update); config - .log_info(|_| { - println!( + .log_info(|config| { + writeln!( + config.stdout(), "\n--- failure {}: {} ---\n", - &semver_query.id, &semver_query.human_readable_name - ); + &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(|_| { - println!("{}Description:{}\n{}\n{:>12} {}\n{:>12} {}\n", + config.log_info(|config| { + writeln!(config.stdout(), "{}Description:{}\n{}\n{:>12} {}\n{:>12} {}\n", Style::new().bold(), Reset, &semver_query.error_message, "ref:", @@ -240,13 +244,14 @@ pub(super) fn run_check_release( crate_version!(), semver_query.id, ) - ); + )?; Ok(()) }) .expect("print failed"); } else { - config.log_info(|_| { - println!( + config.log_info(|config| { + writeln!( + config.stdout(), "{}Description:{}\n{}\n{:>12} {}\n", Style::new().bold(), Reset, @@ -257,15 +262,20 @@ pub(super) fn run_check_release( crate_version!(), semver_query.id, ) - ); + )?; Ok(()) }) .expect("print failed"); } config - .log_info(|_| { - println!("{}Failed in:{}", Style::new().bold(), Reset); + .log_info(|config| { + writeln!( + config.stdout(), + "{}Failed in:{}", + Style::new().bold(), + Reset + )?; Ok(()) }) .expect("print failed"); @@ -283,28 +293,36 @@ pub(super) fn run_check_release( .context("Error instantiating semver query template.") .expect("could not materialize template"); config - .log_info(|_| { - println!(" {}", message); + .log_info(|config| { + writeln!(config.stdout(), " {}", message)?; Ok(()) }) .expect("print failed"); config - .log_extra_verbose(|_| { + .log_extra_verbose(|config| { 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"); - println!("\tlint rule output values:\n{}", indented_serde); + writeln!( + config.stdout(), + "\tlint rule output values:\n{}", + indented_serde + )?; Ok(()) }) .expect("print failed"); } else { config - .log_info(|_| { - println!("{}\n", serde_json::to_string_pretty(&pretty_result)?); + .log_info(|config| { + writeln!( + config.stdout(), + "{}\n", + serde_json::to_string_pretty(&pretty_result)? + )?; Ok(()) }) .expect("print failed"); diff --git a/src/config.rs b/src/config.rs index d8046658..0ac90562 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,8 +1,13 @@ -use anstream::{eprint, eprintln}; +use anstream::AutoStream; use anstyle::{AnsiColor, Color, Reset, Style}; +use std::io::Write; use crate::templating::make_handlebars_registry; +// re-export this so users don't have to add the `anstream` crate directly +// just to set color choice +pub use anstream::ColorChoice; + #[allow(dead_code)] pub struct GlobalConfig { level: Option, @@ -11,6 +16,8 @@ pub struct GlobalConfig { /// /// 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 { @@ -21,10 +28,15 @@ impl Default for GlobalConfig { impl GlobalConfig { pub fn new() -> Self { + let stdout_choice = anstream::stdout().current_choice(); + let stderr_choice = anstream::stdout().current_choice(); + Self { level: None, 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), } } @@ -106,14 +118,14 @@ impl GlobalConfig { justified: bool, ) -> anyhow::Result<()> { if self.is_info() { - eprint!("{}", Style::new().fg_color(Some(color)).bold()); + write!(self.stderr, "{}", Style::new().fg_color(Some(color)).bold())?; if justified { - eprint!("{status:>12}"); + write!(self.stderr, "{status:>12}")?; } else { - eprint!("{status}{}{}:", Reset, Style::new().bold()); + write!(self.stderr, "{status}{}{}:", Reset, Style::new().bold())?; } - eprintln!("{} {message}", Reset); + writeln!(self.stderr, "{Reset} {message}")?; } Ok(()) @@ -135,6 +147,91 @@ impl GlobalConfig { pub fn shell_warn(&mut self, message: impl std::fmt::Display) -> anyhow::Result<()> { 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 in [`ColorChoice::global`]. + /// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice + pub fn set_stderr(&mut self, err: Box) { + self.stderr = AutoStream::new(err, ColorChoice::global()); + } + + /// Sets the stderr output stream + /// + /// Defaults to the global color choice setting in [`ColorChoice::global`]. + /// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice + pub fn set_stdout(&mut self, out: Box) { + self.stdout = AutoStream::new(out, ColorChoice::global()); + } + + /// 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, choice: ColorChoice) { + // TODO - `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(), choice); + } + + /// 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, choice: ColorChoice) { + // TODO - `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(), choice); + } + + /// Sets the color choice for both [`GlobalConfig::stderr`] and [`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. + /// + /// Prefer to use [`ColorChoice::write_global`] to avoid creating new stream objects if you + /// don't need to configure `cargo-semver-checks` colors differently than other crates + /// that use `anstream` for outputting colors. + /// + /// See also [`GlobalConfig::set_err_color_choice`] and [`GlobalConfig::set_out_color_choice`] + pub fn set_color_choice(&mut self, choice: ColorChoice) { + self.set_err_color_choice(choice); + self.set_out_color_choice(choice); + } } #[cfg(test)] diff --git a/src/main.rs b/src/main.rs index f48089bd..44f7f9e2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,7 +12,7 @@ fn main() { let Cargo::SemverChecks(args) = Cargo::parse(); - args.check_release.color_choice.write_global(); + args.color_choice.write_global(); if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; @@ -134,6 +134,10 @@ struct SemverChecks { #[command(subcommand)] command: Option, + + // docstring for help is on the `colorchoice_clap::Color` struct itself + #[command(flatten)] + color_choice: colorchoice_clap::Color, } /// Check your crate for semver violations. @@ -284,16 +288,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, always-ansi (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) - #[command(flatten)] - color_choice: colorchoice_clap::Color, } impl From for cargo_semver_checks::Check { diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index 1af3a474..d32d9c2e 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -1,3 +1,4 @@ +use std::io::Write as _; use std::path::{Path, PathBuf}; use anstream::ColorChoice; @@ -145,32 +146,47 @@ impl RustdocCommand { let output = cmd.output()?; if !output.status.success() { if self.silence { - config.log_error(|_| { + config.log_error(|config| { let delimiter = "-----"; - eprintln!("error: running cargo-doc on crate {crate_name} failed with output:"); - eprintln!( + writeln!( + config.stderr(), + "error: running cargo-doc on crate {crate_name} failed with output:" + )?; + writeln!( + config.stderr(), "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) - ); - eprintln!("error: failed to build rustdoc for crate {crate_name} v{version}"); + )?; + writeln!( + config.stderr(), + "error: failed to build rustdoc for crate {crate_name} v{version}" + )?; Ok(()) })?; } else { - config.log_error(|_| { - eprintln!( + config.log_error(|config| { + writeln!( + config.stderr(), "error: running cargo-doc on crate {crate_name} v{version} failed, see stderr output above" - ); + )?; Ok(()) })?; } config.log_error(|config| { let features = crate_source.feature_list_from_config(config, crate_data.feature_config); - eprintln!("note: this is usually due to a compilation error in the crate,"); - eprintln!(" and is unlikely to be a bug in cargo-semver-checks"); - eprintln!( + writeln!( + config.stderr(), + "note: this is usually due to a compilation error in the crate," + )?; + writeln!( + config.stderr(), + " and is unlikely to be a bug in cargo-semver-checks" + )?; + writeln!( + config.stderr(), "note: the following command can be used to reproduce the compilation error:" - ); + )?; let selector = match crate_source { CrateSource::Registry { version, .. } => format!("{crate_name}@={version}"), CrateSource::ManifestPath { manifest } => format!( @@ -188,14 +204,15 @@ impl RustdocCommand { } else { format!("--features {} ", features.into_iter().join(",")) }; - eprintln!( + writeln!( + config.stderr(), " \ cargo new --lib example && cd example && echo '[workspace]' >> Cargo.toml && cargo add {selector} --no-default-features {feature_list}&& cargo check\n" - ); + )?; Ok(()) })?; anyhow::bail!( @@ -231,23 +248,25 @@ cargo new --lib example && { None } else { - config.log_error(|_| { + config.log_error(|config| { let delimiter = "-----"; - eprintln!( + writeln!( + config.stderr(), "error: running cargo-config on crate {crate_name} failed with output:" - ); - eprintln!( + )?; + writeln!( + config.stderr(), "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) - ); + )?; - eprintln!("error: unexpected cargo config output for crate {crate_name} v{version}\n"); - eprintln!("note: this may be a bug in cargo, or a bug in cargo-semver-checks;"); - eprintln!(" if unsure, feel free to open a GitHub issue on cargo-semver-checks"); - eprintln!("note: running the following command on the crate should reproduce the error:"); - eprintln!( + writeln!(config.stderr(), "error: unexpected cargo config output for crate {crate_name} v{version}\n")?; + writeln!(config.stderr(), "note: this may be a bug in cargo, or a bug in cargo-semver-checks;")?; + writeln!(config.stderr(), " if unsure, feel free to open a GitHub issue on cargo-semver-checks")?; + writeln!(config.stderr(), "note: running the following command on the crate should reproduce the error:")?; + writeln!(config.stderr(), " cargo config -Zunstable-options get --format=json-value build.target\n", - ); + )?; Ok(()) })?; anyhow::bail!( @@ -395,14 +414,15 @@ fn create_placeholder_rustdoc_manifest( ..DependencyDetail::default() }, }; - config.log_verbose(|_| { + config.log_verbose(|config| { if project_with_features.features.is_empty() { return Ok(()); } - eprintln!( + writeln!( + config.stderr(), " Features: {}", project_with_features.features.join(","), - ); + )?; Ok(()) })?; let mut deps = DepsSet::new(); From 59dc0b2596a414a482eb2fbe9fdf7f3f5be892d5 Mon Sep 17 00:00:00 2001 From: m Date: Fri, 12 Apr 2024 15:15:58 -0700 Subject: [PATCH 05/14] 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 --- src/config.rs | 119 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 0ac90562..fe664126 100644 --- a/src/config.rs +++ b/src/config.rs @@ -176,10 +176,10 @@ impl GlobalConfig { self.stderr = AutoStream::new(err, ColorChoice::global()); } - /// Sets the stderr output stream + /// Sets the stdout output stream /// /// Defaults to the global color choice setting in [`ColorChoice::global`]. - /// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice + /// Call [`GlobalConfig::set_out_color_choice`] to customize the color choice pub fn set_stdout(&mut self, out: Box) { self.stdout = AutoStream::new(out, ColorChoice::global()); } @@ -236,7 +236,60 @@ impl GlobalConfig { #[cfg(test)] mod tests { + use std::{ + io::{Cursor, Read, Seek}, + rc::Rc, + sync::Mutex, + }; + use super::*; + use ColorChoice::*; + + /// 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 { + Default::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) { + 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 + ); + } #[test] fn test_log_level_info() { @@ -277,4 +330,66 @@ mod tests { assert!(!config.is_verbose()); assert!(!config.is_extra_verbose()); } + + #[test] + fn test_set_color_choice() { + fn assert_set_color(choice: ColorChoice) { + 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())); + + config.set_color_choice(choice); + + expect_color::(config.stdout(), out); + expect_color::(config.stderr(), err); + } + + assert_set_color::(Always); + assert_set_color::(AlwaysAnsi); + // a SharedBuffer is not a tty, so it should default to no colors. + // TODO: is this test sound? + assert_set_color::(Auto); + assert_set_color::(Never); + } + + #[test] + fn test_set_out_color_choice() { + fn assert_set_out_color(choice: ColorChoice) { + let mut config = GlobalConfig::new(); + + let out = SharedBuffer::new(); + config.set_stdout(Box::new(out.clone())); + + config.set_color_choice(choice); + + expect_color::(config.stdout(), out); + } + + assert_set_out_color::(Always); + assert_set_out_color::(AlwaysAnsi); + assert_set_out_color::(Auto); + assert_set_out_color::(Never); + } + + #[test] + fn test_set_err_color_choice() { + fn assert_set_err_color(choice: ColorChoice) { + let mut config = GlobalConfig::new(); + + let err = SharedBuffer::new(); + config.set_stderr(Box::new(err.clone())); + + config.set_color_choice(choice); + + expect_color::(config.stderr(), err); + } + + assert_set_err_color::(Always); + assert_set_err_color::(AlwaysAnsi); + assert_set_err_color::(Auto); + assert_set_err_color::(Never); + } } From 1ad3dc349749460ef82c090a2b7f74df4048feca Mon Sep 17 00:00:00 2001 From: m Date: Fri, 12 Apr 2024 15:23:32 -0700 Subject: [PATCH 06/14] fix clippy --- src/check_release.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/check_release.rs b/src/check_release.rs index c6a322e7..1d6ddccf 100644 --- a/src/check_release.rs +++ b/src/check_release.rs @@ -233,17 +233,14 @@ pub(super) fn run_check_release( if let Some(ref_link) = semver_query.reference_link.as_deref() { config.log_info(|config| { - writeln!(config.stdout(), "{}Description:{}\n{}\n{:>12} {}\n{:>12} {}\n", + 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:", - format!( - "https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", - crate_version!(), - semver_query.id, - ) + crate_version!(), + semver_query.id, )?; Ok(()) }) @@ -252,16 +249,13 @@ pub(super) fn run_check_release( config.log_info(|config| { writeln!( config.stdout(), - "{}Description:{}\n{}\n{:>12} {}\n", + "{}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:", - format!( - "https://github.com/obi1kenobi/cargo-semver-checks/tree/v{}/src/lints/{}.ron", - crate_version!(), - semver_query.id, - ) + crate_version!(), + semver_query.id, )?; Ok(()) }) From 45fa1219aef9fe9a8cc9fcfef89d05db40f118de Mon Sep 17 00:00:00 2001 From: m Date: Sat, 13 Apr 2024 22:35:29 -0700 Subject: [PATCH 07/14] (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 --- src/config.rs | 174 ++++++++++++++++++++++++++++---------------------- src/main.rs | 24 ++++++- 2 files changed, 120 insertions(+), 78 deletions(-) diff --git a/src/config.rs b/src/config.rs index fe664126..f1be286e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,13 +1,9 @@ -use anstream::AutoStream; +use anstream::{AutoStream, ColorChoice}; use anstyle::{AnsiColor, Color, Reset, Style}; use std::io::Write; use crate::templating::make_handlebars_registry; -// re-export this so users don't have to add the `anstream` crate directly -// just to set color choice -pub use anstream::ColorChoice; - #[allow(dead_code)] pub struct GlobalConfig { level: Option, @@ -27,6 +23,11 @@ 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 stdout_choice = anstream::stdout().current_choice(); let stderr_choice = anstream::stdout().current_choice(); @@ -170,18 +171,20 @@ impl GlobalConfig { /// Sets the stderr output stream /// - /// Defaults to the global color choice setting in [`ColorChoice::global`]. - /// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice + /// 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::new(err, ColorChoice::global()); + self.stderr = AutoStream::auto(err); } /// Sets the stdout output stream /// - /// Defaults to the global color choice setting in [`ColorChoice::global`]. - /// Call [`GlobalConfig::set_out_color_choice`] to customize the color choice + /// 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::new(out, ColorChoice::global()); + self.stdout = AutoStream::auto(out); } /// Individually set the color choice setting for [`GlobalConfig::stderr`] @@ -190,7 +193,7 @@ impl GlobalConfig { /// 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, choice: ColorChoice) { + pub fn set_err_color_choice(&mut self, use_color: bool) { // TODO - `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 @@ -198,7 +201,14 @@ impl GlobalConfig { &mut self.stderr, AutoStream::never(Box::new(std::io::sink())), ); - self.stderr = AutoStream::new(stderr.into_inner(), choice); + self.stderr = AutoStream::new( + stderr.into_inner(), + if use_color { + ColorChoice::Always + } else { + ColorChoice::Never + }, + ); } /// Individually set the color choice setting for [`GlobalConfig::stdout`] @@ -207,7 +217,7 @@ impl GlobalConfig { /// 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, choice: ColorChoice) { + pub fn set_out_color_choice(&mut self, use_color: bool) { // TODO - `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 @@ -215,22 +225,25 @@ impl GlobalConfig { &mut self.stdout, AutoStream::never(Box::new(std::io::sink())), ); - self.stdout = AutoStream::new(stdout.into_inner(), choice); + 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`] /// - /// Defaults to the global color choice in [`ColorChoice::global`], which can be set - /// in [`ColorChoice::write_global`] if you are using the `anstream` crate. - /// - /// Prefer to use [`ColorChoice::write_global`] to avoid creating new stream objects if you - /// don't need to configure `cargo-semver-checks` colors differently than other crates - /// that use `anstream` for outputting colors. + /// 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, choice: ColorChoice) { - self.set_err_color_choice(choice); - self.set_out_color_choice(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); } } @@ -243,7 +256,6 @@ mod tests { }; use super::*; - use ColorChoice::*; /// helper struct to implement `Write + 'static` while keeping /// view access to an underlying buffer @@ -254,7 +266,7 @@ mod tests { impl SharedBuffer { fn new() -> Self { - Default::default() + Self::default() } } @@ -268,10 +280,10 @@ mod tests { } } - /// asserts that there must be color/no color, based on the truth of `COLOR`, + /// 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) { - let expected: &[u8] = if COLOR { + fn expect_color(mut stream: impl Write, buf: SharedBuffer, color: bool) { + let expected: &[u8] = if color { b"\x1b[1mcolor!\x1b[0m" } else { b"color!" @@ -287,10 +299,33 @@ mod tests { assert_eq!( data, expected, "expected color: {}; found color: {}", - COLOR, !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(); @@ -333,63 +368,50 @@ mod tests { #[test] fn test_set_color_choice() { - fn assert_set_color(choice: ColorChoice) { - 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())); - - config.set_color_choice(choice); - - expect_color::(config.stdout(), out); - expect_color::(config.stderr(), err); - } - - assert_set_color::(Always); - assert_set_color::(AlwaysAnsi); - // a SharedBuffer is not a tty, so it should default to no colors. - // TODO: is this test sound? - assert_set_color::(Auto); - assert_set_color::(Never); + 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() { - fn assert_set_out_color(choice: ColorChoice) { - let mut config = GlobalConfig::new(); - - let out = SharedBuffer::new(); - config.set_stdout(Box::new(out.clone())); - - config.set_color_choice(choice); - - expect_color::(config.stdout(), out); - } - - assert_set_out_color::(Always); - assert_set_out_color::(AlwaysAnsi); - assert_set_out_color::(Auto); - assert_set_out_color::(Never); + 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() { - fn assert_set_err_color(choice: ColorChoice) { - let mut config = GlobalConfig::new(); + 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)); + } - let err = SharedBuffer::new(); - config.set_stderr(Box::new(err.clone())); + #[test] + fn test_set_global_color_choice() { + ColorChoice::Always.write_global(); + assert_color_choice(|_| (), Some(true), Some(true)); - config.set_color_choice(choice); + ColorChoice::AlwaysAnsi.write_global(); + assert_color_choice(|_| (), Some(true), Some(true)); - expect_color::(config.stderr(), err); - } + ColorChoice::Never.write_global(); + assert_color_choice(|_| (), Some(false), Some(false)); - assert_set_err_color::(Always); - assert_set_err_color::(AlwaysAnsi); - assert_set_err_color::(Auto); - assert_set_err_color::(Never); + // we don't test `ColorChoice::Auto` because it's not the most sound, as it depends on the + // tty status of the output. } } diff --git a/src/main.rs b/src/main.rs index 44f7f9e2..344bafe2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,7 +1,8 @@ #![forbid(unsafe_code)] -use std::path::PathBuf; +use std::{env, path::PathBuf}; +use anstream::ColorChoice; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, }; @@ -12,7 +13,7 @@ fn main() { let Cargo::SemverChecks(args) = Cargo::parse(); - args.color_choice.write_global(); + configure_color(args.color_choice); if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; @@ -109,6 +110,25 @@ 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. +fn configure_color(cli_choice: colorchoice_clap::Color) { + cli_choice.write_global(); + // if it is still auto (i.e., user passed `--color=auto` or omitted the flag entirely), + // we check the env var. otherwise, the flag takes precedence over whatever the env var is + if ColorChoice::global() == ColorChoice::Auto { + // https://doc.rust-lang.org/cargo/reference/config.html#termcolor + // we don't need to support `always-ansi`; that is more of an implementation detail + // and it is not recognized by cargo proper either. + match env::var("CARGO_TERM_COLOR").as_deref() { + Ok("always") => ColorChoice::Always.write_global(), + Ok("never") => ColorChoice::Never.write_global(), + // we don't need to test for `Auto`, since it is already set + _ => (), + } + } +} + #[derive(Debug, Parser)] #[command(name = "cargo")] #[command(bin_name = "cargo")] From 735600e68215f6c35a4c9b861fb6c34eb09ef2fc Mon Sep 17 00:00:00 2001 From: Max Carr Date: Sun, 14 Apr 2024 17:35:23 +0000 Subject: [PATCH 08/14] Apply suggestions from code review Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> --- src/config.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index f1be286e..89cf3310 100644 --- a/src/config.rs +++ b/src/config.rs @@ -194,7 +194,7 @@ impl GlobalConfig { /// /// See also [`GlobalConfig::set_out_color_choice`] and [`GlobalConfig::set_color_choice`] pub fn set_err_color_choice(&mut self, use_color: bool) { - // TODO - `anstream` doesn't have a good mechanism to set color choice (on one stream) + // `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( @@ -218,7 +218,7 @@ impl GlobalConfig { /// /// See also [`GlobalConfig::set_err_color_choice`] and [`GlobalConfig::set_color_choice`] pub fn set_out_color_choice(&mut self, use_color: bool) { - // TODO - `anstream` doesn't have a good mechanism to set color choice (on one stream) + // `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( @@ -411,7 +411,7 @@ mod tests { ColorChoice::Never.write_global(); assert_color_choice(|_| (), Some(false), Some(false)); - // we don't test `ColorChoice::Auto` because it's not the most sound, as it depends on the - // tty status of the output. + // 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. } } From f043188ebf72d62ea3cfbaddaebe1f06f34ec3b8 Mon Sep 17 00:00:00 2001 From: m Date: Sun, 14 Apr 2024 10:35:33 -0700 Subject: [PATCH 09/14] make `configure_color` more clear --- src/main.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/main.rs b/src/main.rs index 344bafe2..7febac57 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,7 @@ fn main() { let Cargo::SemverChecks(args) = Cargo::parse(); - configure_color(args.color_choice); + configure_color(args.color_choice.as_choice()); if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; @@ -112,20 +112,24 @@ 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. -fn configure_color(cli_choice: colorchoice_clap::Color) { - cli_choice.write_global(); - // if it is still auto (i.e., user passed `--color=auto` or omitted the flag entirely), - // we check the env var. otherwise, the flag takes precedence over whatever the env var is - if ColorChoice::global() == ColorChoice::Auto { +fn configure_color(cli_choice: ColorChoice) { + // if the --color flag is explicitly set to something other than `auto`, + // this takes precedence over the environment variable, so we write that + if cli_choice != ColorChoice::Auto { + cli_choice.write_global(); + } else { + // we match the behavior of cargo in // https://doc.rust-lang.org/cargo/reference/config.html#termcolor - // we don't need to support `always-ansi`; that is more of an implementation detail - // and it is not recognized by cargo proper either. + // note that [`ColorChoice::AlwaysAnsi`] is not supported by cargo. match env::var("CARGO_TERM_COLOR").as_deref() { Ok("always") => ColorChoice::Always.write_global(), Ok("never") => ColorChoice::Never.write_global(), - // we don't need to test for `Auto`, since it is already set + // note: the default global color value is `Auto` + Ok("auto") => ColorChoice::Auto.write_global(), + // ignore invalid or not set environment variables, + // color choice will default to `Auto` _ => (), - } + }; } } From 00221481355f84091027270b53e7b16b4956803c Mon Sep 17 00:00:00 2001 From: m Date: Sun, 14 Apr 2024 10:56:33 -0700 Subject: [PATCH 10/14] clarify rustdoc child process color setting --- src/rustdoc_cmd.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index d32d9c2e..503fa43c 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -131,17 +131,12 @@ impl RustdocCommand { cmd.arg("--no-deps"); } - // TODO - let color_choice = match anstream::stderr().current_choice() { - ColorChoice::Always | ColorChoice::AlwaysAnsi => "--color=always", - ColorChoice::Auto if anstream::stderr().is_terminal() => "--color=always", - ColorChoice::Auto | ColorChoice::Never => "--color=never", - }; - - cmd.arg(color_choice); - // if config.is_stderr_tty() { - // cmd.arg("--color=always"); - // } + // since we write the stderr of the process to our stderr, + // our color preferences will be automatically applied when we write + // the process's stderr to our stderr, as long as we have color + // i.e., if we set no color output in `GlobalConfig`, when we write + // the child stderr to ours, its colors will be automatically stripped + cmd.arg("--color=always"); let output = cmd.output()?; if !output.status.success() { From a08265b0d355bb831ddf3cf2c2c603ec42e21b14 Mon Sep 17 00:00:00 2001 From: m Date: Sun, 14 Apr 2024 11:57:44 -0700 Subject: [PATCH 11/14] handle env vars with flags we had to make our own colorchoice clap handler because the mixin doesn't work with Option<_> --- Cargo.lock | 11 ----------- Cargo.toml | 1 - src/main.rs | 47 +++++++++++++++++++++++++--------------------- src/rustdoc_cmd.rs | 1 - 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4fdf9698..c470d5e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -296,7 +296,6 @@ dependencies = [ "clap", "clap-cargo", "clap-verbosity-flag", - "colorchoice-clap", "directories", "gix", "handlebars", @@ -437,16 +436,6 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" -[[package]] -name = "colorchoice-clap" -version = "1.0.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6fe9ad5d064caf028aeb50818a5c7857c28f746ad04111652b85d9bca8b0d0be" -dependencies = [ - "clap", - "colorchoice", -] - [[package]] name = "console" version = "0.15.8" diff --git a/Cargo.toml b/Cargo.toml index ed2a0220..af01b256 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,7 +45,6 @@ rustc_version = "0.4.0" rayon = "1.8.0" anstyle = "1.0.6" anstream = "0.6.13" -colorchoice-clap = "1.0.3" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/main.rs b/src/main.rs index 7febac57..a12baa18 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,6 @@ use std::{env, path::PathBuf}; -use anstream::ColorChoice; use cargo_semver_checks::{ GlobalConfig, PackageSelection, ReleaseType, Rustdoc, ScopeSelection, SemverQuery, }; @@ -13,7 +12,7 @@ fn main() { let Cargo::SemverChecks(args) = Cargo::parse(); - configure_color(args.color_choice.as_choice()); + configure_color(args.color_choice); if args.bugreport { use bugreport::{bugreport, collector::*, format::Markdown}; @@ -112,25 +111,29 @@ 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. -fn configure_color(cli_choice: ColorChoice) { - // if the --color flag is explicitly set to something other than `auto`, - // this takes precedence over the environment variable, so we write that - if cli_choice != ColorChoice::Auto { - cli_choice.write_global(); - } else { +/// +/// 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. - match env::var("CARGO_TERM_COLOR").as_deref() { - Ok("always") => ColorChoice::Always.write_global(), - Ok("never") => ColorChoice::Never.write_global(), - // note: the default global color value is `Auto` - Ok("auto") => ColorChoice::Auto.write_global(), - // ignore invalid or not set environment variables, - // color choice will default to `Auto` - _ => (), - }; - } + 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)] @@ -159,9 +162,11 @@ struct SemverChecks { #[command(subcommand)] command: Option, - // docstring for help is on the `colorchoice_clap::Color` struct itself - #[command(flatten)] - color_choice: colorchoice_clap::Color, + // 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. diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index 503fa43c..6dff503b 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -1,7 +1,6 @@ use std::io::Write as _; use std::path::{Path, PathBuf}; -use anstream::ColorChoice; use anyhow::Context; use itertools::Itertools as _; From e762fa52c2cc451fcdadfce479be47d0365df276 Mon Sep 17 00:00:00 2001 From: m Date: Sun, 14 Apr 2024 22:32:33 -0700 Subject: [PATCH 12/14] make `Check::check_release` take &mut GlobalConfig --- src/lib.rs | 32 +++++++------------------------- src/main.rs | 19 ++++++++++++------- tests/lib_tests.rs | 7 ++++--- 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b550a69f..9020ce09 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,7 +35,6 @@ pub struct Check { scope: Scope, current: Rustdoc, baseline: Rustdoc, - log_level: Option, release_type: Option, current_feature_config: rustdoc_gen::FeatureConfig, baseline_feature_config: rustdoc_gen::FeatureConfig, @@ -250,7 +249,6 @@ impl Check { scope: Scope::default(), current, baseline: Rustdoc::from_registry_latest_crate_version(), - log_level: Default::default(), release_type: None, current_feature_config: rustdoc_gen::FeatureConfig::default_for_current(), baseline_feature_config: rustdoc_gen::FeatureConfig::default_for_baseline(), @@ -273,20 +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() - } - pub fn with_release_type(&mut self, release_type: ReleaseType) -> &mut Self { self.release_type = Some(release_type); self @@ -388,9 +372,7 @@ impl Check { }) } - pub fn check_release(&self) -> anyhow::Result { - let mut config = GlobalConfig::new().set_level(self.log_level); - + 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, @@ -415,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`. @@ -445,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, @@ -466,7 +448,7 @@ impl Check { )?; let report = run_check_release( - &mut config, + config, &name, current_crate, baseline_crate, @@ -523,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, @@ -546,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 a12baa18..331d196b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -85,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 { @@ -97,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) => { @@ -381,8 +388,6 @@ impl From for cargo_semver_checks::Check { check.with_baseline(baseline); } - check.with_log_level(value.verbosity.log_level()); - if let Some(release_type) = value.release_type { check.with_release_type(release_type); } 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(); From 0cc2a906839010137fc58710b3844a6472447e22 Mon Sep 17 00:00:00 2001 From: m Date: Tue, 16 Apr 2024 10:27:48 -0700 Subject: [PATCH 13/14] respect our color preference in `rustdoc` --- src/config.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++ src/rustdoc_cmd.rs | 12 +++++------ 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/config.rs b/src/config.rs index 89cf3310..aa9de2fb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -245,6 +245,36 @@ impl GlobalConfig { 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)] @@ -414,4 +444,26 @@ mod tests { // 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/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index 6dff503b..d0347733 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -130,12 +130,12 @@ impl RustdocCommand { cmd.arg("--no-deps"); } - // since we write the stderr of the process to our stderr, - // our color preferences will be automatically applied when we write - // the process's stderr to our stderr, as long as we have color - // i.e., if we set no color output in `GlobalConfig`, when we write - // the child stderr to ours, its colors will be automatically stripped - 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() { From 43c335870af429f7d01e8b986c5ff3c60b051dc1 Mon Sep 17 00:00:00 2001 From: m Date: Thu, 18 Apr 2024 11:18:59 -0700 Subject: [PATCH 14/14] minor: save config.stderr() when we use it a lot this also cleans up lines that get unnecessarily changed in the diff --- src/rustdoc_cmd.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/rustdoc_cmd.rs b/src/rustdoc_cmd.rs index d0347733..39047ea9 100644 --- a/src/rustdoc_cmd.rs +++ b/src/rustdoc_cmd.rs @@ -141,18 +141,19 @@ impl RustdocCommand { if !output.status.success() { if self.silence { config.log_error(|config| { + let mut stderr = config.stderr(); let delimiter = "-----"; writeln!( - config.stderr(), + stderr, "error: running cargo-doc on crate {crate_name} failed with output:" )?; writeln!( - config.stderr(), + stderr, "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) )?; writeln!( - config.stderr(), + stderr, "error: failed to build rustdoc for crate {crate_name} v{version}" )?; Ok(()) @@ -169,16 +170,17 @@ impl RustdocCommand { config.log_error(|config| { let features = crate_source.feature_list_from_config(config, crate_data.feature_config); + let mut stderr = config.stderr(); writeln!( - config.stderr(), + stderr, "note: this is usually due to a compilation error in the crate," )?; writeln!( - config.stderr(), + stderr, " and is unlikely to be a bug in cargo-semver-checks" )?; writeln!( - config.stderr(), + stderr, "note: the following command can be used to reproduce the compilation error:" )?; let selector = match crate_source { @@ -199,7 +201,7 @@ impl RustdocCommand { format!("--features {} ", features.into_iter().join(",")) }; writeln!( - config.stderr(), + stderr, " \ cargo new --lib example && cd example && @@ -244,21 +246,22 @@ cargo new --lib example && } else { config.log_error(|config| { let delimiter = "-----"; + let mut stderr = config.stderr(); writeln!( - config.stderr(), + stderr, "error: running cargo-config on crate {crate_name} failed with output:" )?; writeln!( - config.stderr(), + stderr, "{delimiter}\n{}\n{delimiter}\n", String::from_utf8_lossy(&output.stderr) )?; - writeln!(config.stderr(), "error: unexpected cargo config output for crate {crate_name} v{version}\n")?; - writeln!(config.stderr(), "note: this may be a bug in cargo, or a bug in cargo-semver-checks;")?; - writeln!(config.stderr(), " if unsure, feel free to open a GitHub issue on cargo-semver-checks")?; - writeln!(config.stderr(), "note: running the following command on the crate should reproduce the error:")?; - writeln!(config.stderr(), + writeln!(stderr, "error: unexpected cargo config output for crate {crate_name} v{version}\n")?; + 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, " cargo config -Zunstable-options get --format=json-value build.target\n", )?; Ok(())