Skip to content

Commit

Permalink
Auto merge of #12305 - epage:quiet, r=weihanglo
Browse files Browse the repository at this point in the history
fix(script): Be quiet on programmatic output

### What does this PR try to resolve?

This is a part of #12207

The goal is that we shouldn't interfere with end-user output when
"cargo script"s are used programmatically.  The only way to detect this
is when piping.  CI will also look like this.

My thought is that if someone does want to do `#!/usr/bin/env -S cargo -v`, it
should have a consistent meaning between local development
(`cargo run --manifest-path`) and "script mode" (`cargo`), so I
effectively added a new verbosity level in these cases.  To get normal
output in all cases, add a `-v` like the tests do.  Do `-vv` if you want
the normal `-v` mode.  If you want it always quiet, do `--quiet`.

I want to see the default verbosity for interactive "script mode" a bit
quieter to the point that all normal output cargo makes is cleared before
running the built binary.  I am holding off on that now as that could
tie into bigger conversations / refactors
(see https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Re-thinking.20cargo's.20output).

### How should we test and review this PR?

Initial writing of tests and refactors are split split out.  The tests in the final commit will let you see what impact this had on behavior.
  • Loading branch information
bors committed Jun 23, 2023
2 parents e95e2a9 + 0e648c3 commit 03bc66b
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 73 deletions.
118 changes: 72 additions & 46 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anyhow::{anyhow, Context as _};
use cargo::core::shell::Shell;
use cargo::core::{features, CliUnstable};
use cargo::{self, drop_print, drop_println, CliResult, Config};
use cargo::{self, drop_print, drop_println, CargoResult, CliResult, Config};
use clap::{Arg, ArgMatches};
use itertools::Itertools;
use std::collections::HashMap;
Expand Down Expand Up @@ -175,10 +175,11 @@ Run with 'cargo -Z [FLAG] [COMMAND]'",
return Ok(());
}
};
config_configure(config, &expanded_args, subcommand_args, global_args)?;
let exec = Exec::infer(cmd)?;
config_configure(config, &expanded_args, subcommand_args, global_args, &exec)?;
super::init_git(config);

execute_subcommand(config, cmd, subcommand_args)
exec.exec(config, subcommand_args)
}

