Emit errors with cargo:error= #11312

kornelski · 2022-10-31T04:13:07Z

This adds support for cargo:error directive similar to cargo:warning to let custom build scripts fail in a clear way.

Presence of a cargo:error directive hides the full dump of stderr/stdout (unless --verbose is used). This is under assumption that scripts which opt-in to fail with cargo:error will make this message comprehensive, and any other output would be only a distraction.

I haven't made cargo:error fail the compilation, which remains dependent on the script's exit code. This way this feature is technically backwards-compatible with crates that might have used cargo:error as their custom key-value metadata.

To let crate authors to emit high-quality errors with explanations, I've added support for multi-line errors and warnings. The syntax for it is +=, which appends extra line to the preceding error/warning:

cargo:error=the error message
cargo:error+=additional lines
cargo:error+=for the error message

rustbot · 2022-10-31T04:13:11Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

src/cargo/core/compiler/custom_build.rs

epage · 2022-10-31T12:28:23Z

src/cargo/core/compiler/job_queue.rs

-                        let msg = "The following warnings were emitted during compilation:";
-                        self.emit_warnings(Some(msg), &unit, cx)?;


Note: this is a change of behavior

Is this necessary? AFAIK there's no such message for regular (rustc) compilation errors.

My intention wasn't to say "this is a bad change of behavior" but to just raise awareness of how the message is changing. We used to have an introductory statement in this case but will no longer. I felt this would be worth highlighting when this gets a wider view during FCP

epage · 2022-10-31T12:35:34Z

src/cargo/core/compiler/custom_build.rs

+                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"),
+                };


Should we be covering more of these cases in tests?

src/cargo/core/compiler/custom_build.rs

epage · 2022-10-31T12:38:26Z

src/cargo/core/compiler/custom_build.rs

