From d5be17147426b1a01a862b212bb9cf300832b7aa Mon Sep 17 00:00:00 2001 From: Finchie Date: Mon, 24 Jul 2023 15:23:45 +0800 Subject: [PATCH 1/4] Improve error message when library not found Mostly just small tweaks to existing output, including the following new output: - Potential packages to install - Items in PKG_CONFIG_PATH - Command invocation --- src/lib.rs | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 853db8b..fc3070a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -208,12 +208,72 @@ impl fmt::Display for Error { ref command, ref output, } => { - write!( + let crate_name = + env::var("CARGO_PKG_NAME").unwrap_or(String::from("")); + + writeln!(f, "")?; + + // Give a short explanation of what the error is + writeln!( f, - "`{}` did not exit successfully: {}\nerror: could not find system library '{}' required by the '{}' crate\n", - command, output.status, name, env::var("CARGO_PKG_NAME").unwrap_or_default(), + "pkg-config {}", + match output.status.code() { + Some(code) => format!("exited with status code {}", code), + None => "was terminated by signal".to_string(), + } )?; - format_output(output, f) + + // Give the command run so users can reproduce the error + writeln!(f, "> {}\n", command)?; + + // Explain how it was caused + writeln!( + f, + "The system library `{}` required by crate `{}` was not found.", + name, crate_name + )?; + writeln!( + f, + "The file `{}.pc` needs to be installed and the PKG_CONFIG_PATH environment variable must contain its parent directory.", + name + )?; + + // There will be no status code if terminated by signal + if let Some(_code) = output.status.code() { + // NixOS uses a wrapper script for pkg-config that sets the custom + // environment variable PKG_CONFIG_PATH_FOR_TARGET + let search_path = + if let Ok(path_for_target) = env::var("PKG_CONFIG_PATH_FOR_TARGET") { + Some(path_for_target) + } else if let Ok(path) = env::var("PKG_CONFIG_PATH") { + Some(path) + } else { + None + }; + + // Guess the most reasonable course of action + let hint = if let Some(search_path) = search_path { + writeln!( + f, + "PKG_CONFIG_PATH contains the following:\n{}", + search_path + .split(':') + .map(|path| format!(" - {}", path)) + .collect::>() + .join("\n"), + )?; + + format!("you may need to install a package such as {name}, {name}-dev or {name}-devel.", name=name) + } else { + writeln!(f, "PKG_CONFIG_PATH environment variable is not set")?; + format!("If you have installed the library, try adding its parent directory to your PATH.") + }; + + // Try and nudge the user in the right direction so they don't get stuck + writeln!(f, "\nHINT: {}", hint)?; + } + + Ok(()) } Error::Failure { ref command, @@ -498,8 +558,34 @@ impl Config { if output.status.success() { Ok(output.stdout) } else { + // Collect all explicitly-defined environment variables + // this is used to display the equivalent pkg-config shell invocation + let envs = cmd + .get_envs() + .map(|(env, optional_arg)| { + if let Some(arg) = optional_arg { + format!("{}={}", env.to_string_lossy(), arg.to_string_lossy()) + } else { + env.to_string_lossy().to_string() + } + }) + .collect::>(); + + // Collect arguments for the same reason + let args = cmd + .get_args() + .map(|arg| arg.to_string_lossy().to_string()) + .collect::>(); + + // This will look something like: + // PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --libs --cflags {library} Err(Error::Failure { - command: format!("{:?}", cmd), + command: format!( + "{} {} {}", + envs.join(" "), + cmd.get_program().to_string_lossy(), + args.join(" ") + ), output, }) } From a0640aadcbe7bc28334633869733e71eceed3f8b Mon Sep 17 00:00:00 2001 From: Finchie Date: Tue, 19 Dec 2023 17:00:01 +0800 Subject: [PATCH 2/4] Wrap `std::process::Command` to track arguments and environment variables Before, this was accomplished by using `get_env` and `get_args` manually, but these features are unavailable in 1.30 (current MSRV). To get around this, `Command` is wrapped as `WrappedCommand` and tracks any environment variables or arguments that are set by the caller. Then, the `Display` implementation actually handles pretty-printing the invocation, with the added benefit that other errors can take advantage of this nicer output! --- src/lib.rs | 120 +++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 30 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index fc3070a..4c3d5f0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -67,12 +67,23 @@ use std::env; use std::error; use std::ffi::{OsStr, OsString}; use std::fmt; +use std::fmt::Display; use std::io; use std::ops::{Bound, RangeBounds}; use std::path::PathBuf; use std::process::{Command, Output}; use std::str; +/// Wrapper struct to polyfill methods introduced in 1.57 (`get_envs`, `get_args` etc). +/// This is needed to reconstruct the pkg-config command for output in a copy- +/// paste friendly format via `Display`. +struct WrappedCommand { + inner: Command, + program: OsString, + env_vars: Vec<(OsString, OsString)>, + args: Vec, +} + #[derive(Clone, Debug)] pub struct Config { statik: Option, @@ -148,6 +159,81 @@ pub enum Error { __Nonexhaustive, } +impl WrappedCommand { + fn new>(program: S) -> Self { + Self { + inner: Command::new(program.as_ref()), + program: program.as_ref().to_os_string(), + env_vars: Vec::new(), + args: Vec::new(), + } + } + + fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator + Clone, + S: AsRef, + { + self.inner.args(args.clone()); + self.args + .extend(args.into_iter().map(|arg| arg.as_ref().to_os_string())); + + self + } + + fn arg>(&mut self, arg: S) -> &mut Self { + self.inner.arg(arg.as_ref()); + self.args.push(arg.as_ref().to_os_string()); + + self + } + + fn env(&mut self, key: K, value: V) -> &mut Self + where + K: AsRef, + V: AsRef, + { + self.inner.env(key.as_ref(), value.as_ref()); + self.env_vars + .push((key.as_ref().to_os_string(), value.as_ref().to_os_string())); + + self + } + + fn output(&mut self) -> io::Result { + self.inner.output() + } +} + +/// Output a command invocation that can be copy-pasted into the terminal. +/// `Command`'s existing debug implementation is not used for that reason, +/// as it can sometimes lead to output such as: +/// `PKG_CONFIG_ALLOW_SYSTEM_CFLAGS="1" PKG_CONFIG_ALLOW_SYSTEM_LIBS="1" "pkg-config" "--libs" "--cflags" "mylibrary"` +/// Which cannot be copy-pasted into terminals such as nushell, and is a bit noisy. +/// This will look something like: +/// `PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --libs --cflags mylibrary` +impl Display for WrappedCommand { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // Format all explicitly defined environment variables + let envs = self + .env_vars + .iter() + .map(|(env, arg)| format!("{}={}", env.to_string_lossy(), arg.to_string_lossy())) + .collect::>() + .join(" "); + + // Format all pkg-config arguments + let args = self + .args + .iter() + .map(|arg| arg.to_string_lossy().to_string()) + .collect::>() + .join(" "); + + write!(f, "{} {} {}", envs, self.program.to_string_lossy(), args) + } +} + impl error::Error for Error {} impl fmt::Debug for Error { @@ -558,47 +644,21 @@ impl Config { if output.status.success() { Ok(output.stdout) } else { - // Collect all explicitly-defined environment variables - // this is used to display the equivalent pkg-config shell invocation - let envs = cmd - .get_envs() - .map(|(env, optional_arg)| { - if let Some(arg) = optional_arg { - format!("{}={}", env.to_string_lossy(), arg.to_string_lossy()) - } else { - env.to_string_lossy().to_string() - } - }) - .collect::>(); - - // Collect arguments for the same reason - let args = cmd - .get_args() - .map(|arg| arg.to_string_lossy().to_string()) - .collect::>(); - - // This will look something like: - // PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --libs --cflags {library} Err(Error::Failure { - command: format!( - "{} {} {}", - envs.join(" "), - cmd.get_program().to_string_lossy(), - args.join(" ") - ), + command: format!("{}", cmd), output, }) } } Err(cause) => Err(Error::Command { - command: format!("{:?}", cmd), + command: format!("{}", cmd), cause, }), } } - fn command(&self, exe: OsString, name: &str, args: &[&str]) -> Command { - let mut cmd = Command::new(exe); + fn command(&self, exe: OsString, name: &str, args: &[&str]) -> WrappedCommand { + let mut cmd = WrappedCommand::new(exe); if self.is_static(name) { cmd.arg("--static"); } From 1ac32cd3ef57773b99246782e704df0379955ca1 Mon Sep 17 00:00:00 2001 From: Finchie Date: Wed, 20 Dec 2023 00:47:54 +0800 Subject: [PATCH 3/4] Print correct variable name for search path As pointed out by @sdroege in #158, Nix users would be misled when `PKG_CONFIG_PATH_FOR_TARGET` is listed in error output but mistakenly named `PKG_CONFIG_PATH`. --- src/lib.rs | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4c3d5f0..c3f9a85 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -326,22 +326,25 @@ impl fmt::Display for Error { // There will be no status code if terminated by signal if let Some(_code) = output.status.code() { - // NixOS uses a wrapper script for pkg-config that sets the custom + // Nix uses a wrapper script for pkg-config that sets the custom // environment variable PKG_CONFIG_PATH_FOR_TARGET - let search_path = - if let Ok(path_for_target) = env::var("PKG_CONFIG_PATH_FOR_TARGET") { - Some(path_for_target) - } else if let Ok(path) = env::var("PKG_CONFIG_PATH") { - Some(path) - } else { - None - }; + let search_locations = ["PKG_CONFIG_PATH_FOR_TARGET", "PKG_CONFIG_PATH"]; + + // Find a search path to use + let mut search_data = None; + for location in search_locations { + if let Ok(search_path) = env::var(location) { + search_data = Some((location, search_path)); + break; + } + } // Guess the most reasonable course of action - let hint = if let Some(search_path) = search_path { + let hint = if let Some((search_location, search_path)) = search_data { writeln!( f, - "PKG_CONFIG_PATH contains the following:\n{}", + "{} contains the following:\n{}", + search_location, search_path .split(':') .map(|path| format!(" - {}", path)) @@ -351,8 +354,13 @@ impl fmt::Display for Error { format!("you may need to install a package such as {name}, {name}-dev or {name}-devel.", name=name) } else { - writeln!(f, "PKG_CONFIG_PATH environment variable is not set")?; - format!("If you have installed the library, try adding its parent directory to your PATH.") + // Even on Nix, setting PKG_CONFIG_PATH seems to be a viable option + writeln!(f, "The PKG_CONFIG_PATH environment variable is not set.")?; + + format!( + "if you have installed the library, try setting PKG_CONFIG_PATH to the directory containing `{}.pc`.", + name + ) }; // Try and nudge the user in the right direction so they don't get stuck From 08a1c5f62e9fff6b47db944a4e8a8b283a5bd7b2 Mon Sep 17 00:00:00 2001 From: Finchie Date: Wed, 20 Dec 2023 01:00:28 +0800 Subject: [PATCH 4/4] Fix compilation for 1ac32cd on Rust 1.30 --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index c3f9a85..87e35bb7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -332,7 +332,7 @@ impl fmt::Display for Error { // Find a search path to use let mut search_data = None; - for location in search_locations { + for location in search_locations.iter() { if let Ok(search_path) = env::var(location) { search_data = Some((location, search_path)); break;