From bebeca3ea850a31420dfaabf073445101a4e3fc9 Mon Sep 17 00:00:00 2001 From: Kornel Date: Sun, 30 Oct 2022 16:45:34 +0000 Subject: [PATCH 1/4] Emit errors with cargo:error= --- src/cargo/core/compiler/custom_build.rs | 57 ++++++++++++++----------- src/cargo/core/compiler/job_queue.rs | 30 ++++--------- tests/testsuite/build_script.rs | 42 ++++++++++++++++++ 3 files changed, 84 insertions(+), 45 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index b3929b8df70..8204afe8abf 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -17,6 +17,7 @@ use std::str; use std::sync::{Arc, Mutex}; const CARGO_WARNING: &str = "cargo:warning="; +const CARGO_ERROR: &str = "cargo:error="; /// Contains the parsed output of a custom build script. #[derive(Clone, Debug, Hash, Default)] @@ -40,11 +41,21 @@ pub struct BuildOutput { pub rerun_if_changed: Vec, /// Environment variables which, when changed, will cause a rebuild. pub rerun_if_env_changed: Vec, - /// Warnings generated by this build. + /// Warnings and errors generated by this build. /// - /// These are only displayed if this is a "local" package, `-vv` is used, + /// Warnings are only displayed if this is a "local" package, `-vv` is used, /// or there is a build error for any target in this package. - pub warnings: Vec, + /// + /// Fatal errors are treated as the root cause of the build script failure, + /// and suppress other output unless `-vv` is used. + pub diagnostics: Vec, +} + +/// Warnings or errors in `BuildOutput` +#[derive(Clone, Debug, Hash)] +pub enum Diagnostic { + Warning(String), + Error(String), } /// Map of packages to build script output. @@ -385,12 +396,14 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { state.running(&cmd); let timestamp = paths::set_invocation_time(&script_run_dir)?; let prefix = format!("[{} {}] ", id.name(), id.version()); - let mut warnings_in_case_of_panic = Vec::new(); + let mut diagnostics = Vec::new(); let output = cmd .exec_with_streaming( &mut |stdout| { if let Some(warning) = stdout.strip_prefix(CARGO_WARNING) { - warnings_in_case_of_panic.push(warning.to_owned()); + diagnostics.push(Diagnostic::Warning(warning.to_owned())); + } else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) { + diagnostics.push(Diagnostic::Error(error.to_owned())); } if extra_verbose { state.stdout(format!("{}{}", prefix, stdout))?; @@ -408,12 +421,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { .with_context(|| format!("failed to run custom build command for `{}`", pkg_descr)); if let Err(error) = output { - insert_warnings_in_build_outputs( - build_script_outputs, - id, - metadata_hash, - warnings_in_case_of_panic, - ); + insert_warnings_in_build_outputs(build_script_outputs, id, metadata_hash, diagnostics); return Err(error); } @@ -500,10 +508,10 @@ fn insert_warnings_in_build_outputs( build_script_outputs: Arc>, id: PackageId, metadata_hash: Metadata, - warnings: Vec, + diagnostics: Vec, ) { let build_output_with_only_warnings = BuildOutput { - warnings, + diagnostics, ..BuildOutput::default() }; build_script_outputs @@ -559,7 +567,7 @@ impl BuildOutput { let mut metadata = Vec::new(); let mut rerun_if_changed = Vec::new(); let mut rerun_if_env_changed = Vec::new(); - let mut warnings = Vec::new(); + let mut diagnostics = Vec::new(); let whence = format!("build script of `{}`", pkg_descr); for line in input.split(|b| *b == b'\n') { @@ -624,16 +632,15 @@ impl BuildOutput { "rustc-link-search" => library_paths.push(PathBuf::from(value)), "rustc-link-arg-cdylib" | "rustc-cdylib-link-arg" => { if !targets.iter().any(|target| target.is_cdylib()) { - warnings.push(format!( - "cargo:{} was specified in the build script of {}, \ + diagnostics.push(Diagnostic::Warning(format!( + "cargo:{key} was specified in the build script of {pkg_descr}, \ but that package does not contain a cdylib target\n\ \n\ Allowing this was an unintended change in the 1.50 \ release, and may become an error in the future. \ For more information, see \ .", - key, pkg_descr - )); + ))); } linker_args.push((LinkType::Cdylib, value)) } @@ -685,7 +692,9 @@ impl BuildOutput { if extra_check_cfg { check_cfgs.push(value.to_string()); } else { - warnings.push(format!("cargo:{} requires -Zcheck-cfg=output flag", key)); + diagnostics.push(Diagnostic::Warning(format!( + "cargo:{key} requires -Zcheck-cfg=output flag", + ))); } } "rustc-env" => { @@ -716,10 +725,9 @@ impl BuildOutput { if nightly_features_allowed || rustc_bootstrap_allows(library_name.as_deref()) { - warnings.push(format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\n\ + diagnostics.push(Diagnostic::Warning(format!("Cannot set `RUSTC_BOOTSTRAP={val}` from {whence}.\n\ note: Crates cannot set `RUSTC_BOOTSTRAP` themselves, as doing so would subvert the stability guarantees of Rust for your project.", - val, whence - )); + ))); } else { // Setting RUSTC_BOOTSTRAP would change the behavior of the crate. // Abort with an error. @@ -735,7 +743,8 @@ impl BuildOutput { env.push((key, val)); } } - "warning" => warnings.push(value.to_string()), + // "error" is not parsed here for backwards compatibility, and because this function is for successful outputs + "warning" => diagnostics.push(Diagnostic::Warning(value.to_string())), "rerun-if-changed" => rerun_if_changed.push(PathBuf::from(value)), "rerun-if-env-changed" => rerun_if_env_changed.push(value.to_string()), _ => metadata.push((key.to_string(), value.to_string())), @@ -752,7 +761,7 @@ impl BuildOutput { metadata, rerun_if_changed, rerun_if_env_changed, - warnings, + diagnostics, }) } diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index bd3a859f04d..75a7339f492 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -65,6 +65,7 @@ use log::{debug, trace}; use semver::Version; use super::context::OutputFile; +use super::custom_build::Diagnostic; use super::job::{ Freshness::{self, Dirty, Fresh}, Job, @@ -783,8 +784,7 @@ impl<'cfg> DrainState<'cfg> { match result { Ok(()) => self.finish(id, &unit, artifact, cx)?, Err(error) => { - let msg = "The following warnings were emitted during compilation:"; - self.emit_warnings(Some(msg), &unit, cx)?; + self.emit_diagnostics(&unit, cx)?; self.back_compat_notice(cx, &unit)?; return Err(ErrorToHandle { error, @@ -1153,12 +1153,8 @@ impl<'cfg> DrainState<'cfg> { } } - fn emit_warnings( - &mut self, - msg: Option<&str>, - unit: &Unit, - cx: &mut Context<'_, '_>, - ) -> CargoResult<()> { + /// Print warnings and errors + fn emit_diagnostics(&mut self, unit: &Unit, cx: &mut Context<'_, '_>) -> CargoResult<()> { let outputs = cx.build_script_outputs.lock().unwrap(); let metadata = match cx.find_build_script_metadata(unit) { Some(metadata) => metadata, @@ -1166,18 +1162,10 @@ impl<'cfg> DrainState<'cfg> { }; let bcx = &mut cx.bcx; if let Some(output) = outputs.get(metadata) { - if !output.warnings.is_empty() { - if let Some(msg) = msg { - writeln!(bcx.config.shell().err(), "{}\n", msg)?; - } - - for warning in output.warnings.iter() { - bcx.config.shell().warn(warning)?; - } - - if msg.is_some() { - // Output an empty line. - writeln!(bcx.config.shell().err())?; + for diag in output.diagnostics.iter() { + match diag { + Diagnostic::Warning(warning) => bcx.config.shell().warn(warning)?, + Diagnostic::Error(error) => bcx.config.shell().error(error)?, } } } @@ -1278,7 +1266,7 @@ impl<'cfg> DrainState<'cfg> { cx: &mut Context<'_, '_>, ) -> CargoResult<()> { if unit.mode.is_run_custom_build() && unit.show_warnings(cx.bcx.config) { - self.emit_warnings(None, unit, cx)?; + self.emit_diagnostics(unit, cx)?; } let unlocked = self.queue.finish(unit, &artifact); match artifact { diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index e81f6ef1ece..95df092fd85 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -46,6 +46,48 @@ Caused by: .run(); } +#[cargo_test] +fn custom_build_script_failed_custom_error() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + + name = "foo" + version = "0.5.0" + authors = ["wycats@example.com"] + build = "build.rs" + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "build.rs", + r#"fn main() { + println!("cargo:error=failed"); + println!("cargo:error=error2"); + std::process::exit(101); + }"#, + ) + .build(); + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +[COMPILING] foo v0.5.0 ([CWD]) +[ERROR] failed +[ERROR] error2 +[ERROR] failed to run custom build command for `foo v0.5.0 ([CWD])` + +Caused by: + process didn't exit successfully: `[..]/build-script-build` (exit [..]: 101) + --- stdout + cargo:error=failed + cargo:error=error2", + ) + .run(); +} + #[cargo_test] fn custom_build_env_vars() { let p = project() From 13f97d2e0ddbfbd972f2f7b9fdbfc5801f2d455d Mon Sep 17 00:00:00 2001 From: Kornel Date: Sun, 30 Oct 2022 21:29:49 +0000 Subject: [PATCH 2/4] Hint when errors are hidden --- src/cargo/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index f6aadcce94d..940f4cd1d2f 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -146,9 +146,8 @@ fn _display_error(err: &Error, shell: &mut Shell, as_err: bool) -> bool { for (i, err) in err.chain().enumerate() { // If we're not in verbose mode then only print cause chain until one // marked as `VerboseError` appears. - // - // Generally the top error shouldn't be verbose, but check it anyways. if shell.verbosity() != Verbose && err.is::() { + drop(shell.note("for more details, run again with '--verbose'")); return true; } if err.is::() { From db04f5f27064f0433db54883c0a5ed4506fb671a Mon Sep 17 00:00:00 2001 From: Kornel Date: Sun, 30 Oct 2022 20:00:10 +0000 Subject: [PATCH 3/4] Align build script message wording with other compilation errors --- src/cargo/core/compiler/custom_build.rs | 86 +++++++++++++++++-------- tests/testsuite/build_script.rs | 11 +--- 2 files changed, 61 insertions(+), 36 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 8204afe8abf..c339f280cc4 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -7,6 +7,7 @@ use crate::core::{profiles::ProfileRoot, PackageId, Target}; use crate::util::errors::CargoResult; use crate::util::machine_message::{self, Message}; use crate::util::{internal, profile}; +use crate::VerboseError; use anyhow::{bail, Context as _}; use cargo_platform::Cfg; use cargo_util::paths; @@ -310,6 +311,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { }) .collect::>(); let library_name = unit.pkg.library().map(|t| t.crate_name()); + let package_name = unit.pkg.name(); let pkg_descr = unit.pkg.to_string(); let build_script_outputs = Arc::clone(&cx.build_script_outputs); let id = unit.pkg.package_id(); @@ -397,35 +399,63 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { let timestamp = paths::set_invocation_time(&script_run_dir)?; let prefix = format!("[{} {}] ", id.name(), id.version()); let mut diagnostics = Vec::new(); - let output = cmd - .exec_with_streaming( - &mut |stdout| { - if let Some(warning) = stdout.strip_prefix(CARGO_WARNING) { - diagnostics.push(Diagnostic::Warning(warning.to_owned())); - } else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) { - diagnostics.push(Diagnostic::Error(error.to_owned())); - } - if extra_verbose { - state.stdout(format!("{}{}", prefix, stdout))?; - } - Ok(()) - }, - &mut |stderr| { - if extra_verbose { - state.stderr(format!("{}{}", prefix, stderr))?; - } - Ok(()) - }, - true, - ) - .with_context(|| format!("failed to run custom build command for `{}`", pkg_descr)); - - if let Err(error) = output { - insert_warnings_in_build_outputs(build_script_outputs, id, metadata_hash, diagnostics); - return Err(error); - } + let script_result = cmd.exec_with_streaming( + &mut |stdout| { + if let Some(warning) = stdout.strip_prefix(CARGO_WARNING) { + diagnostics.push(Diagnostic::Warning(warning.to_owned())); + } else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) { + diagnostics.push(Diagnostic::Error(error.to_owned())); + } + if extra_verbose { + state.stdout(format!("{}{}", prefix, stdout))?; + } + Ok(()) + }, + &mut |stderr| { + if extra_verbose { + state.stderr(format!("{}{}", prefix, stderr))?; + } + Ok(()) + }, + true, + ); + + let output = match script_result { + Ok(output) => output, + Err(mut error) => { + let errors_seen = diagnostics + .iter() + .filter(|diag| matches!(diag, Diagnostic::Error { .. })) + .count(); + let warnings_seen = diagnostics.len() - errors_seen; + if errors_seen > 0 { + // Hides the full stdout/stderr printout unless verbose flag is used + error = anyhow::Error::from(VerboseError::new(error)); + } - let output = output.unwrap(); + let errors = match errors_seen { + 0 => " due to a custom build script failure".to_string(), + 1 => " due to the previous error".to_string(), + count => format!(" due to {count} previous errors"), + }; + let warnings = match warnings_seen { + 0 => String::new(), + 1 => "; 1 warning emitted".to_string(), + count => format!("; {count} warnings emitted"), + }; + + insert_warnings_in_build_outputs( + build_script_outputs, + id, + metadata_hash, + diagnostics, + ); + + return Err(error.context(format!( + "could not build `{package_name}`{errors}{warnings}" + ))); + } + }; // After the build command has finished running, we need to be sure to // remember all of its output so we can later discover precisely what it diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 95df092fd85..84b3dab1d37 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -38,7 +38,7 @@ fn custom_build_script_failed() { [COMPILING] foo v0.5.0 ([CWD]) [RUNNING] `rustc --crate-name build_script_build build.rs [..]--crate-type bin [..]` [RUNNING] `[..]/build-script-build` -[ERROR] failed to run custom build command for `foo v0.5.0 ([CWD])` +[ERROR] could not build `foo` due to a custom build script failure Caused by: process didn't exit successfully: `[..]/build-script-build` (exit [..]: 101)", @@ -77,13 +77,8 @@ fn custom_build_script_failed_custom_error() { [COMPILING] foo v0.5.0 ([CWD]) [ERROR] failed [ERROR] error2 -[ERROR] failed to run custom build command for `foo v0.5.0 ([CWD])` - -Caused by: - process didn't exit successfully: `[..]/build-script-build` (exit [..]: 101) - --- stdout - cargo:error=failed - cargo:error=error2", +[ERROR] could not build `foo` due to 2 previous errors +[NOTE] for more details, run again with '--verbose'", ) .run(); } From 2def6a9dd39e9ef6beb3673ced031aca0c702927 Mon Sep 17 00:00:00 2001 From: Kornel Date: Mon, 31 Oct 2022 01:17:16 +0000 Subject: [PATCH 4/4] cargo:warning+= syntax --- src/cargo/core/compiler/custom_build.rs | 35 ++++++++++++++++++++----- tests/testsuite/build_script.rs | 11 +++++++- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index c339f280cc4..0f94126a545 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -17,8 +17,7 @@ use std::path::{Path, PathBuf}; use std::str; use std::sync::{Arc, Mutex}; -const CARGO_WARNING: &str = "cargo:warning="; -const CARGO_ERROR: &str = "cargo:error="; +const CARGO_LINE_PREFIX: &str = "cargo:"; /// Contains the parsed output of a custom build script. #[derive(Clone, Debug, Hash, Default)] @@ -401,10 +400,28 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { let mut diagnostics = Vec::new(); let script_result = cmd.exec_with_streaming( &mut |stdout| { - if let Some(warning) = stdout.strip_prefix(CARGO_WARNING) { - diagnostics.push(Diagnostic::Warning(warning.to_owned())); - } else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) { - diagnostics.push(Diagnostic::Error(error.to_owned())); + let kv = stdout.strip_prefix(CARGO_LINE_PREFIX).and_then(|kv| { + let mut kv = kv.splitn(2, '='); + Some((kv.next()?, kv.next()?)) + }); + if let Some((key, value)) = kv { + match key { + "warning" => diagnostics.push(Diagnostic::Warning(value.to_owned())), + "error" => diagnostics.push(Diagnostic::Error(value.to_owned())), + "warning+" => { + if let Some(Diagnostic::Warning(msg)) = diagnostics.last_mut() { + msg.push_str("\n "); + msg.push_str(value); + } + } + "error+" => { + if let Some(Diagnostic::Error(msg)) = diagnostics.last_mut() { + msg.push_str("\n "); + msg.push_str(value); + } + } + _ => {} + } } if extra_verbose { state.stdout(format!("{}{}", prefix, stdout))?; @@ -775,6 +792,12 @@ impl BuildOutput { } // "error" is not parsed here for backwards compatibility, and because this function is for successful outputs "warning" => diagnostics.push(Diagnostic::Warning(value.to_string())), + "warning+" => { + if let Some(Diagnostic::Warning(msg)) = diagnostics.last_mut() { + msg.push_str("\n "); + msg.push_str(&value); + } + } "rerun-if-changed" => rerun_if_changed.push(PathBuf::from(value)), "rerun-if-env-changed" => rerun_if_env_changed.push(value.to_string()), _ => metadata.push((key.to_string(), value.to_string())), diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 84b3dab1d37..def59aefb72 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -65,7 +65,12 @@ fn custom_build_script_failed_custom_error() { "build.rs", r#"fn main() { println!("cargo:error=failed"); + println!("cargo:error+=because oops"); + println!("cargo:warning=multi"); + println!("cargo:warning+=line"); + println!("cargo:warning+=warning"); println!("cargo:error=error2"); + println!("cargo:warning+=this one has nothing to append to"); std::process::exit(101); }"#, ) @@ -76,8 +81,12 @@ fn custom_build_script_failed_custom_error() { "\ [COMPILING] foo v0.5.0 ([CWD]) [ERROR] failed + because oops +[WARNING] multi + line + warning [ERROR] error2 -[ERROR] could not build `foo` due to 2 previous errors +[ERROR] could not build `foo` due to 2 previous errors; 1 warning emitted [NOTE] for more details, run again with '--verbose'", ) .run();