From d674c2294ba8392ddbd9897ad9a285615f97994b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 11:29:11 -0500 Subject: [PATCH 1/3] refactor(cli): Make help behave like other subcommands Before, we had hacks to intercept raw arguments and to intercept clap errors and assume what their intention was to be able to implement our help system. This flips it around and makes help like any other subcommand, simplifying cargo initialization. --- Cargo.toml | 2 +- src/bin/cargo/cli.rs | 35 ++++-------------------- src/bin/cargo/commands/help.rs | 50 ++++++++++++---------------------- src/bin/cargo/commands/mod.rs | 2 ++ 4 files changed, 25 insertions(+), 64 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b887b6e8b98..21fb83dd47d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,7 +61,7 @@ toml_edit = { version = "0.14.3", features = ["serde", "easy", "perf"] } unicode-xid = "0.2.0" url = "2.2.2" walkdir = "2.2" -clap = "3.2.1" +clap = "3.2.18" unicode-width = "0.1.5" openssl = { version = '0.10.11', optional = true } im-rc = "15.0.0" diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 9e5eb70ceb8..d5a3a72e065 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -1,10 +1,7 @@ use anyhow::anyhow; use cargo::core::{features, CliUnstable}; use cargo::{self, drop_print, drop_println, CliResult, Config}; -use clap::{ - error::{ContextKind, ContextValue}, - AppSettings, Arg, ArgMatches, -}; +use clap::{AppSettings, Arg, ArgMatches}; use itertools::Itertools; use std::collections::HashMap; use std::fmt::Write; @@ -29,31 +26,7 @@ pub fn main(config: &mut Config) -> CliResult { // In general, try to avoid loading config values unless necessary (like // the [alias] table). - if commands::help::handle_embedded_help(config) { - return Ok(()); - } - - let args = match cli().try_get_matches() { - Ok(args) => args, - Err(e) => { - if e.kind() == clap::ErrorKind::UnrecognizedSubcommand { - // An unrecognized subcommand might be an external subcommand. - let cmd = e - .context() - .find_map(|c| match c { - (ContextKind::InvalidSubcommand, &ContextValue::String(ref cmd)) => { - Some(cmd) - } - _ => None, - }) - .expect("UnrecognizedSubcommand implies the presence of InvalidSubcommand"); - return super::execute_external_subcommand(config, cmd, &[cmd, "--help"]) - .map_err(|_| e.into()); - } else { - return Err(e.into()); - } - } - }; + let args = cli().try_get_matches()?; // Global args need to be extracted before expanding aliases because the // clap code for extracting a subcommand discards global options @@ -412,7 +385,7 @@ impl GlobalArgs { } } -fn cli() -> App { +pub fn cli() -> App { let is_rustup = std::env::var_os("RUSTUP_HOME").is_some(); let usage = if is_rustup { "cargo [+toolchain] [OPTIONS] [SUBCOMMAND]" @@ -425,6 +398,8 @@ fn cli() -> App { // Doesn't mix well with our list of common cargo commands. See clap-rs/clap#3108 for // opening clap up to allow us to style our help template .disable_colored_help(true) + // Provide a custom help subcommand for calling into man pages + .disable_help_subcommand(true) .override_usage(usage) .help_template( "\ diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index 16b7e71e6be..fa0d98fd81a 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -1,4 +1,5 @@ use crate::aliased_command; +use crate::command_prelude::*; use cargo::util::errors::CargoResult; use cargo::{drop_println, Config}; use cargo_util::paths::resolve_executable; @@ -10,43 +11,26 @@ use std::path::Path; const COMPRESSED_MAN: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/man.tgz")); -/// Checks if the `help` command is being issued. -/// -/// This runs before clap processing, because it needs to intercept the `help` -/// command if a man page is available. -/// -/// Returns `true` if help information was successfully displayed to the user. -/// In this case, Cargo should exit. -pub fn handle_embedded_help(config: &Config) -> bool { - match try_help(config) { - Ok(true) => true, - Ok(false) => false, - Err(e) => { - log::warn!("help failed: {:?}", e); - false - } - } +pub fn cli() -> App { + subcommand("help") + .about("Displays help for a cargo subcommand") + .arg(Arg::new("SUBCOMMAND")) } -fn try_help(config: &Config) -> CargoResult { - let mut args = std::env::args_os() - .skip(1) - .skip_while(|arg| arg.to_str().map_or(false, |s| s.starts_with('-'))); - if !args - .next() - .map_or(false, |arg| arg.to_str() == Some("help")) - { - return Ok(false); +pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { + let subcommand = args.get_one::("SUBCOMMAND"); + if let Some(subcommand) = subcommand { + if !try_help(config, subcommand)? { + crate::execute_external_subcommand(config, subcommand, &[subcommand, "--help"])?; + } + } else { + let mut cmd = crate::cli::cli(); + let _ = cmd.print_help(); } - let subcommand = match args.next() { - Some(arg) => arg, - None => return Ok(false), - }; - let subcommand = match subcommand.to_str() { - Some(s) => s, - None => return Ok(false), - }; + Ok(()) +} +fn try_help(config: &Config, subcommand: &str) -> CargoResult { let subcommand = match check_alias(config, subcommand) { // If this alias is more than a simple subcommand pass-through, show the alias. Some(argv) if argv.len() > 1 => { diff --git a/src/bin/cargo/commands/mod.rs b/src/bin/cargo/commands/mod.rs index 92ee7d75e45..0499f9a437f 100644 --- a/src/bin/cargo/commands/mod.rs +++ b/src/bin/cargo/commands/mod.rs @@ -13,6 +13,7 @@ pub fn builtin() -> Vec { fix::cli(), generate_lockfile::cli(), git_checkout::cli(), + help::cli(), init::cli(), install::cli(), locate_project::cli(), @@ -54,6 +55,7 @@ pub fn builtin_exec(cmd: &str) -> Option CliResu "fix" => fix::exec, "generate-lockfile" => generate_lockfile::exec, "git-checkout" => git_checkout::exec, + "help" => help::exec, "init" => init::exec, "install" => install::exec, "locate-project" => locate_project::exec, From ddc21ea4cdf01bf784f063f0815c4aaf0367f0be Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 11:46:54 -0500 Subject: [PATCH 2/3] refactor(cli): Delay fix's access to config My hope is to make it so we can lazy load the config. This makes it so we only load the config for the fix proxy if needed. I also feel like this better clarifies the intention of the code that we are running in a special mode. --- src/bin/cargo/main.rs | 12 +++++------- src/cargo/ops/fix.rs | 23 +++++++++++++---------- src/cargo/ops/mod.rs | 2 +- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 1619b487b2f..bd439f8f193 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -30,13 +30,11 @@ fn main() { } }; - let result = match cargo::ops::fix_maybe_exec_rustc(&config) { - Ok(true) => Ok(()), - Ok(false) => { - let _token = cargo::util::job::setup(); - cli::main(&mut config) - } - Err(e) => Err(CliError::from(e)), + let result = if let Some(lock_addr) = cargo::ops::fix_get_proxy_lock_addr() { + cargo::ops::fix_exec_rustc(&config, &lock_addr).map_err(|e| CliError::from(e)) + } else { + let _token = cargo::util::job::setup(); + cli::main(&mut config) }; match result { diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 0b40a2942a6..5fb35b8905d 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -333,20 +333,23 @@ to prevent this issue from happening. Ok(()) } +/// Provide the lock address when running in proxy mode +/// +/// Returns `None` if `fix` is not being run (not in proxy mode). Returns +/// `Some(...)` if in `fix` proxy mode +pub fn fix_get_proxy_lock_addr() -> Option { + env::var(FIX_ENV).ok() +} + /// Entry point for `cargo` running as a proxy for `rustc`. /// /// This is called every time `cargo` is run to check if it is in proxy mode. /// -/// Returns `false` if `fix` is not being run (not in proxy mode). Returns -/// `true` if in `fix` proxy mode, and the fix was complete without any -/// warnings or errors. If there are warnings or errors, this does not return, +/// If there are warnings or errors, this does not return, /// and the process exits with the corresponding `rustc` exit code. -pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { - let lock_addr = match env::var(FIX_ENV) { - Ok(s) => s, - Err(_) => return Ok(false), - }; - +/// +/// See [`fix_proxy_lock_addr`] +pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> { let args = FixArgs::get()?; trace!("cargo-fix as rustc got file {:?}", args.file); @@ -392,7 +395,7 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { // any. If stderr is empty then there's no need for the final exec at // the end, we just bail out here. if output.status.success() && output.stderr.is_empty() { - return Ok(true); + return Ok(()); } // Otherwise, if our rustc just failed, then that means that we broke the diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index a5715c13ec3..d9895ea0882 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -19,7 +19,7 @@ pub use self::cargo_read_manifest::{read_package, read_packages}; pub use self::cargo_run::run; pub use self::cargo_test::{run_benches, run_tests, TestOptions}; pub use self::cargo_uninstall::uninstall; -pub use self::fix::{fix, fix_maybe_exec_rustc, FixOptions}; +pub use self::fix::{fix, fix_exec_rustc, fix_get_proxy_lock_addr, FixOptions}; pub use self::lockfile::{load_pkg_lockfile, resolve_to_string, write_pkg_lockfile}; pub use self::registry::HttpTimeout; pub use self::registry::{configure_http_handle, http_handle, http_handle_and_timeout}; From eda1652b401de91bc5373906725aaa5a9b3dd04a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 12:06:19 -0500 Subject: [PATCH 3/3] refactor(cli): Lazily do first-pass config loading This will be a help for cases like #10952 which I would expect would assert that the config is not loaded before changing the current_dir. --- src/bin/cargo/cli.rs | 51 ++++++++++++++++++++++++++++++++++++++++--- src/bin/cargo/main.rs | 13 +++-------- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index d5a3a72e065..5d7fc66d37f 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -1,4 +1,5 @@ use anyhow::anyhow; +use cargo::core::shell::Shell; use cargo::core::{features, CliUnstable}; use cargo::{self, drop_print, drop_println, CliResult, Config}; use clap::{AppSettings, Arg, ArgMatches}; @@ -21,12 +22,13 @@ lazy_static::lazy_static! { ]); } -pub fn main(config: &mut Config) -> CliResult { +pub fn main(config: &mut LazyConfig) -> CliResult { + let args = cli().try_get_matches()?; + // 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().try_get_matches()?; + let config = config.get_mut(); // Global args need to be extracted before expanding aliases because the // clap code for extracting a subcommand discards global options @@ -463,6 +465,49 @@ See 'cargo help ' for more information on a specific command.\n", .subcommands(commands::builtin()) } +/// Delay loading [`Config`] until access. +/// +/// In the common path, the [`Config`] 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 LazyConfig { + config: Option, +} + +impl LazyConfig { + pub fn new() -> Self { + Self { config: 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.config.is_some() + } + + /// Get the config, loading it if needed + /// + /// On error, the process is terminated + pub fn get(&mut self) -> &Config { + self.get_mut() + } + + /// Get the config, loading it if needed + /// + /// On error, the process is terminated + pub fn get_mut(&mut self) -> &mut Config { + self.config.get_or_insert_with(|| match Config::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(); diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index bd439f8f193..e4f224031cd 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -1,7 +1,6 @@ #![warn(rust_2018_idioms)] // while we're getting used to 2018 #![allow(clippy::all)] -use cargo::core::shell::Shell; use cargo::util::toml::StringOrVec; use cargo::util::CliError; use cargo::util::{self, closest_msg, command_prelude, CargoResult, CliResult, Config}; @@ -22,23 +21,17 @@ fn main() { #[cfg(not(feature = "pretty-env-logger"))] env_logger::init_from_env("CARGO_LOG"); - let mut config = match Config::default() { - Ok(cfg) => cfg, - Err(e) => { - let mut shell = Shell::new(); - cargo::exit_with_error(e.into(), &mut shell) - } - }; + let mut config = cli::LazyConfig::new(); let result = if let Some(lock_addr) = cargo::ops::fix_get_proxy_lock_addr() { - cargo::ops::fix_exec_rustc(&config, &lock_addr).map_err(|e| CliError::from(e)) + cargo::ops::fix_exec_rustc(config.get(), &lock_addr).map_err(|e| CliError::from(e)) } else { let _token = cargo::util::job::setup(); cli::main(&mut config) }; match result { - Err(e) => cargo::exit_with_error(e, &mut *config.shell()), + Err(e) => cargo::exit_with_error(e, &mut config.get_mut().shell()), Ok(()) => {} } }