Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(cli): Control clap colors through config #13463

Merged
merged 5 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 27 additions & 58 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use anyhow::{anyhow, Context as _};
use cargo::core::shell::Shell;
use cargo::core::{features, CliUnstable};
use cargo::util::config::TermConfig;
use cargo::{drop_print, drop_println, CargoResult};
use clap::builder::UnknownArgumentValueParser;
use itertools::Itertools;
Expand All @@ -13,14 +13,17 @@ use super::commands;
use super::list_commands;
use crate::command_prelude::*;
use crate::util::is_rustup;
use cargo::core::shell::ColorChoice;
use cargo::util::style;

pub fn main(lazy_gctx: &mut LazyContext) -> CliResult {
let args = cli().try_get_matches()?;
pub fn main(gctx: &mut GlobalContext) -> CliResult {
// CAUTION: Be careful with using `config` until it is configured below.
// In general, try to avoid loading config values unless necessary (like
// the [alias] table).

let args = cli(gctx).try_get_matches()?;

// Update the process-level notion of cwd
// This must be completed before config is initialized
assert_eq!(lazy_gctx.is_init(), false);
if let Some(new_cwd) = args.get_one::<std::path::PathBuf>("directory") {
// This is a temporary hack. This cannot access `Config`, so this is a bit messy.
// This does not properly parse `-Z` flags that appear after the subcommand.
Expand All @@ -40,13 +43,9 @@ pub fn main(lazy_gctx: &mut LazyContext) -> CliResult {
.into());
}
std::env::set_current_dir(&new_cwd).context("could not change to requested directory")?;
gctx.reload_cwd()?;
}

// CAUTION: Be careful with using `config` until it is configured below.
// In general, try to avoid loading config values unless necessary (like
// the [alias] table).
let gctx = lazy_gctx.get_mut();

let (expanded_args, global_args) = expand_aliases(gctx, args, vec![])?;

if expanded_args
Expand Down Expand Up @@ -175,7 +174,7 @@ Run with `{literal}cargo -Z{literal:#} {placeholder}[FLAG] [COMMAND]{placeholder
Some((cmd, args)) => (cmd, args),
_ => {
// No subcommand provided.
cli().print_help()?;
cli(gctx).print_help()?;
return Ok(());
}
};
Expand Down Expand Up @@ -338,7 +337,7 @@ For more information, see issue #12207 <https://github.com/rust-lang/cargo/issue
// Note that an alias to an external command will not receive
// these arguments. That may be confusing, but such is life.
let global_args = GlobalArgs::new(sub_args);
let new_args = cli().no_binary_name(true).try_get_matches_from(alias)?;
let new_args = cli(gctx).no_binary_name(true).try_get_matches_from(alias)?;

let new_cmd = new_args.subcommand_name().expect("subcommand is required");
already_expanded.push(cmd.to_string());
Expand Down Expand Up @@ -514,7 +513,19 @@ impl GlobalArgs {
}
}

pub fn cli() -> Command {
pub fn cli(gctx: &GlobalContext) -> Command {
// Don't let config errors get in the way of parsing arguments
let term = gctx.get::<TermConfig>("term").unwrap_or_default();
Copy link
Member

Choose a reason for hiding this comment

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

The issue is that we sidestep without calling GlobalContext::merge_cli_args, which GlobalContext::configure does.

There might also be a discrepancy when we call reload_cwd(), but that is minimal as the code path eventually calls GlobalContext::configure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure what the concern is here.

Is it that we aren't respecting --color which you raised elsewhere and you are just calling out the relevant code?

Or is it that we aren't respecting the config from -C to control the colors from CLI parsing? If so, that is a similar problem to --color support (and more difficult / brittle to support)

Copy link
Member

Choose a reason for hiding this comment

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

I was just making sure we're aware of this and also #9012 is not completely resolved. Should we reword #9012 to track --config and --color or open a new one?

I am good with merge this PR as-is btw. Feel free to r=weihanglo.

let color = term
.color
.and_then(|c| c.parse().ok())
.unwrap_or(ColorChoice::CargoAuto);
let color = match color {
ColorChoice::Always => clap::ColorChoice::Always,
ColorChoice::Never => clap::ColorChoice::Never,
ColorChoice::CargoAuto => clap::ColorChoice::Auto,
};

let usage = if is_rustup() {
color_print::cstr!("<cyan,bold>cargo</> <cyan>[+toolchain] [OPTIONS] [COMMAND]</>\n <cyan,bold>cargo</> <cyan>[+toolchain] [OPTIONS]</> <cyan,bold>-Zscript</> <cyan><<MANIFEST_RS>> [ARGS]...</>")
} else {
Expand All @@ -539,6 +550,7 @@ pub fn cli() -> Command {
// We also want these to come before auto-generated `--help`
.next_display_order(800)
.allow_external_subcommands(true)
.color(color)
.styles(styles)
// Provide a custom help subcommand for calling into man pages
.disable_help_subcommand(true)
Expand Down Expand Up @@ -646,53 +658,10 @@ See '<cyan,bold>cargo help</> <cyan><<command>></>' for more information on a sp
.subcommands(commands::builtin())
}

/// Delay loading [`GlobalContext`] until access.
///
/// In the common path, the [`GlobalContext`] is dependent on CLI parsing and shouldn't be loaded until
/// after that is done but some other paths (like fix or earlier errors) might need access to it,
/// so this provides a way to share the instance and the implementation across these different
/// accesses.
pub struct LazyContext {
gctx: Option<GlobalContext>,
}

impl LazyContext {
pub fn new() -> Self {
Self { gctx: None }
}

/// Check whether the config is loaded
///
/// This is useful for asserts in case the environment needs to be setup before loading
pub fn is_init(&self) -> bool {
self.gctx.is_some()
}

/// Get the config, loading it if needed
///
/// On error, the process is terminated
pub fn get(&mut self) -> &GlobalContext {
self.get_mut()
}

/// Get the config, loading it if needed
///
/// On error, the process is terminated
pub fn get_mut(&mut self) -> &mut GlobalContext {
self.gctx
.get_or_insert_with(|| match GlobalContext::default() {
Ok(cfg) => cfg,
Err(e) => {
let mut shell = Shell::new();
cargo::exit_with_error(e.into(), &mut shell)
}
})
}
}

#[test]
fn verify_cli() {
cli().debug_assert();
let gctx = GlobalContext::default().unwrap();
cli(&gctx).debug_assert();
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/help.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
}
}
} else {
let mut cmd = crate::cli::cli();
let mut cmd = crate::cli::cli(gctx);
let _ = cmd.print_help();
}
Ok(())
Expand Down
15 changes: 11 additions & 4 deletions src/bin/cargo/main.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![allow(clippy::self_named_module_files)] // false positive in `commands/build.rs`

use cargo::core::shell::Shell;
use cargo::util::network::http::http_handle;
use cargo::util::network::http::needs_custom_http_transport;
use cargo::util::{self, closest_msg, command_prelude, CargoResult};
Expand All @@ -19,17 +20,23 @@ use crate::command_prelude::*;
fn main() {
setup_logger();

let mut lazy_gctx = cli::LazyContext::new();
let mut gctx = match GlobalContext::default() {
Ok(gctx) => gctx,
Err(e) => {
let mut shell = Shell::new();
cargo::exit_with_error(e.into(), &mut shell)
}
};

let result = if let Some(lock_addr) = cargo::ops::fix_get_proxy_lock_addr() {
cargo::ops::fix_exec_rustc(lazy_gctx.get(), &lock_addr).map_err(|e| CliError::from(e))
cargo::ops::fix_exec_rustc(&gctx, &lock_addr).map_err(|e| CliError::from(e))
} else {
let _token = cargo::util::job::setup();
cli::main(&mut lazy_gctx)
cli::main(&mut gctx)
};

match result {
Err(e) => cargo::exit_with_error(e, &mut lazy_gctx.get_mut().shell()),
Err(e) => cargo::exit_with_error(e, &mut *gctx.shell()),
Ok(()) => {}
}
}
Expand Down
161 changes: 86 additions & 75 deletions src/cargo/core/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,44 +9,6 @@ use crate::util::errors::CargoResult;
use crate::util::hostname;
use crate::util::style::*;

pub enum TtyWidth {
NoTty,
Known(usize),
Guess(usize),
}

impl TtyWidth {
/// Returns the width of the terminal to use for diagnostics (which is
/// relayed to rustc via `--diagnostic-width`).
pub fn diagnostic_terminal_width(&self) -> Option<usize> {
// ALLOWED: For testing cargo itself only.
#[allow(clippy::disallowed_methods)]
if let Ok(width) = std::env::var("__CARGO_TEST_TTY_WIDTH_DO_NOT_USE_THIS") {
return Some(width.parse().unwrap());
}
match *self {
TtyWidth::NoTty | TtyWidth::Guess(_) => None,
TtyWidth::Known(width) => Some(width),
}
}

/// Returns the width used by progress bars for the tty.
pub fn progress_max_width(&self) -> Option<usize> {
match *self {
TtyWidth::NoTty => None,
TtyWidth::Known(width) | TtyWidth::Guess(width) => Some(width),
}
}
}

/// The requested verbosity of output.
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum Verbosity {
Verbose,
Normal,
Quiet,
}

/// An abstraction around console output that remembers preferences for output
/// verbosity and color.
pub struct Shell {
Expand Down Expand Up @@ -77,31 +39,6 @@ impl fmt::Debug for Shell {
}
}

/// A `Write`able object, either with or without color support
enum ShellOut {
/// A plain write object without color support
Write(AutoStream<Box<dyn Write>>),
/// Color-enabled stdio, with information on whether color should be used
Stream {
stdout: AutoStream<std::io::Stdout>,
stderr: AutoStream<std::io::Stderr>,
stderr_tty: bool,
color_choice: ColorChoice,
hyperlinks: bool,
},
}

/// Whether messages should use color output
#[derive(Debug, PartialEq, Clone, Copy)]
pub enum ColorChoice {
/// Force color output
Always,
/// Force disable color output
Never,
/// Intelligently guess whether to use color output
CargoAuto,
}

impl Shell {
/// Creates a new shell (color choice and verbosity), defaulting to 'auto' color and verbose
/// output.
Expand Down Expand Up @@ -299,18 +236,10 @@ impl Shell {
..
} = self.output
{
let cfg = match color {
Some("always") => ColorChoice::Always,
Some("never") => ColorChoice::Never,

Some("auto") | None => ColorChoice::CargoAuto,

Some(arg) => anyhow::bail!(
"argument for --color must be auto, always, or \
never, but found `{}`",
arg
),
};
let cfg = color
.map(|c| c.parse())
.transpose()?
.unwrap_or(ColorChoice::CargoAuto);
*color_choice = cfg;
let stdout_choice = cfg.to_anstream_color_choice();
let stderr_choice = cfg.to_anstream_color_choice();
Expand Down Expand Up @@ -444,6 +373,20 @@ impl Default for Shell {
}
}

/// A `Write`able object, either with or without color support
enum ShellOut {
/// A plain write object without color support
Write(AutoStream<Box<dyn Write>>),
/// Color-enabled stdio, with information on whether color should be used
Stream {
stdout: AutoStream<std::io::Stdout>,
stderr: AutoStream<std::io::Stderr>,
stderr_tty: bool,
color_choice: ColorChoice,
hyperlinks: bool,
},
}

impl ShellOut {
/// Prints out a message with a status. The status comes first, and is bold plus the given
/// color. The status can be justified, in which case the max width that will right align is
Expand Down Expand Up @@ -488,6 +431,55 @@ impl ShellOut {
}
}

pub enum TtyWidth {
NoTty,
Known(usize),
Guess(usize),
}

impl TtyWidth {
/// Returns the width of the terminal to use for diagnostics (which is
/// relayed to rustc via `--diagnostic-width`).
pub fn diagnostic_terminal_width(&self) -> Option<usize> {
// ALLOWED: For testing cargo itself only.
#[allow(clippy::disallowed_methods)]
if let Ok(width) = std::env::var("__CARGO_TEST_TTY_WIDTH_DO_NOT_USE_THIS") {
return Some(width.parse().unwrap());
}
match *self {
TtyWidth::NoTty | TtyWidth::Guess(_) => None,
TtyWidth::Known(width) => Some(width),
}
}

/// Returns the width used by progress bars for the tty.
pub fn progress_max_width(&self) -> Option<usize> {
match *self {
TtyWidth::NoTty => None,
TtyWidth::Known(width) | TtyWidth::Guess(width) => Some(width),
}
}
}

/// The requested verbosity of output.
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum Verbosity {
Verbose,
Normal,
Quiet,
}

/// Whether messages should use color output
#[derive(Debug, PartialEq, Clone, Copy)]
pub enum ColorChoice {
/// Force color output
Always,
/// Force disable color output
Never,
/// Intelligently guess whether to use color output
CargoAuto,
}

impl ColorChoice {
/// Converts our color choice to anstream's version.
fn to_anstream_color_choice(self) -> anstream::ColorChoice {
Expand All @@ -499,6 +491,25 @@ impl ColorChoice {
}
}

impl std::str::FromStr for ColorChoice {
type Err = anyhow::Error;
fn from_str(color: &str) -> Result<Self, Self::Err> {
let cfg = match color {
"always" => ColorChoice::Always,
"never" => ColorChoice::Never,

"auto" => ColorChoice::CargoAuto,

arg => anyhow::bail!(
"argument for --color must be auto, always, or \
never, but found `{}`",
arg
),
};
Ok(cfg)
}
}

fn supports_color(choice: anstream::ColorChoice) -> bool {
match choice {
anstream::ColorChoice::Always
Expand Down
Loading