pub fn get_version_string(is_verbose: bool) -> String {
Expand Down Expand Up @@ -363,12 +364,26 @@ fn config_configure(
args: &ArgMatches,
subcommand_args: &ArgMatches,
global_args: GlobalArgs,
exec: &Exec,
) -> CliResult {
let arg_target_dir = &subcommand_args.value_of_path("target-dir", config);
let verbose = global_args.verbose + args.verbose();
let mut verbose = global_args.verbose + args.verbose();
// quiet is unusual because it is redefined in some subcommands in order
// to provide custom help text.
let quiet = args.flag("quiet") || subcommand_args.flag("quiet") || global_args.quiet;
let mut quiet = args.flag("quiet") || subcommand_args.flag("quiet") || global_args.quiet;
if matches!(exec, Exec::Manifest(_)) && !quiet {
// Verbosity is shifted quieter for `Exec::Manifest` as it is can be used as if you ran
// `cargo install` and we especially shouldn't pollute programmatic output.
//
// For now, interactive output has the same default output as `cargo run` but that is
// subject to change.
if let Some(lower) = verbose.checked_sub(1) {
verbose = lower;
} else if !config.shell().is_err_tty() {
// Don't pollute potentially-scripted output
quiet = true;
}
}
let global_color = global_args.color; // Extract so it can take reference.
let color = args
.get_one::<String>("color")
Expand Down Expand Up @@ -399,53 +414,64 @@ fn config_configure(
Ok(())
}

/// Precedence isn't the most obvious from this function because
/// - Some is determined by `expand_aliases`
/// - Some is enforced by `avoid_ambiguity_between_builtins_and_manifest_commands`
///
/// In actuality, it is:
/// 1. built-ins xor manifest-command
/// 2. aliases
/// 3. external subcommands
fn execute_subcommand(config: &mut Config, cmd: &str, subcommand_args: &ArgMatches) -> CliResult {
if let Some(exec) = commands::builtin_exec(cmd) {
return exec(config, subcommand_args);
enum Exec {
Builtin(commands::Exec),
Manifest(String),
External(String),
}

impl Exec {
/// Precedence isn't the most obvious from this function because
/// - Some is determined by `expand_aliases`
/// - Some is enforced by `avoid_ambiguity_between_builtins_and_manifest_commands`
///
/// In actuality, it is:
/// 1. built-ins xor manifest-command
/// 2. aliases
/// 3. external subcommands
fn infer(cmd: &str) -> CargoResult<Self> {
if let Some(exec) = commands::builtin_exec(cmd) {
Ok(Self::Builtin(exec))
} else if commands::run::is_manifest_command(cmd) {
Ok(Self::Manifest(cmd.to_owned()))
} else {
Ok(Self::External(cmd.to_owned()))
}
}

if commands::run::is_manifest_command(cmd) {
let ext_path = super::find_external_subcommand(config, cmd);
if !config.cli_unstable().script && ext_path.is_some() {
config.shell().warn(format_args!(
"\
fn exec(self, config: &mut Config, subcommand_args: &ArgMatches) -> CliResult {
match self {
Self::Builtin(exec) => exec(config, subcommand_args),
Self::Manifest(cmd) => {
let ext_path = super::find_external_subcommand(config, &cmd);
if !config.cli_unstable().script && ext_path.is_some() {
config.shell().warn(format_args!(
"\
external subcommand `{cmd}` has the appearance of a manfiest-command
This was previously accepted but will be phased out when `-Zscript` is stabilized.
For more information, see issue #12207 <https://github.com/rust-lang/cargo/issues/12207>.",
))?;
let mut ext_args = vec![OsStr::new(cmd)];
ext_args.extend(
subcommand_args
.get_many::<OsString>("")
.unwrap_or_default()
.map(OsString::as_os_str),
);
super::execute_external_subcommand(config, cmd, &ext_args)
} else {
let ext_args: Vec<OsString> = subcommand_args
.get_many::<OsString>("")
.unwrap_or_default()
.cloned()
.collect();
commands::run::exec_manifest_command(config, cmd, &ext_args)
))?;
Self::External(cmd).exec(config, subcommand_args)
} else {
let ext_args: Vec<OsString> = subcommand_args
.get_many::<OsString>("")
.unwrap_or_default()
.cloned()
.collect();
commands::run::exec_manifest_command(config, &cmd, &ext_args)
}
}
Self::External(cmd) => {
let mut ext_args = vec![OsStr::new(&cmd)];
ext_args.extend(
subcommand_args
.get_many::<OsString>("")
.unwrap_or_default()
.map(OsString::as_os_str),
);
super::execute_external_subcommand(config, &cmd, &ext_args)
}
}
} else {
let mut ext_args = vec![OsStr::new(cmd)];
ext_args.extend(
subcommand_args
.get_many::<OsString>("")
.unwrap_or_default()
.map(OsString::as_os_str),
);
super::execute_external_subcommand(config, cmd, &ext_args)
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/bin/cargo/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ pub fn builtin() -> Vec<Command> {
]
}

pub fn builtin_exec(cmd: &str) -> Option<fn(&mut Config, &ArgMatches) -> CliResult> {
pub type Exec = fn(&mut Config, &ArgMatches) -> CliResult;

pub fn builtin_exec(cmd: &str) -> Option<Exec> {
let f = match cmd {
"add" => add::exec,
"bench" => bench::exec,
Expand Down
1 change: 1 addition & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -1484,6 +1484,7 @@ A parameter is identified as a manifest-command if it has one of:

Differences between `cargo run --manifest-path <path>` and `cargo <path>`
- `cargo <path>` runs with the config for `<path>` and not the current dir, more like `cargo install --path <path>`
- `cargo <path>` is at a verbosity level below the normal default. Pass `-v` to get normal output.

### `[lints]`

Expand Down
Loading

0 comments on commit 03bc66b

Please sign in to comment.