Auto merge of #14743 - ehuss:build-script-errors, r=epage

Allow build scripts to report error messages through `cargo::error`

Adds the `cargo::error` build script directive. It is similar to `cargo::warning`, but it emits an error message and it also fails the build.

This is a repost of #14435 with the tests updated, a note added to the documentation about using this in a library, and updating the MSRV.

Closes #10159

Author: bors

src/cargo/core/compiler/custom_build.rs

@@ -50,6 +50,11 @@ use std::path::{Path, PathBuf};
 use std::str::{self, FromStr};
 use std::sync::{Arc, Mutex};
 
+/// A build script instruction that tells Cargo to display an error after the
+/// build script has finished running. Read [the doc] for more.
+///
+/// [the doc]: https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargo-error
+const CARGO_ERROR_SYNTAX: &str = "cargo::error=";
 /// Deprecated: A build script instruction that tells Cargo to display a warning after the
 /// build script has finished running. Read [the doc] for more.
 ///
@@ -60,6 +65,15 @@ const OLD_CARGO_WARNING_SYNTAX: &str = "cargo:warning=";
 ///
 /// [the doc]: https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargo-warning
 const NEW_CARGO_WARNING_SYNTAX: &str = "cargo::warning=";
+
+#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
+pub enum Severity {
+    Error,
+    Warning,
+}
+
+pub type LogMessage = (Severity, String);
+
 /// Contains the parsed output of a custom build script.
 #[derive(Clone, Debug, Hash, Default, PartialEq, Eq, PartialOrd, Ord)]
 pub struct BuildOutput {
@@ -82,11 +96,13 @@ 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 generated by this build.
+    /// Errors and warnings generated by this build.
     ///
-    /// These 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<String>,
+    /// These are only displayed if this is a "local" package, `-vv` is used, or
+    /// there is a build error for any target in this package. Note that any log
+    /// message of severity `Error` will by itself cause a build error, and will
+    /// cause all log messages to be displayed.
+    pub log_messages: Vec<LogMessage>,
 }
 
 /// Map of packages to build script output.