+                        "warning" => diagnostics.push(Diagnostic::Warning(value.to_owned())),
+                        "error" => diagnostics.push(Diagnostic::Error(value.to_owned())),
+                        "warning+" => {


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?

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= or cargo:warning=. Long lines are unclear, and repeated cargo: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 \n would be ambiguous with line breaks in string literals and require double-escaping println!("\\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 keeping cargo: prefix seems safe, and = vs += should be intuitive to Rust programmers.

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.

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.

epage · 2022-10-31T12:44:06Z

src/cargo/core/compiler/custom_build.rs

+                    } else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) {
+                        diagnostics.push(Diagnostic::Error(error.to_owned()));


Please add a test for this

It is covered by custom_build_script_failed_custom_error test

That test is in the commit for += and will be split out of this PR.

I've moved part of the test to earlier commits

epage · 2022-10-31T12:50:02Z

src/cargo/core/compiler/custom_build.rs

@@ -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


I haven't made cargo:error fail the compilation, which remains dependent on the script's exit code. This way this feature is technically backwards-compatible with crates that might have used cargo:error as their custom key-value metadata.

This is referring to

cargo:KEY=VALUE — Metadata, used by links scripts.

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

Treat the script as having errored whether or not it returns an error code.

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.

I've elaborated on the design here: #10159 (comment)

kornelski · 2022-11-29T16:11:50Z

@epage @joshtriplett I'm waiting on the decisions:

is the cargo:warning+=second line syntax fine for multi-line warnings and errors? I don't have strong feelings about any particular syntax, but I think some support for multi-line errors is important for quality of the error messages. BTW, you may want to also consider Rerun-if-changed without disabling other heuristics as a side-effect #4587 needs some syntax for "add to existing set instead of replacing it", which could use += syntax too.
how to handle when build script prints cargo:error= and does not return a failure exit code. For this I suggest downgrading the error message to a warning, and emitting another warning that the build script behaved inconsistently. This way exit code remains the sole way of signaling build failure, and this change won't intertwine output parsing with control flow.

I think you should be able to discuss these issues without me changing the code back and forth, so I don't think they're blocked on the PR changes, but rather the PR is waiting for team's decisions.

epage · 2022-11-29T19:59:52Z

As I've previously said, we should decouple the += conversation from the cargo:error conversation. If you don't want to directly split this out into a separate PR, maybe create a Issue about it for us to discuss? Our contributing guidelines do ask for having Issues first and then, once Accepted, for PRs.

I have not been able to meet with the team for the past while. I'm hoping I can this next time. I have added these to the agenda.

bors · 2023-02-02T00:21:25Z

☔ The latest upstream changes (presumably #11252) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2023-08-10T17:27:25Z

Just note that with #12332 and later #12201, there should be no more backward compatibility concern for adding the cargo::error directive.

ratmice · 2023-12-31T07:07:10Z

I'm curious if @kornelski is considering picking this up again (given his other patch?),
If not, I would be happy to work on it.

Edit: moved some comments here to #13233

epage · 2024-01-01T22:21:30Z

+= needs some more fundamental design discussions and needs its own issue separate from error=

rustbot assigned epage Oct 31, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2022

epage reviewed Oct 31, 2022

View reviewed changes

epage added S-blocked and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked labels Oct 31, 2022

kornelski added 4 commits November 6, 2022 14:49

Emit errors with cargo:error=

bebeca3

Hint when errors are hidden

13f97d2

Align build script message wording with other compilation errors

db04f5f

cargo:warning+= syntax

2def6a9

kornelski force-pushed the build-cargo-error branch from da8263d to 2def6a9 Compare November 6, 2022 14:58

kornelski mentioned this pull request Jan 1, 2023

Trim noise from build script errors #11525

Closed

epage added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jan 31, 2023

XaurDesu mentioned this pull request May 7, 2023

Cannot extend build directives within cargo #11461

Closed

ratmice mentioned this pull request Jan 1, 2024

emitting multi-line messages from build scripts #13233

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit errors with cargo:error= #11312

Emit errors with cargo:error= #11312

kornelski commented Oct 31, 2022 •

edited

rustbot commented Oct 31, 2022

epage Oct 31, 2022

kornelski Nov 1, 2022 •

edited

epage Nov 1, 2022

epage Oct 31, 2022

epage Oct 31, 2022

kornelski Nov 1, 2022 •

edited

epage Nov 1, 2022

epage Dec 6, 2022

epage Oct 31, 2022

kornelski Nov 1, 2022

epage Nov 1, 2022

kornelski Nov 6, 2022

epage Oct 31, 2022

kornelski Nov 1, 2022

kornelski commented Nov 29, 2022 •

edited

epage commented Nov 29, 2022 •

edited

bors commented Feb 2, 2023

weihanglo commented Aug 10, 2023

ratmice commented Dec 31, 2023 •

edited

epage commented Jan 1, 2024

		let msg = "The following warnings were emitted during compilation:";
		self.emit_warnings(Some(msg), &unit, cx)?;

		} else if let Some(error) = stdout.strip_prefix(CARGO_ERROR) {
		diagnostics.push(Diagnostic::Error(error.to_owned()));

Emit errors with cargo:error= #11312

Are you sure you want to change the base?

Emit errors with cargo:error= #11312

Conversation

kornelski commented Oct 31, 2022 • edited

rustbot commented Oct 31, 2022

Choose a reason for hiding this comment

kornelski Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornelski Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornelski commented Nov 29, 2022 • edited

epage commented Nov 29, 2022 • edited

bors commented Feb 2, 2023

weihanglo commented Aug 10, 2023

ratmice commented Dec 31, 2023 • edited

epage commented Jan 1, 2024

kornelski commented Oct 31, 2022 •

edited

kornelski Nov 1, 2022 •

edited

kornelski Nov 1, 2022 •

edited

kornelski commented Nov 29, 2022 •

edited

epage commented Nov 29, 2022 •

edited

ratmice commented Dec 31, 2023 •

edited