Emit errors with cargo:error= #11312
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -16,7 +17,7 @@ use std::path::{Path, PathBuf}; | |
| use std::str; | ||
| use std::sync::{Arc, Mutex}; | ||
|
|
||
| const CARGO_LINE_PREFIX: &str = "cargo:"; | ||
|
|
||
| /// 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<PathBuf>, | ||
| /// Environment variables which, when changed, will cause a rebuild. | ||
| pub rerun_if_env_changed: Vec<String>, | ||
| /// Warnings and errors generated by this build. | ||
| /// | ||
| /// 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. | ||
| /// | ||
| /// Fatal errors are treated as the root cause of the build script failure, | ||
| /// and suppress other output unless `-vv` is used. | ||
| pub diagnostics: Vec<Diagnostic>, | ||
| } | ||
|
|
||
| /// Warnings or errors in `BuildOutput` | ||
| #[derive(Clone, Debug, Hash)] | ||
| pub enum Diagnostic { | ||
| Warning(String), | ||
| Error(String), | ||
| } | ||
|
|
||
| /// Map of packages to build script output. | ||
|
|
@@ -299,6 +310,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> { | |
| }) | ||
| .collect::<Vec<_>>(); | ||
| 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(); | ||
|
|
@@ -385,39 +397,82 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> { | |
| state.running(&cmd); | ||
| let timestamp = paths::set_invocation_time(&script_run_dir)?; | ||
| let prefix = format!("[{} {}] ", id.name(), id.version()); | ||
| let mut diagnostics = Vec::new(); | ||
| let script_result = cmd.exec_with_streaming( | ||
| &mut |stdout| { | ||
| 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); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| 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 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"), | ||
| }; | ||
|
Comment on lines
+453
to
+462
There was a problem hiding this comment. Should we be covering more of these cases in tests? |
||
|
|
||
| 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 | ||
|
|
@@ -500,10 +555,10 @@ fn insert_warnings_in_build_outputs( | |
| build_script_outputs: Arc<Mutex<BuildScriptOutputs>>, | ||
| id: PackageId, | ||
| metadata_hash: Metadata, | ||
| diagnostics: Vec<Diagnostic>, | ||
| ) { | ||
| let build_output_with_only_warnings = BuildOutput { | ||
| diagnostics, | ||
| ..BuildOutput::default() | ||
| }; | ||
| build_script_outputs | ||
|
|
@@ -559,7 +614,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 diagnostics = Vec::new(); | ||
| let whence = format!("build script of `{}`", pkg_descr); | ||
|
|
||
| for line in input.split(|b| *b == b'\n') { | ||
|
|
@@ -624,16 +679,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()) { | ||
| 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 \ | ||
| <https://github.com/rust-lang/cargo/issues/9562>.", | ||
| ))); | ||
| } | ||
| linker_args.push((LinkType::Cdylib, value)) | ||
| } | ||
|
|
@@ -685,7 +739,9 @@ impl BuildOutput { | |
| if extra_check_cfg { | ||
| check_cfgs.push(value.to_string()); | ||
| } else { | ||
| diagnostics.push(Diagnostic::Warning(format!( | ||
| "cargo:{key} requires -Zcheck-cfg=output flag", | ||
| ))); | ||
| } | ||
| } | ||
| "rustc-env" => { | ||
|
|
@@ -716,10 +772,9 @@ impl BuildOutput { | |
| if nightly_features_allowed | ||
| || rustc_bootstrap_allows(library_name.as_deref()) | ||
| { | ||
| 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.", | ||
| ))); | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } else { | ||
| // Setting RUSTC_BOOTSTRAP would change the behavior of the crate. | ||
| // Abort with an error. | ||
|
|
@@ -735,7 +790,14 @@ impl BuildOutput { | |
| env.push((key, val)); | ||
| } | ||
| } | ||
| // "error" is not parsed here for backwards compatibility, and because this function is for successful outputs | ||
|
There was a problem hiding this comment.
This is referring to
See https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script That is a bit of a compatibility trap as theoretically any new directive is a breaking change. The previous direction from the cargo team was
I feel like not doing this is a behavior trap for users. I would recommend re-raising this in the Issue and for us to come up with a compatible solution that allows erroring. There was a problem hiding this comment. I've elaborated on the design here: #10159 (comment) |
||
| "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())), | ||
|
|
@@ -752,7 +814,7 @@ impl BuildOutput { | |
| metadata, | ||
| rerun_if_changed, | ||
| rerun_if_env_changed, | ||
| diagnostics, | ||
| }) | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,14 +38,60 @@ 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] could not build `foo` due to a custom build script failure | ||
|
|
||
| Caused by: | ||
| process didn't exit successfully: `[..]/build-script-build` (exit [..]: 101)", | ||
| ) | ||
| .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 = ["[email protected]"] | ||
| build = "build.rs" | ||
| "#, | ||
| ) | ||
| .file("src/main.rs", "fn main() {}") | ||
| .file( | ||
| "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); | ||
| }"#, | ||
| ) | ||
| .build(); | ||
| p.cargo("build") | ||
| .with_status(101) | ||
| .with_stderr( | ||
| "\ | ||
| [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; 1 warning emitted | ||
| [NOTE] for more details, run again with '--verbose'", | ||
| ) | ||
| .run(); | ||
| } | ||
|
|
||
| #[cargo_test] | ||
| fn custom_build_env_vars() { | ||
| let p = project() | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split this out into a separate Issue/PR so we can make sure we are agreed on the semantics we want for this before going to the review stage and so we don't block your other work on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean just the
+=syntax?I've considered alternatives:
do nothing. Authors of crates like openssl-sys have a lot of explaining to do, so they will either have to cram everything into a one long line, or they will emit lots of lines with
cargo:error=orcargo:warning=. Long lines are unclear, and repeatedcargo:warning=directives give noisy formatting that is inconsistent with rustc error output.support line breaks with a character sequence inside a one-liner message. No obvious sequence comes to my mind. Something recognizable like
\nwould be ambiguous with line breaks in string literals and require double-escapingprintln!("\\n")and escaping of backslashes inside the error message. It's a hassle.there could be some other line prefix syntax for "append to previous line" instead of
error+=, e.g. start next line with a tab character (like HTTP/1.1 headers)"\tsecond line"or some other syntax like"...second line", but keepingcargo:prefix seems safe, and=vs+=should be intuitive to Rust programmers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
+=syntax. While discussing alternatives can be interesting, again I feel its best that we have a dedicated discussion on this, so please split this out. This is not strictly required for merging error messages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed in the cargo team meeting. We came up with several concerns and it would be best to have a dedicated Issue for finishing the conversation on them.