From a05afe443f48ae70595afc396c2648cdb3c07fc5 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 13 Mar 2019 22:53:06 -0700 Subject: [PATCH 01/13] WIP cargo-clippy command --- src/bin/cargo/commands/clippy.rs | 76 ++++++++++++++++++++++++++++++++ src/bin/cargo/commands/mod.rs | 3 ++ src/cargo/util/config.rs | 18 +++++++- 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 src/bin/cargo/commands/clippy.rs diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs new file mode 100644 index 00000000000..e4f4d5b1b66 --- /dev/null +++ b/src/bin/cargo/commands/clippy.rs @@ -0,0 +1,76 @@ +use crate::command_prelude::*; + +use cargo::ops; + +pub fn cli() -> App { + subcommand("clippy-preview") + // subcommand aliases are handled in aliased_command() + // .alias("c") + .about("Check a local package and all of its dependencies for errors") + .arg_package_spec( + "Package(s) to check", + "Check all packages in the workspace", + "Exclude packages from the check", + ) + .arg_jobs() + .arg_targets_all( + "Check only this package's library", + "Check only the specified binary", + "Check all binaries", + "Check only the specified example", + "Check all examples", + "Check only the specified test target", + "Check all tests", + "Check only the specified bench target", + "Check all benches", + "Check all targets", + ) + .arg_release("Check artifacts in release mode, with optimizations") + .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE")) + .arg_features() + .arg_target_triple("Check for the target triple") + .arg_target_dir() + .arg_manifest_path() + .arg_message_format() + .after_help( + "\ +If the `--package` argument is given, then SPEC is a package ID specification +which indicates which package should be built. If it is not given, then the +current package is built. For more information on SPEC and its format, see the +`cargo help pkgid` command. + +All packages in the workspace are checked if the `--all` flag is supplied. The +`--all` flag is automatically assumed for a virtual manifest. +Note that `--exclude` has to be specified in conjunction with the `--all` flag. + +Compilation can be configured via the use of profiles which are configured in +the manifest. The default profile for this command is `dev`, but passing +the `--release` flag will use the `release` profile instead. + +The `--profile test` flag can be used to check unit tests with the +`#[cfg(test)]` attribute. +", + ) +} + +pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { + config.set_clippy_override(true); + let ws = args.workspace(config)?; + let test = match args.value_of("profile") { + Some("test") => true, + None => false, + Some(profile) => { + let err = failure::format_err!( + "unknown profile: `{}`, only `test` is \ + currently supported", + profile + ); + return Err(CliError::new(err, 101)); + } + }; + let mode = CompileMode::Check { test }; + let compile_opts = args.compile_options(config, mode, Some(&ws))?; + + ops::compile(&ws, &compile_opts)?; + Ok(()) +} diff --git a/src/bin/cargo/commands/mod.rs b/src/bin/cargo/commands/mod.rs index c62f2aad211..860c78de674 100644 --- a/src/bin/cargo/commands/mod.rs +++ b/src/bin/cargo/commands/mod.rs @@ -6,6 +6,7 @@ pub fn builtin() -> Vec { build::cli(), check::cli(), clean::cli(), + clippy::cli(), doc::cli(), fetch::cli(), fix::cli(), @@ -41,6 +42,7 @@ pub fn builtin_exec(cmd: &str) -> Option) -> Cli "build" => build::exec, "check" => check::exec, "clean" => clean::exec, + "clippy-preview" => clippy::exec, "doc" => doc::exec, "fetch" => fetch::exec, "fix" => fix::exec, @@ -76,6 +78,7 @@ pub mod bench; pub mod build; pub mod check; pub mod clean; +pub mod clippy; pub mod doc; pub mod fetch; pub mod fix; diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 3f96618c7ac..2e97e098301 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -74,6 +74,8 @@ pub struct Config { env: HashMap, /// Profiles loaded from config. profiles: LazyCell, + /// Use `clippy-driver` instead of `rustc` + clippy_override: bool, } impl Config { @@ -129,6 +131,7 @@ impl Config { target_dir: None, env, profiles: LazyCell::new(), + clippy_override: false, } } @@ -190,6 +193,11 @@ impl Config { .map(AsRef::as_ref) } + /// Sets the clippy override. If this is true, clippy-driver is invoked instead of rustc. + pub fn set_clippy_override(&mut self, val: bool) { + self.clippy_override = val; + } + /// Gets the path to the `rustc` executable. pub fn rustc(&self, ws: Option<&Workspace<'_>>) -> CargoResult { let cache_location = ws.map(|ws| { @@ -197,9 +205,14 @@ impl Config { .join(".rustc_info.json") .into_path_unlocked() }); + let wrapper = if self.clippy_override { + Some(self.get_tool("clippy-driver")?) + } else { + self.maybe_get_tool("rustc-wrapper")? + }; Rustc::new( self.get_tool("rustc")?, - self.maybe_get_tool("rustc_wrapper")?, + wrapper, &self .home() .join("bin") @@ -743,14 +756,17 @@ impl Config { } else { PathBuf::from(tool_path) }; + println!("some tool {}", tool); return Ok(Some(path)); } let var = format!("build.{}", tool); if let Some(tool_path) = self.get_path(&var)? { + println!("some tool {}", tool); return Ok(Some(tool_path.val)); } + println!("NO TOOL {}", tool); Ok(None) } From 30da7728e685cfb2c77b63438e42903fcd843f3a Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 15 Mar 2019 11:33:01 -0700 Subject: [PATCH 02/13] fix clippy help text, check for clippy-driver --- src/bin/cargo/commands/clippy.rs | 14 +++++++++++++- src/bin/cargo/main.rs | 17 ++--------------- src/cargo/util/config.rs | 20 +++++++++++++------- src/cargo/util/paths.rs | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index e4f4d5b1b66..bb8fe553b78 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -6,7 +6,7 @@ pub fn cli() -> App { subcommand("clippy-preview") // subcommand aliases are handled in aliased_command() // .alias("c") - .about("Check a local package and all of its dependencies for errors") + .about("Checks a package to catch common mistakes and improve your Rust code.") .arg_package_spec( "Package(s) to check", "Check all packages in the workspace", @@ -49,6 +49,18 @@ the `--release` flag will use the `release` profile instead. The `--profile test` flag can be used to check unit tests with the `#[cfg(test)]` attribute. + +To allow or deny a lint from the command line you can use `cargo clippy --` +with: + + -W --warn OPT Set lint warnings + -A --allow OPT Set lint allowed + -D --deny OPT Set lint denied + -F --forbid OPT Set lint forbidden + +You can use tool lints to allow or deny lints from your code, eg.: + + #[allow(clippy::needless_lifetimes)] ", ) } diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 39bc48f6887..242e84e4e2f 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -7,9 +7,10 @@ use std::collections::BTreeSet; use std::env; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use cargo::core::shell::Shell; +use cargo::util::paths::is_executable; use cargo::util::{self, command_prelude, lev_distance, CargoResult, CliResult, Config}; use cargo::util::{CliError, ProcessError}; @@ -164,20 +165,6 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> Cli Err(CliError::new(err, 101)) } -#[cfg(unix)] -fn is_executable>(path: P) -> bool { - use std::os::unix::prelude::*; - fs::metadata(path) - .map(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) - .unwrap_or(false) -} -#[cfg(windows)] -fn is_executable>(path: P) -> bool { - fs::metadata(path) - .map(|metadata| metadata.is_file()) - .unwrap_or(false) -} - fn search_directories(config: &Config) -> Vec { let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")]; if let Some(val) = env::var_os("PATH") { diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 2e97e098301..492fc9f1058 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -20,6 +20,7 @@ use serde::Deserialize; use serde::{de, de::IntoDeserializer}; use url::Url; +use self::ConfigValue as CV; use crate::core::profiles::ConfigProfiles; use crate::core::shell::Verbosity; use crate::core::{CliUnstable, Shell, SourceId, Workspace}; @@ -30,7 +31,6 @@ use crate::util::Filesystem; use crate::util::Rustc; use crate::util::ToUrl; use crate::util::{paths, validate_package_name}; -use self::ConfigValue as CV; /// Configuration information for cargo. This is not specific to a build, it is information /// relating to cargo itself. @@ -206,7 +206,16 @@ impl Config { .into_path_unlocked() }); let wrapper = if self.clippy_override { - Some(self.get_tool("clippy-driver")?) + let tool = self.get_tool("clippy-driver")?; + let tool = paths::resolve_executable(&tool).map_err(|e| { + failure::format_err!("{}: please run `rustup component add clippy`", e) + })?; + if !paths::is_executable(&tool) { + return Err(failure::format_err!( + "found file for `clippy-driver` but its not an executable. what the heck is going on !? please run `rustup component add clippy`" + )); + } + Some(tool) } else { self.maybe_get_tool("rustc-wrapper")? }; @@ -407,6 +416,7 @@ impl Config { match self.get_env(key)? { Some(v) => Ok(Some(v)), None => { + // println!("{:#?}", self); let config_key = key.to_config(); let o_cv = self.get_cv(&config_key)?; match o_cv { @@ -756,17 +766,14 @@ impl Config { } else { PathBuf::from(tool_path) }; - println!("some tool {}", tool); return Ok(Some(path)); } let var = format!("build.{}", tool); if let Some(tool_path) = self.get_path(&var)? { - println!("some tool {}", tool); return Ok(Some(tool_path.val)); } - println!("NO TOOL {}", tool); Ok(None) } @@ -940,8 +947,7 @@ impl ConfigError { } } -impl std::error::Error for ConfigError { -} +impl std::error::Error for ConfigError {} // Future note: currently, we cannot override `Fail::cause` (due to // specialization) so we have no way to return the underlying causes. In the diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index 8556450ece6..3edd81677ce 100644 --- a/src/cargo/util/paths.rs +++ b/src/cargo/util/paths.rs @@ -89,6 +89,20 @@ pub fn normalize_path(path: &Path) -> PathBuf { ret } +#[cfg(unix)] +pub fn is_executable>(path: P) -> bool { + use std::os::unix::prelude::*; + fs::metadata(path) + .map(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) + .unwrap_or(false) +} +#[cfg(windows)] +pub fn is_executable>(path: P) -> bool { + fs::metadata(path) + .map(|metadata| metadata.is_file()) + .unwrap_or(false) +} + pub fn resolve_executable(exec: &Path) -> CargoResult { if exec.components().count() == 1 { let paths = env::var_os("PATH").ok_or_else(|| failure::format_err!("no PATH"))?; From 4051b0cb571a82eb91d4664a24c8c5028aea8f32 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 18 Mar 2019 13:51:29 -0700 Subject: [PATCH 03/13] fix: mark clippy-preview unstable, remove cruft --- src/bin/cargo/commands/clippy.rs | 8 ++++++++ src/cargo/util/config.rs | 1 - 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index bb8fe553b78..5fe703a7143 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -80,9 +80,17 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { return Err(CliError::new(err, 101)); } }; + let mode = CompileMode::Check { test }; let compile_opts = args.compile_options(config, mode, Some(&ws))?; + if !config.cli_unstable().unstable_options { + return Err(failure::format_err!( + "`clippy-preview` is unstable, pass `-Z unstable-options` to enable it" + ) + .into()); + } + ops::compile(&ws, &compile_opts)?; Ok(()) } diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 492fc9f1058..c25c7eac892 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -416,7 +416,6 @@ impl Config { match self.get_env(key)? { Some(v) => Ok(Some(v)), None => { - // println!("{:#?}", self); let config_key = key.to_config(); let o_cv = self.get_cv(&config_key)?; match o_cv { From 98520877ae0573a41b10f6a97cd3ecb376f2e008 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 18 Mar 2019 16:22:37 -0700 Subject: [PATCH 04/13] fix: remove profile arg, exe check, add rustup check --- src/bin/cargo/commands/clippy.rs | 24 +----------------------- src/bin/cargo/main.rs | 17 +++++++++++++++-- src/cargo/util/config.rs | 16 ++++++++++------ src/cargo/util/paths.rs | 14 -------------- 4 files changed, 26 insertions(+), 45 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index 5fe703a7143..d8ad772a897 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -4,8 +4,6 @@ use cargo::ops; pub fn cli() -> App { subcommand("clippy-preview") - // subcommand aliases are handled in aliased_command() - // .alias("c") .about("Checks a package to catch common mistakes and improve your Rust code.") .arg_package_spec( "Package(s) to check", @@ -26,7 +24,6 @@ pub fn cli() -> App { "Check all targets", ) .arg_release("Check artifacts in release mode, with optimizations") - .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE")) .arg_features() .arg_target_triple("Check for the target triple") .arg_target_dir() @@ -43,13 +40,6 @@ All packages in the workspace are checked if the `--all` flag is supplied. The `--all` flag is automatically assumed for a virtual manifest. Note that `--exclude` has to be specified in conjunction with the `--all` flag. -Compilation can be configured via the use of profiles which are configured in -the manifest. The default profile for this command is `dev`, but passing -the `--release` flag will use the `release` profile instead. - -The `--profile test` flag can be used to check unit tests with the -`#[cfg(test)]` attribute. - To allow or deny a lint from the command line you can use `cargo clippy --` with: @@ -68,20 +58,8 @@ You can use tool lints to allow or deny lints from your code, eg.: pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { config.set_clippy_override(true); let ws = args.workspace(config)?; - let test = match args.value_of("profile") { - Some("test") => true, - None => false, - Some(profile) => { - let err = failure::format_err!( - "unknown profile: `{}`, only `test` is \ - currently supported", - profile - ); - return Err(CliError::new(err, 101)); - } - }; - let mode = CompileMode::Check { test }; + let mode = CompileMode::Check { test: false }; let compile_opts = args.compile_options(config, mode, Some(&ws))?; if !config.cli_unstable().unstable_options { diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 242e84e4e2f..39bc48f6887 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -7,10 +7,9 @@ use std::collections::BTreeSet; use std::env; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use cargo::core::shell::Shell; -use cargo::util::paths::is_executable; use cargo::util::{self, command_prelude, lev_distance, CargoResult, CliResult, Config}; use cargo::util::{CliError, ProcessError}; @@ -165,6 +164,20 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> Cli Err(CliError::new(err, 101)) } +#[cfg(unix)] +fn is_executable>(path: P) -> bool { + use std::os::unix::prelude::*; + fs::metadata(path) + .map(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) + .unwrap_or(false) +} +#[cfg(windows)] +fn is_executable>(path: P) -> bool { + fs::metadata(path) + .map(|metadata| metadata.is_file()) + .unwrap_or(false) +} + fn search_directories(config: &Config) -> Vec { let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")]; if let Some(val) = env::var_os("PATH") { diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index c25c7eac892..c736317ea98 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -208,13 +208,17 @@ impl Config { let wrapper = if self.clippy_override { let tool = self.get_tool("clippy-driver")?; let tool = paths::resolve_executable(&tool).map_err(|e| { - failure::format_err!("{}: please run `rustup component add clippy`", e) + let rustup_in_path = self + .get_tool("rustup") + .and_then(|tool| paths::resolve_executable(&tool)) + .is_ok(); + let has_rustup_env = std::env::var("RUSTUP_TOOLCHAIN").is_ok(); + if dbg!(rustup_in_path) || dbg!(has_rustup_env) { + failure::format_err!("{}: please run `rustup component add clippy`", e) + } else { + failure::format_err!("{}: please install clippy component", e) + } })?; - if !paths::is_executable(&tool) { - return Err(failure::format_err!( - "found file for `clippy-driver` but its not an executable. what the heck is going on !? please run `rustup component add clippy`" - )); - } Some(tool) } else { self.maybe_get_tool("rustc-wrapper")? diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index 3edd81677ce..8556450ece6 100644 --- a/src/cargo/util/paths.rs +++ b/src/cargo/util/paths.rs @@ -89,20 +89,6 @@ pub fn normalize_path(path: &Path) -> PathBuf { ret } -#[cfg(unix)] -pub fn is_executable>(path: P) -> bool { - use std::os::unix::prelude::*; - fs::metadata(path) - .map(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) - .unwrap_or(false) -} -#[cfg(windows)] -pub fn is_executable>(path: P) -> bool { - fs::metadata(path) - .map(|metadata| metadata.is_file()) - .unwrap_or(false) -} - pub fn resolve_executable(exec: &Path) -> CargoResult { if exec.components().count() == 1 { let paths = env::var_os("PATH").ok_or_else(|| failure::format_err!("no PATH"))?; From 1f8c1c4f60685f8f6abee9b76495c0c7e654445f Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 18 Mar 2019 16:25:44 -0700 Subject: [PATCH 05/13] fix: remove dbg macro calls --- src/cargo/util/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index c736317ea98..f7de0c0f8b5 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -213,7 +213,7 @@ impl Config { .and_then(|tool| paths::resolve_executable(&tool)) .is_ok(); let has_rustup_env = std::env::var("RUSTUP_TOOLCHAIN").is_ok(); - if dbg!(rustup_in_path) || dbg!(has_rustup_env) { + if rustup_in_path || has_rustup_env { failure::format_err!("{}: please run `rustup component add clippy`", e) } else { failure::format_err!("{}: please install clippy component", e) From e2f2c8d6018bbccc40f5ed73d9579ec69afaa5ad Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 18 Mar 2019 16:45:41 -0700 Subject: [PATCH 06/13] fix: non rustup clippy missing error --- src/cargo/util/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index f7de0c0f8b5..9312152e6d2 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -216,7 +216,7 @@ impl Config { if rustup_in_path || has_rustup_env { failure::format_err!("{}: please run `rustup component add clippy`", e) } else { - failure::format_err!("{}: please install clippy component", e) + failure::format_err!("{}: please install clippy", e) } })?; Some(tool) From 0389edc69bcb66c2dd7738523e17da7e5fd82622 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Thu, 21 Mar 2019 15:57:14 -0700 Subject: [PATCH 07/13] impl centralized RustcWrapper logic --- src/bin/cargo/cli.rs | 3 +- src/bin/cargo/commands/clippy.rs | 4 +- src/cargo/core/compiler/build_config.rs | 8 +++ src/cargo/core/compiler/build_context/mod.rs | 27 ++++++++- src/cargo/core/compiler/compilation.rs | 16 +---- src/cargo/core/compiler/mod.rs | 14 ++--- src/cargo/ops/cargo_fetch.rs | 2 +- src/cargo/util/config.rs | 31 +--------- src/cargo/util/mod.rs | 2 +- src/cargo/util/process_builder.rs | 5 +- src/cargo/util/rustc.rs | 63 ++++++++++++++++++-- 11 files changed, 111 insertions(+), 64 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index af02bbbfae7..42d1f9c642a 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -48,7 +48,7 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" } if let Some(code) = args.value_of("explain") { - let mut procss = config.rustc(None)?.process(); + let mut procss = config.load_global_rustc(None)?.process(); procss.arg("--explain").arg(code).exec()?; return Ok(()); } @@ -236,4 +236,3 @@ See 'cargo help ' for more information on a specific command.\n", ) .subcommands(commands::builtin()) } - diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index d8ad772a897..a821606eaa7 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -56,11 +56,11 @@ You can use tool lints to allow or deny lints from your code, eg.: } pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { - config.set_clippy_override(true); let ws = args.workspace(config)?; let mode = CompileMode::Check { test: false }; - let compile_opts = args.compile_options(config, mode, Some(&ws))?; + let mut compile_opts = args.compile_options(config, mode, Some(&ws))?; + compile_opts.build_config.set_clippy_override(true); if !config.cli_unstable().unstable_options { return Err(failure::format_err!( diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 70b2b355cc9..04997ffcaf1 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -30,6 +30,8 @@ pub struct BuildConfig { /// Extra args to inject into rustc commands. pub extra_rustc_args: Vec, pub rustfix_diagnostic_server: RefCell>, + /// Use `clippy-driver` instead of `rustc` + pub clippy_override: bool, } impl BuildConfig { @@ -102,6 +104,7 @@ impl BuildConfig { extra_rustc_env: Vec::new(), extra_rustc_args: Vec::new(), rustfix_diagnostic_server: RefCell::new(None), + clippy_override: false, }) } @@ -112,6 +115,11 @@ impl BuildConfig { pub fn test(&self) -> bool { self.mode == CompileMode::Test || self.mode == CompileMode::Bench } + + /// Sets the clippy override. If this is true, clippy-driver is invoked instead of rustc. + pub fn set_clippy_override(&mut self, val: bool) { + self.clippy_override = val; + } } #[derive(Clone, Copy, Debug, PartialEq, Eq)] diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index cc3096c4eb5..d28386f4f6f 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -9,6 +9,8 @@ use crate::core::profiles::Profiles; use crate::core::{Dependency, Workspace}; use crate::core::{PackageId, PackageSet, Resolve}; use crate::util::errors::CargoResult; +use crate::util::paths; +use crate::util::rustc::RustcWrapper; use crate::util::{profile, Cfg, CfgExpr, Config, Rustc}; use super::{BuildConfig, BuildOutput, Kind, Unit}; @@ -50,7 +52,30 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { profiles: &'a Profiles, extra_compiler_args: HashMap, Vec>, ) -> CargoResult> { - let rustc = config.rustc(Some(ws))?; + let mut rustc = config.load_global_rustc(Some(ws))?; + + if build_config.clippy_override { + let tool = config.get_tool("clippy-driver")?; + let tool = paths::resolve_executable(&tool).map_err(|e| { + let rustup_in_path = config + .get_tool("rustup") + .and_then(|tool| paths::resolve_executable(&tool)) + .is_ok(); + let has_rustup_env = std::env::var("RUSTUP_TOOLCHAIN").is_ok(); + if rustup_in_path || has_rustup_env { + failure::format_err!("{}: please run `rustup component add clippy`", e) + } else { + failure::format_err!("{}: please install clippy", e) + } + })?; + rustc.push_wrapper(RustcWrapper::new(tool)); + } else if build_config.cargo_as_rustc_wrapper { + let mut wrapper = RustcWrapper::new(env::current_exe()?); + let prog = rustc.path.as_os_str().to_owned(); + wrapper.env("RUSTC", prog); + rustc.push_wrapper(wrapper); + } + let host_config = TargetConfig::new(config, &rustc.host)?; let target_config = match build_config.requested_target.as_ref() { Some(triple) => TargetConfig::new(config, triple)?, diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 11014bae39a..5313a3b10a6 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -75,20 +75,8 @@ pub struct Compilation<'cfg> { impl<'cfg> Compilation<'cfg> { pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult> { - // If we're using cargo as a rustc wrapper then we're in a situation - // like `cargo fix`. For now just disregard the `RUSTC_WRAPPER` env var - // (which is typically set to `sccache` for now). Eventually we'll - // probably want to implement `RUSTC_WRAPPER` for `cargo fix`, but we'll - // leave that open as a bug for now. - let mut rustc = if bcx.build_config.cargo_as_rustc_wrapper { - let mut rustc = bcx.rustc.process_no_wrapper(); - let prog = rustc.get_program().to_owned(); - rustc.env("RUSTC", prog); - rustc.program(env::current_exe()?); - rustc - } else { - bcx.rustc.process() - }; + let mut rustc = bcx.rustc.process(); + if bcx.config.extra_verbose() { rustc.display_env_vars(); } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 534c707d6c1..c58a78679d1 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -22,13 +22,6 @@ use log::debug; use same_file::is_same_file; use serde::Serialize; -use crate::core::manifest::TargetSourcePath; -use crate::core::profiles::{Lto, PanicStrategy, Profile}; -use crate::core::{PackageId, Target}; -use crate::util::errors::{CargoResult, CargoResultExt, Internal, ProcessError}; -use crate::util::paths; -use crate::util::{self, machine_message, process, Freshness, ProcessBuilder}; -use crate::util::{internal, join_paths, profile}; pub use self::build_config::{BuildConfig, CompileMode, MessageFormat}; pub use self::build_context::{BuildContext, FileFlavor, TargetConfig, TargetInfo}; use self::build_plan::BuildPlan; @@ -39,6 +32,13 @@ use self::job::{Job, Work}; use self::job_queue::JobQueue; pub use self::layout::is_bad_artifact_name; use self::output_depinfo::output_depinfo; +use crate::core::manifest::TargetSourcePath; +use crate::core::profiles::{Lto, PanicStrategy, Profile}; +use crate::core::{PackageId, Target}; +use crate::util::errors::{CargoResult, CargoResultExt, Internal, ProcessError}; +use crate::util::paths; +use crate::util::{self, machine_message, process, Freshness, ProcessBuilder}; +use crate::util::{internal, join_paths, profile}; /// Indicates whether an object is for the host architcture or the target architecture. /// diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index 179b659f8c7..37ccc07c438 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -21,7 +21,7 @@ pub fn fetch<'a>( let jobs = Some(1); let config = ws.config(); let build_config = BuildConfig::new(config, jobs, &options.target, CompileMode::Build)?; - let rustc = config.rustc(Some(ws))?; + let rustc = config.load_global_rustc(Some(ws))?; let target_info = TargetInfo::new(config, &build_config.requested_target, &rustc, Kind::Target)?; { diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 9312152e6d2..46996ef724d 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -74,8 +74,6 @@ pub struct Config { env: HashMap, /// Profiles loaded from config. profiles: LazyCell, - /// Use `clippy-driver` instead of `rustc` - clippy_override: bool, } impl Config { @@ -131,7 +129,6 @@ impl Config { target_dir: None, env, profiles: LazyCell::new(), - clippy_override: false, } } @@ -193,36 +190,14 @@ impl Config { .map(AsRef::as_ref) } - /// Sets the clippy override. If this is true, clippy-driver is invoked instead of rustc. - pub fn set_clippy_override(&mut self, val: bool) { - self.clippy_override = val; - } - /// Gets the path to the `rustc` executable. - pub fn rustc(&self, ws: Option<&Workspace<'_>>) -> CargoResult { + pub fn load_global_rustc(&self, ws: Option<&Workspace<'_>>) -> CargoResult { let cache_location = ws.map(|ws| { ws.target_dir() .join(".rustc_info.json") .into_path_unlocked() }); - let wrapper = if self.clippy_override { - let tool = self.get_tool("clippy-driver")?; - let tool = paths::resolve_executable(&tool).map_err(|e| { - let rustup_in_path = self - .get_tool("rustup") - .and_then(|tool| paths::resolve_executable(&tool)) - .is_ok(); - let has_rustup_env = std::env::var("RUSTUP_TOOLCHAIN").is_ok(); - if rustup_in_path || has_rustup_env { - failure::format_err!("{}: please run `rustup component add clippy`", e) - } else { - failure::format_err!("{}: please install clippy", e) - } - })?; - Some(tool) - } else { - self.maybe_get_tool("rustc-wrapper")? - }; + let wrapper = self.maybe_get_tool("rustc-wrapper")?; Rustc::new( self.get_tool("rustc")?, wrapper, @@ -782,7 +757,7 @@ impl Config { /// Looks for a path for `tool` in an environment variable or config path, defaulting to `tool` /// as a path. - fn get_tool(&self, tool: &str) -> CargoResult { + pub fn get_tool(&self, tool: &str) -> CargoResult { self.maybe_get_tool(tool) .map(|t| t.unwrap_or_else(|| PathBuf::from(tool))) } diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index d973016ba02..e46df6ce943 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -47,7 +47,7 @@ pub mod process_builder; pub mod profile; mod progress; mod read2; -mod rustc; +pub mod rustc; mod sha256; pub mod to_semver; pub mod to_url; diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index de96420cc9b..ddd92be7380 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -80,10 +80,7 @@ impl ProcessBuilder { /// (chainable) Replaces the args list with the given `args`. pub fn args_replace>(&mut self, args: &[T]) -> &mut ProcessBuilder { - self.args = args - .iter() - .map(|t| t.as_ref().to_os_string()) - .collect(); + self.args = args.iter().map(|t| t.as_ref().to_os_string()).collect(); self } diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 59fd2304179..88699feaddd 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -2,6 +2,7 @@ use std::collections::hash_map::{Entry, HashMap}; use std::env; +use std::ffi::{OsStr, OsString}; use std::hash::{Hash, Hasher, SipHasher}; use std::path::{Path, PathBuf}; use std::process::Stdio; @@ -20,7 +21,7 @@ pub struct Rustc { pub path: PathBuf, /// An optional program that will be passed the path of the rust exe as its first argument, and /// rustc args following this. - pub wrapper: Option, + pub wrapper: Option, /// Verbose version information (the output of `rustc -vV`) pub verbose_version: String, /// The host triple (arch-platform-OS), this comes from verbose_version. @@ -28,6 +29,56 @@ pub struct Rustc { cache: Mutex, } +/// Information on the `rustc` wrapper +#[derive(Debug)] +pub struct RustcWrapper { + path: PathBuf, + env: HashMap>, +} + +impl RustcWrapper { + pub fn new>(path: T) -> RustcWrapper { + RustcWrapper { + path: path.into(), + env: HashMap::new(), + } + } + + /// (chainable) Sets an environment variable for the process. + pub fn env>(&mut self, key: &str, val: T) -> &mut RustcWrapper { + self.env + .insert(key.to_string(), Some(val.as_ref().to_os_string())); + self + } + + /// (chainable) Unsets an environment variable for the process. + pub fn env_remove(&mut self, key: &str) -> &mut RustcWrapper { + self.env.insert(key.to_string(), None); + self + } + + pub fn process(&self) -> ProcessBuilder { + let mut cmd = util::process(&self.path); + + for (k, v) in &self.env { + match *v { + Some(ref v) => { + cmd.env(k, v); + } + None => { + cmd.env_remove(k); + } + } + } + + cmd + } + + pub fn is_empty(&self) -> bool { + self.path.as_os_str().is_empty() + } +} + impl Rustc { /// Runs the compiler at `path` to learn various pieces of information about /// it, with an optional wrapper. @@ -59,7 +110,7 @@ impl Rustc { Ok(Rustc { path, - wrapper, + wrapper: wrapper.map(|w| RustcWrapper::new(w)), verbose_version, host, cache: Mutex::new(cache), @@ -69,8 +120,8 @@ impl Rustc { /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. pub fn process(&self) -> ProcessBuilder { match self.wrapper { - Some(ref wrapper) if !wrapper.as_os_str().is_empty() => { - let mut cmd = util::process(wrapper); + Some(ref wrapper) if !wrapper.is_empty() => { + let mut cmd = wrapper.process(); cmd.arg(&self.path); cmd } @@ -89,6 +140,10 @@ impl Rustc { pub fn cached_success(&self, cmd: &ProcessBuilder) -> CargoResult { self.cache.lock().unwrap().cached_success(cmd) } + + pub fn push_wrapper(&mut self, wrapper: RustcWrapper) { + self.wrapper = Some(wrapper); + } } /// It is a well known that `rustc` is not the fastest compiler in the world. From 116c05306684072baf919981fddbc9ccf74064e0 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Thu, 21 Mar 2019 16:06:06 -0700 Subject: [PATCH 08/13] totally unnecessary template type because its fun --- src/cargo/util/rustc.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 88699feaddd..ec275fdfcf8 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -110,7 +110,7 @@ impl Rustc { Ok(Rustc { path, - wrapper: wrapper.map(|w| RustcWrapper::new(w)), + wrapper: wrapper.map(RustcWrapper::new), verbose_version, host, cache: Mutex::new(cache), @@ -141,8 +141,8 @@ impl Rustc { self.cache.lock().unwrap().cached_success(cmd) } - pub fn push_wrapper(&mut self, wrapper: RustcWrapper) { - self.wrapper = Some(wrapper); + pub fn push_wrapper>>(&mut self, wrapper: T) { + self.wrapper = wrapper.into(); } } From d2ab686f3d161d8545f7d7b53144d9cdc3f27eb3 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Sun, 24 Mar 2019 09:09:12 -0700 Subject: [PATCH 09/13] fix not passing tests --- src/cargo/core/compiler/build_context/mod.rs | 10 +++++----- src/cargo/core/compiler/compilation.rs | 15 ++++++++++++++- src/cargo/util/config.rs | 2 +- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index d28386f4f6f..8632b50dc8e 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -69,11 +69,11 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { } })?; rustc.push_wrapper(RustcWrapper::new(tool)); - } else if build_config.cargo_as_rustc_wrapper { - let mut wrapper = RustcWrapper::new(env::current_exe()?); - let prog = rustc.path.as_os_str().to_owned(); - wrapper.env("RUSTC", prog); - rustc.push_wrapper(wrapper); + // } else if build_config.cargo_as_rustc_wrapper { + // let mut wrapper = RustcWrapper::new(env::current_exe()?); + // let prog = dbg!(rustc.path.as_os_str().to_owned()); + // wrapper.env("RUSTC", prog); + // rustc.push_wrapper(wrapper); } let host_config = TargetConfig::new(config, &rustc.host)?; diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 5313a3b10a6..354d6b568f4 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -75,7 +75,20 @@ pub struct Compilation<'cfg> { impl<'cfg> Compilation<'cfg> { pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult> { - let mut rustc = bcx.rustc.process(); + // If we're using cargo as a rustc wrapper then we're in a situation + // like `cargo fix`. For now just disregard the `RUSTC_WRAPPER` env var + // (which is typically set to `sccache` for now). Eventually we'll + // probably want to implement `RUSTC_WRAPPER` for `cargo fix`, but we'll + // leave that open as a bug for now. + let mut rustc = if bcx.build_config.cargo_as_rustc_wrapper { + let mut rustc = bcx.rustc.process_no_wrapper(); + let prog = rustc.get_program().to_owned(); + rustc.env("RUSTC", prog); + rustc.program(env::current_exe()?); + rustc + } else { + bcx.rustc.process() + }; if bcx.config.extra_verbose() { rustc.display_env_vars(); diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 46996ef724d..5072b8c199f 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -197,7 +197,7 @@ impl Config { .join(".rustc_info.json") .into_path_unlocked() }); - let wrapper = self.maybe_get_tool("rustc-wrapper")?; + let wrapper = self.maybe_get_tool("rustc_wrapper")?; Rustc::new( self.get_tool("rustc")?, wrapper, From bccc0ee4e361f5ef34357097f7c21decd4c983bf Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Sun, 24 Mar 2019 09:29:57 -0700 Subject: [PATCH 10/13] actually fix it so cargo fix kinda works with new wrapper stuff --- src/cargo/core/compiler/build_context/mod.rs | 13 ++++++++----- src/cargo/core/compiler/compilation.rs | 18 +----------------- src/cargo/ops/fix.rs | 2 +- 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 8632b50dc8e..5a7391bb5d8 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -69,11 +69,14 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { } })?; rustc.push_wrapper(RustcWrapper::new(tool)); - // } else if build_config.cargo_as_rustc_wrapper { - // let mut wrapper = RustcWrapper::new(env::current_exe()?); - // let prog = dbg!(rustc.path.as_os_str().to_owned()); - // wrapper.env("RUSTC", prog); - // rustc.push_wrapper(wrapper); + } else if build_config.cargo_as_rustc_wrapper { + let mut wrapper = RustcWrapper::new(env::current_exe()?); + let prog = rustc.path.as_os_str().to_owned(); + wrapper.env("RUSTC", prog); + for (k, v) in build_config.extra_rustc_env.iter() { + wrapper.env(k, v); + } + rustc.push_wrapper(wrapper); } let host_config = TargetConfig::new(config, &rustc.host)?; diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 354d6b568f4..0f162b72bc0 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -75,27 +75,11 @@ pub struct Compilation<'cfg> { impl<'cfg> Compilation<'cfg> { pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult> { - // If we're using cargo as a rustc wrapper then we're in a situation - // like `cargo fix`. For now just disregard the `RUSTC_WRAPPER` env var - // (which is typically set to `sccache` for now). Eventually we'll - // probably want to implement `RUSTC_WRAPPER` for `cargo fix`, but we'll - // leave that open as a bug for now. - let mut rustc = if bcx.build_config.cargo_as_rustc_wrapper { - let mut rustc = bcx.rustc.process_no_wrapper(); - let prog = rustc.get_program().to_owned(); - rustc.env("RUSTC", prog); - rustc.program(env::current_exe()?); - rustc - } else { - bcx.rustc.process() - }; + let mut rustc = bcx.rustc.process(); if bcx.config.extra_verbose() { rustc.display_env_vars(); } - for (k, v) in bcx.build_config.extra_rustc_env.iter() { - rustc.env(k, v); - } for arg in bcx.build_config.extra_rustc_args.iter() { rustc.arg(arg); } diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 0d2bfbcee3d..9d0f67cff16 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -593,7 +593,7 @@ impl Default for PrepareFor { impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); - for arg in env::args_os().skip(1) { + for arg in env::args_os().skip(2) { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { ret.file = Some(path); From 47c82f05c96fdd65b0cb12ccadea8e34e8567155 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Sun, 24 Mar 2019 09:44:22 -0700 Subject: [PATCH 11/13] remove unnecessary rustc env for cargo fix --- src/cargo/core/compiler/build_context/mod.rs | 2 -- src/cargo/ops/fix.rs | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 5a7391bb5d8..aca9a8ad25e 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -71,8 +71,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { rustc.push_wrapper(RustcWrapper::new(tool)); } else if build_config.cargo_as_rustc_wrapper { let mut wrapper = RustcWrapper::new(env::current_exe()?); - let prog = rustc.path.as_os_str().to_owned(); - wrapper.env("RUSTC", prog); for (k, v) in build_config.extra_rustc_env.iter() { wrapper.env(k, v); } diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 9d0f67cff16..6d8a4c78fc2 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -207,7 +207,7 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { let args = FixArgs::get(); trace!("cargo-fix as rustc got file {:?}", args.file); - let rustc = env::var_os("RUSTC").expect("failed to find RUSTC env var"); + let rustc = args.rustc.as_ref().expect("fix wrapper rustc was not set"); // Our goal is to fix only the crates that the end user is interested in. // That's very likely to only mean the crates in the workspace the user is @@ -576,6 +576,7 @@ struct FixArgs { enabled_edition: Option, other: Vec, primary_package: bool, + rustc: Option, } enum PrepareFor { @@ -593,6 +594,7 @@ impl Default for PrepareFor { impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); + ret.rustc = env::args_os().nth(1).map(PathBuf::from); for arg in env::args_os().skip(2) { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { From a23f8115c5a2882e43382ed0ab0ae13b64bf8635 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 29 Mar 2019 17:39:24 -0700 Subject: [PATCH 12/13] fix comments --- src/cargo/core/compiler/build_context/mod.rs | 25 +++----- src/cargo/core/compiler/compilation.rs | 3 - src/cargo/util/rustc.rs | 63 ++------------------ 3 files changed, 13 insertions(+), 78 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index aca9a8ad25e..d72cf454519 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -8,9 +8,8 @@ use log::debug; use crate::core::profiles::Profiles; use crate::core::{Dependency, Workspace}; use crate::core::{PackageId, PackageSet, Resolve}; +use crate::util; use crate::util::errors::CargoResult; -use crate::util::paths; -use crate::util::rustc::RustcWrapper; use crate::util::{profile, Cfg, CfgExpr, Config, Rustc}; use super::{BuildConfig, BuildOutput, Kind, Unit}; @@ -55,26 +54,16 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { let mut rustc = config.load_global_rustc(Some(ws))?; if build_config.clippy_override { - let tool = config.get_tool("clippy-driver")?; - let tool = paths::resolve_executable(&tool).map_err(|e| { - let rustup_in_path = config - .get_tool("rustup") - .and_then(|tool| paths::resolve_executable(&tool)) - .is_ok(); - let has_rustup_env = std::env::var("RUSTUP_TOOLCHAIN").is_ok(); - if rustup_in_path || has_rustup_env { - failure::format_err!("{}: please run `rustup component add clippy`", e) - } else { - failure::format_err!("{}: please install clippy", e) - } - })?; - rustc.push_wrapper(RustcWrapper::new(tool)); + rustc.set_wrapper(util::process("clippy-driver")); } else if build_config.cargo_as_rustc_wrapper { - let mut wrapper = RustcWrapper::new(env::current_exe()?); + let mut wrapper = util::process(env::current_exe()?); for (k, v) in build_config.extra_rustc_env.iter() { wrapper.env(k, v); } - rustc.push_wrapper(wrapper); + for arg in build_config.extra_rustc_args.iter() { + wrapper.arg(arg); + } + rustc.set_wrapper(wrapper); } let host_config = TargetConfig::new(config, &rustc.host)?; diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 0f162b72bc0..13d5d96be57 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -80,9 +80,6 @@ impl<'cfg> Compilation<'cfg> { if bcx.config.extra_verbose() { rustc.display_env_vars(); } - for arg in bcx.build_config.extra_rustc_args.iter() { - rustc.arg(arg); - } let srv = bcx.build_config.rustfix_diagnostic_server.borrow(); if let Some(server) = &*srv { server.configure(&mut rustc); diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index ec275fdfcf8..3a3451317ef 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -2,7 +2,6 @@ use std::collections::hash_map::{Entry, HashMap}; use std::env; -use std::ffi::{OsStr, OsString}; use std::hash::{Hash, Hasher, SipHasher}; use std::path::{Path, PathBuf}; use std::process::Stdio; @@ -21,7 +20,7 @@ pub struct Rustc { pub path: PathBuf, /// An optional program that will be passed the path of the rust exe as its first argument, and /// rustc args following this. - pub wrapper: Option, + pub wrapper: Option, /// Verbose version information (the output of `rustc -vV`) pub verbose_version: String, /// The host triple (arch-platform-OS), this comes from verbose_version. @@ -29,56 +28,6 @@ pub struct Rustc { cache: Mutex, } -/// Information on the `rustc` wrapper -#[derive(Debug)] -pub struct RustcWrapper { - path: PathBuf, - env: HashMap>, -} - -impl RustcWrapper { - pub fn new>(path: T) -> RustcWrapper { - RustcWrapper { - path: path.into(), - env: HashMap::new(), - } - } - - /// (chainable) Sets an environment variable for the process. - pub fn env>(&mut self, key: &str, val: T) -> &mut RustcWrapper { - self.env - .insert(key.to_string(), Some(val.as_ref().to_os_string())); - self - } - - /// (chainable) Unsets an environment variable for the process. - pub fn env_remove(&mut self, key: &str) -> &mut RustcWrapper { - self.env.insert(key.to_string(), None); - self - } - - pub fn process(&self) -> ProcessBuilder { - let mut cmd = util::process(&self.path); - - for (k, v) in &self.env { - match *v { - Some(ref v) => { - cmd.env(k, v); - } - None => { - cmd.env_remove(k); - } - } - } - - cmd - } - - pub fn is_empty(&self) -> bool { - self.path.as_os_str().is_empty() - } -} - impl Rustc { /// Runs the compiler at `path` to learn various pieces of information about /// it, with an optional wrapper. @@ -110,7 +59,7 @@ impl Rustc { Ok(Rustc { path, - wrapper: wrapper.map(RustcWrapper::new), + wrapper: wrapper.map(util::process), verbose_version, host, cache: Mutex::new(cache), @@ -120,8 +69,8 @@ impl Rustc { /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. pub fn process(&self) -> ProcessBuilder { match self.wrapper { - Some(ref wrapper) if !wrapper.is_empty() => { - let mut cmd = wrapper.process(); + Some(ref wrapper) if !wrapper.get_program().is_empty() => { + let mut cmd = wrapper.clone(); cmd.arg(&self.path); cmd } @@ -141,8 +90,8 @@ impl Rustc { self.cache.lock().unwrap().cached_success(cmd) } - pub fn push_wrapper>>(&mut self, wrapper: T) { - self.wrapper = wrapper.into(); + pub fn set_wrapper(&mut self, wrapper: ProcessBuilder) { + self.wrapper = Some(wrapper); } } From 842da62afb5a212be0b8cf6545ba86bb94758676 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Apr 2019 10:22:23 -0700 Subject: [PATCH 13/13] Remove no longer needed fields in `BuildConfig` Now the fields are just all represented as `rustc_wrapper` which internally contains all process configuration that's later passed down to `Rustc` as necessary. --- src/bin/cargo/commands/clippy.rs | 5 ++- src/cargo/core/compiler/build_config.rs | 21 +++---------- src/cargo/core/compiler/build_context/mod.rs | 15 ++------- src/cargo/ops/fix.rs | 33 ++++++-------------- 4 files changed, 19 insertions(+), 55 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index a821606eaa7..44054ae9c37 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -1,6 +1,7 @@ use crate::command_prelude::*; use cargo::ops; +use cargo::util; pub fn cli() -> App { subcommand("clippy-preview") @@ -60,7 +61,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { let mode = CompileMode::Check { test: false }; let mut compile_opts = args.compile_options(config, mode, Some(&ws))?; - compile_opts.build_config.set_clippy_override(true); if !config.cli_unstable().unstable_options { return Err(failure::format_err!( @@ -69,6 +69,9 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { .into()); } + let wrapper = util::process("clippy-driver"); + compile_opts.build_config.rustc_wrapper = Some(wrapper); + ops::compile(&ws, &compile_opts)?; Ok(()) } diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 04997ffcaf1..2056408097e 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -3,6 +3,7 @@ use std::path::Path; use serde::ser; +use crate::util::ProcessBuilder; use crate::util::{CargoResult, CargoResultExt, Config, RustfixDiagnosticServer}; /// Configuration information for a rustc build. @@ -23,15 +24,9 @@ pub struct BuildConfig { pub force_rebuild: bool, /// Output a build plan to stdout instead of actually compiling. pub build_plan: bool, - /// Use Cargo itself as the wrapper around rustc, only used for `cargo fix`. - pub cargo_as_rustc_wrapper: bool, - /// Extra env vars to inject into rustc commands. - pub extra_rustc_env: Vec<(String, String)>, - /// Extra args to inject into rustc commands. - pub extra_rustc_args: Vec, + /// An optional wrapper, if any, used to wrap rustc invocations + pub rustc_wrapper: Option, pub rustfix_diagnostic_server: RefCell>, - /// Use `clippy-driver` instead of `rustc` - pub clippy_override: bool, } impl BuildConfig { @@ -100,11 +95,8 @@ impl BuildConfig { message_format: MessageFormat::Human, force_rebuild: false, build_plan: false, - cargo_as_rustc_wrapper: false, - extra_rustc_env: Vec::new(), - extra_rustc_args: Vec::new(), + rustc_wrapper: None, rustfix_diagnostic_server: RefCell::new(None), - clippy_override: false, }) } @@ -115,11 +107,6 @@ impl BuildConfig { pub fn test(&self) -> bool { self.mode == CompileMode::Test || self.mode == CompileMode::Bench } - - /// Sets the clippy override. If this is true, clippy-driver is invoked instead of rustc. - pub fn set_clippy_override(&mut self, val: bool) { - self.clippy_override = val; - } } #[derive(Clone, Copy, Debug, PartialEq, Eq)] diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index d72cf454519..e216bfa800d 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -8,7 +8,6 @@ use log::debug; use crate::core::profiles::Profiles; use crate::core::{Dependency, Workspace}; use crate::core::{PackageId, PackageSet, Resolve}; -use crate::util; use crate::util::errors::CargoResult; use crate::util::{profile, Cfg, CfgExpr, Config, Rustc}; @@ -52,18 +51,8 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { extra_compiler_args: HashMap, Vec>, ) -> CargoResult> { let mut rustc = config.load_global_rustc(Some(ws))?; - - if build_config.clippy_override { - rustc.set_wrapper(util::process("clippy-driver")); - } else if build_config.cargo_as_rustc_wrapper { - let mut wrapper = util::process(env::current_exe()?); - for (k, v) in build_config.extra_rustc_env.iter() { - wrapper.env(k, v); - } - for arg in build_config.extra_rustc_args.iter() { - wrapper.arg(arg); - } - rustc.set_wrapper(wrapper); + if let Some(wrapper) = &build_config.rustc_wrapper { + rustc.set_wrapper(wrapper.clone()); } let host_config = TargetConfig::new(config, &rustc.host)?; diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 6d8a4c78fc2..76452630884 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -56,7 +56,7 @@ use crate::core::Workspace; use crate::ops::{self, CompileOptions}; use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer}; use crate::util::errors::CargoResult; -use crate::util::paths; +use crate::util::{self, paths}; use crate::util::{existing_vcs_repo, LockServer, LockServerClient}; const FIX_ENV: &str = "__CARGO_FIX_PLZ"; @@ -81,46 +81,31 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { // Spin up our lock server, which our subprocesses will use to synchronize fixes. let lock_server = LockServer::new()?; - opts.compile_opts - .build_config - .extra_rustc_env - .push((FIX_ENV.to_string(), lock_server.addr().to_string())); + let mut wrapper = util::process(env::current_exe()?); + wrapper.env(FIX_ENV, lock_server.addr().to_string()); let _started = lock_server.start()?; opts.compile_opts.build_config.force_rebuild = true; if opts.broken_code { - let key = BROKEN_CODE_ENV.to_string(); - opts.compile_opts - .build_config - .extra_rustc_env - .push((key, "1".to_string())); + wrapper.env(BROKEN_CODE_ENV, "1"); } if opts.edition { - let key = EDITION_ENV.to_string(); - opts.compile_opts - .build_config - .extra_rustc_env - .push((key, "1".to_string())); + wrapper.env(EDITION_ENV, "1"); } else if let Some(edition) = opts.prepare_for { - opts.compile_opts - .build_config - .extra_rustc_env - .push((PREPARE_FOR_ENV.to_string(), edition.to_string())); + wrapper.env(PREPARE_FOR_ENV, edition); } if opts.idioms { - opts.compile_opts - .build_config - .extra_rustc_env - .push((IDIOMS_ENV.to_string(), "1".to_string())); + wrapper.env(IDIOMS_ENV, "1"); } - opts.compile_opts.build_config.cargo_as_rustc_wrapper = true; + *opts .compile_opts .build_config .rustfix_diagnostic_server .borrow_mut() = Some(RustfixDiagnosticServer::new()?); + opts.compile_opts.build_config.rustc_wrapper = Some(wrapper); ops::compile(ws, &opts.compile_opts)?; Ok(())