@@ -474,15 +490,18 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
         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 log_messages_in_case_of_panic = Vec::new();
         let output = cmd
             .exec_with_streaming(
                 &mut |stdout| {
+                    if let Some(error) = stdout.strip_prefix(CARGO_ERROR_SYNTAX) {
+                        log_messages_in_case_of_panic.push((Severity::Error, error.to_owned()));
+                    }
                     if let Some(warning) = stdout
                         .strip_prefix(OLD_CARGO_WARNING_SYNTAX)
                         .or(stdout.strip_prefix(NEW_CARGO_WARNING_SYNTAX))
                     {
-                        warnings_in_case_of_panic.push(warning.to_owned());
+                        log_messages_in_case_of_panic.push((Severity::Warning, warning.to_owned()));
                     }
                     if extra_verbose {
                         state.stdout(format!("{}{}", prefix, stdout))?;
@@ -522,15 +541,29 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
                 build_error_context
             });
 
+        // If the build failed
         if let Err(error) = output {
-            insert_warnings_in_build_outputs(
+            insert_log_messages_in_build_outputs(
                 build_script_outputs,
                 id,
                 metadata_hash,
-                warnings_in_case_of_panic,
+                log_messages_in_case_of_panic,
             );
             return Err(error);
         }
+        // ... or it logged any errors
+        else if log_messages_in_case_of_panic
+            .iter()
+            .any(|(severity, _)| *severity == Severity::Error)
+        {
+            insert_log_messages_in_build_outputs(
+                build_script_outputs,
+                id,
+                metadata_hash,
+                log_messages_in_case_of_panic,
+            );
+            anyhow::bail!("build script logged errors");
+        }
 
         let output = output.unwrap();
 
@@ -611,22 +644,23 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
     Ok(job)
 }
 
-/// When a build script run fails, store only warnings and nuke other outputs,
-/// as they are likely broken.
-fn insert_warnings_in_build_outputs(
+/// When a build script run fails, store only log messages, and nuke other
+/// outputs, as they are likely broken.
+fn insert_log_messages_in_build_outputs(
     build_script_outputs: Arc<Mutex<BuildScriptOutputs>>,
     id: PackageId,
     metadata_hash: Metadata,
-    warnings: Vec<String>,
+    log_messages: Vec<LogMessage>,
 ) {
-    let build_output_with_only_warnings = BuildOutput {
-        warnings,
+    let build_output_with_only_log_messages = BuildOutput {
+        log_messages,
         ..BuildOutput::default()
     };
-    build_script_outputs
-        .lock()
-        .unwrap()
-        .insert(id, metadata_hash, build_output_with_only_warnings);
+    build_script_outputs.lock().unwrap().insert(
+        id,
+        metadata_hash,
+        build_output_with_only_log_messages,
+    );
 }
 
 impl BuildOutput {
@@ -678,7 +712,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 log_messages = Vec::new();
         let whence = format!("build script of `{}`", pkg_descr);
         // Old syntax:
         //    cargo:rustc-flags=VALUE
@@ -850,15 +884,18 @@ 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!(
-                            "{}{} was specified in the build script of {}, \
+                        log_messages.push((
+                            Severity::Warning,
+                            format!(
+                                "{}{} was specified in the build script of {}, \
                              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>.",
-                            syntax_prefix, key, pkg_descr
+                                syntax_prefix, key, pkg_descr
+                            ),
                         ));
                     }
                     linker_args.push((LinkArgTarget::Cdylib, value))
@@ -944,10 +981,10 @@ impl BuildOutput {
                         if nightly_features_allowed
                             || rustc_bootstrap_allows(library_name.as_deref())
                         {
-                            warnings.push(format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\n\
+                            log_messages.push((Severity::Warning, format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\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.
@@ -963,7 +1000,8 @@ impl BuildOutput {
                         env.push((key, val));
                     }
                 }
-                "warning" => warnings.push(value.to_string()),
+                "error" => log_messages.push((Severity::Error, value.to_string())),
+                "warning" => log_messages.push((Severity::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" => {
@@ -988,7 +1026,7 @@ impl BuildOutput {
             metadata,
             rerun_if_changed,
             rerun_if_env_changed,
-            warnings,
+            log_messages,
         })
     }
 

src/cargo/core/compiler/job_queue/mod.rs

@@ -132,6 +132,7 @@ pub use self::job::Freshness::{self, Dirty, Fresh};
 pub use self::job::{Job, Work};
 pub use self::job_state::JobState;
 use super::build_runner::OutputFile;
+use super::custom_build::Severity;
 use super::timings::Timings;
 use super::{BuildContext, BuildPlan, BuildRunner, CompileMode, Unit};
 use crate::core::compiler::descriptive_pkg_name;
@@ -684,8 +685,8 @@ impl<'gctx> DrainState<'gctx> {
                         self.queue.finish(&unit, &artifact);
                     }
                     Err(error) => {
-                        let msg = "The following warnings were emitted during compilation:";
-                        self.emit_warnings(Some(msg), &unit, build_runner)?;
+                        let show_warnings = true;
+                        self.emit_log_messages(&unit, build_runner, show_warnings)?;
                         self.back_compat_notice(build_runner, &unit)?;
                         return Err(ErrorToHandle {
                             error,
@@ -962,33 +963,37 @@ impl<'gctx> DrainState<'gctx> {
         }
     }
 
-    fn emit_warnings(
+    fn emit_log_messages(
         &mut self,
-        msg: Option<&str>,
         unit: &Unit,
         build_runner: &mut BuildRunner<'_, '_>,
+        show_warnings: bool,
     ) -> CargoResult<()> {
         let outputs = build_runner.build_script_outputs.lock().unwrap();
         let Some(metadata) = build_runner.find_build_script_metadata(unit) else {
             return Ok(());
         };
         let bcx = &mut build_runner.bcx;
         if let Some(output) = outputs.get(metadata) {
-            if !output.warnings.is_empty() {
-                if let Some(msg) = msg {
-                    writeln!(bcx.gctx.shell().err(), "{}\n", msg)?;
-                }
-
-                for warning in output.warnings.iter() {
-                    let warning_with_package =
-                        format!("{}@{}: {}", unit.pkg.name(), unit.pkg.version(), warning);
-
-                    bcx.gctx.shell().warn(warning_with_package)?;
-                }
-
-                if msg.is_some() {
-                    // Output an empty line.
-                    writeln!(bcx.gctx.shell().err())?;
+            if !output.log_messages.is_empty()
+                && (show_warnings
+                    || output
+                        .log_messages
+                        .iter()
+                        .any(|(severity, _)| *severity == Severity::Error))
+            {
+                let msg_with_package =
+                    |msg: &str| format!("{}@{}: {}", unit.pkg.name(), unit.pkg.version(), msg);
+
+                for (severity, message) in output.log_messages.iter() {
+                    match severity {
+                        Severity::Error => {
+                            bcx.gctx.shell().error(msg_with_package(message))?;
+                        }
+                        Severity::Warning => {
+                            bcx.gctx.shell().warn(msg_with_package(message))?;
+                        }
+                    }
                 }
             }
         }
@@ -1098,8 +1103,12 @@ impl<'gctx> DrainState<'gctx> {
         artifact: Artifact,
         build_runner: &mut BuildRunner<'_, '_>,
     ) -> CargoResult<()> {
-        if unit.mode.is_run_custom_build() && unit.show_warnings(build_runner.bcx.gctx) {
-            self.emit_warnings(None, unit, build_runner)?;
+        if unit.mode.is_run_custom_build() {
+            self.emit_log_messages(
+                unit,
+                build_runner,
+                unit.show_warnings(build_runner.bcx.gctx),
+            )?;
         }
         let unlocked = self.queue.finish(unit, &artifact);
         match artifact {

src/doc/src/reference/build-scripts.md

@@ -128,6 +128,7 @@ one detailed below.
 * [`cargo::rustc-env=VAR=VALUE`](#rustc-env) --- Sets an environment variable.
 * [`cargo::rustc-cdylib-link-arg=FLAG`](#rustc-cdylib-link-arg) --- Passes custom
   flags to a linker for cdylib crates.
+- [`cargo::error=MESSAGE`](#cargo-error) --- Displays an error on the terminal.
 * [`cargo::warning=MESSAGE`](#cargo-warning) --- Displays a warning on the
   terminal.
 * [`cargo::metadata=KEY=VALUE`](#the-links-manifest-key) --- Metadata, used by `links`
@@ -313,13 +314,27 @@ link-arg=FLAG` option][link-arg] to the compiler, but only when building a
 `cdylib` library target. Its usage is highly platform specific. It is useful
 to set the shared library version or the runtime-path.
 
+### `cargo::error=MESSAGE` {#cargo-error}
+
+The `error` instruction tells Cargo to display an error after the build script
+has finished running, and then fail the build.
+
+ > Note: Build script libraries should carefully consider if they want to
+ > use `cargo::error` versus returning a `Result`. It may be better to return
+ > a `Result`, and allow the caller to decide if the error is fatal or not.
+ > The caller can then decide whether or not to display the `Err` variant
+ > using `cargo::error`.
+
+> **MSRV:** Respected as of 1.84
+
 ### `cargo::warning=MESSAGE` {#cargo-warning}
 
 The `warning` instruction tells Cargo to display a warning after the build
 script has finished running. Warnings are only shown for `path` dependencies
 (that is, those you're working on locally), so for example warnings printed
-out in [crates.io] crates are not emitted by default. The `-vv` "very verbose"
-flag may be used to have Cargo display warnings for all crates.
+out in [crates.io] crates are not emitted by default, unless the build fails.
+The `-vv` "very verbose" flag may be used to have Cargo display warnings for
+all crates.
 
 ## Build Dependencies