From 215e4dad6ff85ea6f1524ec0d425fc374068cc82 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Jun 2023 15:54:45 -0500 Subject: [PATCH 1/4] refactor(cli): Decouple exec lookup from doing it --- src/bin/cargo/cli.rs | 98 ++++++++++++++++++++--------------- src/bin/cargo/commands/mod.rs | 4 +- 2 files changed, 58 insertions(+), 44 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 7b7725fa271..8f0ae0e52bd 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -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; @@ -178,7 +178,8 @@ Run with 'cargo -Z [FLAG] [COMMAND]'", config_configure(config, &expanded_args, subcommand_args, global_args)?; super::init_git(config); - execute_subcommand(config, cmd, subcommand_args) + let exec = Exec::infer(cmd)?; + exec.exec(config, subcommand_args) } pub fn get_version_string(is_verbose: bool) -> String { @@ -399,53 +400,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 { + 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 .", - ))?; - let mut ext_args = vec![OsStr::new(cmd)]; - ext_args.extend( - subcommand_args - .get_many::("") - .unwrap_or_default() - .map(OsString::as_os_str), - ); - super::execute_external_subcommand(config, cmd, &ext_args) - } else { - let ext_args: Vec = subcommand_args - .get_many::("") - .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 = subcommand_args + .get_many::("") + .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::("") + .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::("") - .unwrap_or_default() - .map(OsString::as_os_str), - ); - super::execute_external_subcommand(config, cmd, &ext_args) } } diff --git a/src/bin/cargo/commands/mod.rs b/src/bin/cargo/commands/mod.rs index da3109260a4..b9da0e5fb35 100644 --- a/src/bin/cargo/commands/mod.rs +++ b/src/bin/cargo/commands/mod.rs @@ -43,7 +43,9 @@ pub fn builtin() -> Vec { ] } -pub fn builtin_exec(cmd: &str) -> Option CliResult> { +pub type Exec = fn(&mut Config, &ArgMatches) -> CliResult; + +pub fn builtin_exec(cmd: &str) -> Option { let f = match cmd { "add" => add::exec, "bench" => bench::exec, From ec3c8181929dd1862c69592fd1f901814a296b7a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Jun 2023 16:55:02 -0500 Subject: [PATCH 2/4] refactor(cli): Infer the command before configuring This will let the command have some sway in how we configure --- src/bin/cargo/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 8f0ae0e52bd..45de56cd212 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -175,10 +175,10 @@ Run with 'cargo -Z [FLAG] [COMMAND]'", return Ok(()); } }; + let exec = Exec::infer(cmd)?; config_configure(config, &expanded_args, subcommand_args, global_args)?; super::init_git(config); - let exec = Exec::infer(cmd)?; exec.exec(config, subcommand_args) } From 279077987be8dd455f4f81c5951058187a0c4f54 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Jun 2023 19:26:55 -0500 Subject: [PATCH 3/4] test(script): Verify verbosity --- tests/testsuite/script.rs | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/testsuite/script.rs b/tests/testsuite/script.rs index 51f4868a6ce..8a2ebe12dd0 100644 --- a/tests/testsuite/script.rs +++ b/tests/testsuite/script.rs @@ -373,6 +373,52 @@ args: ["-NotAnArg"] .run(); } +#[cargo_test] +fn default_verbosity() { + let script = ECHO_SCRIPT; + let p = cargo_test_support::project() + .file("script.rs", script) + .build(); + + p.cargo("-Zscript script.rs -NotAnArg") + .masquerade_as_nightly_cargo(&["script"]) + .with_stdout( + r#"bin: [..]/debug/script[EXE] +args: ["-NotAnArg"] +"#, + ) + .with_stderr( + "\ +[WARNING] `package.edition` is unspecifiead, defaulting to `2021` +[COMPILING] script v0.0.0 ([ROOT]/foo) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s +[RUNNING] `[..]/debug/script[EXE] -NotAnArg` +", + ) + .run(); +} + +#[cargo_test] +fn quiet() { + let script = ECHO_SCRIPT; + let p = cargo_test_support::project() + .file("script.rs", script) + .build(); + + p.cargo("-Zscript -q script.rs -NotAnArg") + .masquerade_as_nightly_cargo(&["script"]) + .with_stdout( + r#"bin: [..]/debug/script[EXE] +args: ["-NotAnArg"] +"#, + ) + .with_stderr( + "\ +", + ) + .run(); +} + #[cargo_test] fn test_line_numbering_preserved() { let script = r#"#!/usr/bin/env cargo From 0e648c3ee441bc35a9250921250ca3c35b41940b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Jun 2023 16:39:45 -0500 Subject: [PATCH 4/4] fix(script): Be quiet on programmatic output The goal is that we shouldn't interefere 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). --- src/bin/cargo/cli.rs | 20 +++++++++-- src/doc/src/reference/unstable.md | 1 + tests/testsuite/script.rs | 58 ++++++++++++++----------------- 3 files changed, 45 insertions(+), 34 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 45de56cd212..db52bc8f27a 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -176,7 +176,7 @@ Run with 'cargo -Z [FLAG] [COMMAND]'", } }; let exec = Exec::infer(cmd)?; - config_configure(config, &expanded_args, subcommand_args, global_args)?; + config_configure(config, &expanded_args, subcommand_args, global_args, &exec)?; super::init_git(config); exec.exec(config, subcommand_args) @@ -364,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::("color") diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 31afa2133bc..0e2478baf88 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -1483,6 +1483,7 @@ A parameter is identified as a manifest-command if it has one of: Differences between `cargo run --manifest-path ` and `cargo ` - `cargo ` runs with the config for `` and not the current dir, more like `cargo install --path ` +- `cargo ` is at a verbosity level below the normal default. Pass `-v` to get normal output. ### `[lints]` diff --git a/tests/testsuite/script.rs b/tests/testsuite/script.rs index 8a2ebe12dd0..fcf58de6981 100644 --- a/tests/testsuite/script.rs +++ b/tests/testsuite/script.rs @@ -26,7 +26,7 @@ fn basic_rs() { .file("echo.rs", ECHO_SCRIPT) .build(); - p.cargo("-Zscript echo.rs") + p.cargo("-Zscript -v echo.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/echo[EXE] @@ -50,7 +50,7 @@ fn basic_path() { .file("echo", ECHO_SCRIPT) .build(); - p.cargo("-Zscript ./echo") + p.cargo("-Zscript -v ./echo") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/echo[EXE] @@ -74,7 +74,7 @@ fn basic_cargo_toml() { .file("src/main.rs", ECHO_SCRIPT) .build(); - p.cargo("-Zscript Cargo.toml") + p.cargo("-Zscript -v Cargo.toml") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: target/debug/foo[EXE] @@ -97,7 +97,7 @@ fn path_required() { .file("echo", ECHO_SCRIPT) .build(); - p.cargo("-Zscript echo") + p.cargo("-Zscript -v echo") .masquerade_as_nightly_cargo(&["script"]) .with_status(101) .with_stdout("") @@ -126,7 +126,7 @@ fn manifest_precedence_over_plugins() { path.push(p.root().join("path-test")); let path = std::env::join_paths(path.iter()).unwrap(); - p.cargo("-Zscript echo.rs") + p.cargo("-Zscript -v echo.rs") .env("PATH", &path) .masquerade_as_nightly_cargo(&["script"]) .with_stdout( @@ -157,7 +157,7 @@ fn warn_when_plugin_masks_manifest_on_stable() { path.push(p.root().join("path-test")); let path = std::env::join_paths(path.iter()).unwrap(); - p.cargo("echo.rs") + p.cargo("-v echo.rs") .env("PATH", &path) .with_stdout("") .with_stderr( @@ -176,7 +176,7 @@ fn requires_nightly() { .file("echo.rs", ECHO_SCRIPT) .build(); - p.cargo("echo.rs") + p.cargo("-v echo.rs") .with_status(101) .with_stdout("") .with_stderr( @@ -193,7 +193,7 @@ fn requires_z_flag() { .file("echo.rs", ECHO_SCRIPT) .build(); - p.cargo("echo.rs") + p.cargo("-v echo.rs") .masquerade_as_nightly_cargo(&["script"]) .with_status(101) .with_stdout("") @@ -221,7 +221,7 @@ fn main() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -252,7 +252,7 @@ fn main() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -281,7 +281,7 @@ fn main() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"msg = undefined @@ -298,7 +298,7 @@ fn main() { .run(); // Verify we don't rebuild - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"msg = undefined @@ -314,7 +314,7 @@ fn main() { .run(); // Verify we do rebuild - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .env("_MESSAGE", "hello") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( @@ -374,7 +374,7 @@ args: ["-NotAnArg"] } #[cargo_test] -fn default_verbosity() { +fn default_programmatic_verbosity() { let script = ECHO_SCRIPT; let p = cargo_test_support::project() .file("script.rs", script) @@ -389,10 +389,6 @@ args: ["-NotAnArg"] ) .with_stderr( "\ -[WARNING] `package.edition` is unspecifiead, defaulting to `2021` -[COMPILING] script v0.0.0 ([ROOT]/foo) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s -[RUNNING] `[..]/debug/script[EXE] -NotAnArg` ", ) .run(); @@ -431,7 +427,7 @@ fn main() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"line: 4 @@ -455,7 +451,7 @@ fn test_escaped_hyphen_arg() { .file("script.rs", script) .build(); - p.cargo("-Zscript -- script.rs -NotAnArg") + p.cargo("-Zscript -v -- script.rs -NotAnArg") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/script[EXE] @@ -480,7 +476,7 @@ fn test_unescaped_hyphen_arg() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs -NotAnArg") + p.cargo("-Zscript -v script.rs -NotAnArg") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/script[EXE] @@ -505,7 +501,7 @@ fn test_same_flags() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs --help") + p.cargo("-Zscript -v script.rs --help") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/script[EXE] @@ -530,7 +526,7 @@ fn test_name_has_weird_chars() { .file("s-h.w§c!.rs", script) .build(); - p.cargo("-Zscript s-h.w§c!.rs") + p.cargo("-Zscript -v s-h.w§c!.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [..]/debug/s-h-w-c-[EXE] @@ -553,7 +549,7 @@ fn script_like_dir() { .file("script.rs/foo", "something") .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_status(101) .with_stderr( @@ -568,7 +564,7 @@ error: manifest path `script.rs` is a directory but expected a file fn missing_script_rs() { let p = cargo_test_support::project().build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_status(101) .with_stderr( @@ -596,7 +592,7 @@ fn main() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs --help") + p.cargo("-Zscript -v script.rs --help") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -636,7 +632,7 @@ fn main() { .file("bar/src/lib.rs", "pub fn bar() {}") .build(); - p.cargo("-Zscript script.rs --help") + p.cargo("-Zscript -v script.rs --help") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -666,7 +662,7 @@ fn main() { .file("build.rs", "broken") .build(); - p.cargo("-Zscript script.rs --help") + p.cargo("-Zscript -v script.rs --help") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -695,7 +691,7 @@ fn main() { .file("src/bin/not-script/main.rs", "fn main() {}") .build(); - p.cargo("-Zscript script.rs --help") + p.cargo("-Zscript -v script.rs --help") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"Hello world! @@ -719,7 +715,7 @@ fn implicit_target_dir() { .file("script.rs", script) .build(); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [ROOT]/home/.cargo/target/[..]/debug/script[EXE] @@ -747,7 +743,7 @@ fn no_local_lockfile() { assert!(!local_lockfile_path.exists()); - p.cargo("-Zscript script.rs") + p.cargo("-Zscript -v script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout( r#"bin: [ROOT]/home/.cargo/target/[..]/debug/script[EXE]