From 9b04300dc31e9db148e36221147ecfe0bea72054 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 24 Apr 2026 19:39:09 +0200 Subject: [PATCH] refactor: use `anyhow` for xtask errors Terminating the process at arbitrary points with `std::process::exit` when errors occur has several problems. There is a lack of information about what lead up to the error, and it prevents destructors from running, which in the cases of xtasks can for example result in temporary files being left on the file system. Instead, use `anyhow` which conveniently integrates with Rust's Result type, allowing to return `anyhow::Result`, which is an alias for `Result`, which is compatible with any error type that implements `std::error::Error`. The advantages of using `anyhow` over plain `Result`s are that it makes it easier to handle different error types, attach context to errors, and show the call/context stack associated with the error. Returning an `anyhow::Result<()>` from `main` is possible because it implements `std::process::Termination`, so we get automatic error reporting and corresponding exit codes by simply bubbling up errors to `main`, attaching context as desired, and finally returning the result from `main.` In addition to removing the `std::process::exit` calls, this commit also improves error handling in a few spots in other ways, such as replacing `unwrap` by returning errors. Closes #12674 --- Cargo.lock | 7 +++ Cargo.toml | 1 + crates/xtask/Cargo.toml | 1 + crates/xtask/src/format.rs | 84 +++++++++++++++++++--------------- crates/xtask/src/lib.rs | 59 ++++++++++-------------- crates/xtask/src/main.rs | 62 +++++++++++++------------ crates/xtask/src/shellcheck.rs | 56 ++++++++++++----------- 7 files changed, 146 insertions(+), 124 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 218725a4e..2ae113c4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -67,6 +67,12 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "anyhow" +version = "1.0.102" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" + [[package]] name = "assert_matches" version = "1.5.0" @@ -1233,6 +1239,7 @@ name = "xtask" version = "0.0.0" dependencies = [ "anstyle", + "anyhow", "clap", "fish-build-helper", "fish-tempfile", diff --git a/Cargo.toml b/Cargo.toml index 023ea2d6d..ae59348f0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,7 @@ license = "GPL-2.0-only AND LGPL-2.0-or-later AND MIT AND PSF-2.0" [workspace.dependencies] anstyle = "1.0.13" +anyhow = "1.0.102" assert_matches = "1.5.0" bitflags = "2.5.0" cc = "1.0.94" diff --git a/crates/xtask/Cargo.toml b/crates/xtask/Cargo.toml index 66c92aaee..ce84c94fd 100644 --- a/crates/xtask/Cargo.toml +++ b/crates/xtask/Cargo.toml @@ -7,6 +7,7 @@ repository.workspace = true [dependencies] anstyle.workspace = true +anyhow.workspace = true clap.workspace = true fish-build-helper.workspace = true fish-tempfile.workspace = true diff --git a/crates/xtask/src/format.rs b/crates/xtask/src/format.rs index 88fd397c2..cd290ac5d 100644 --- a/crates/xtask/src/format.rs +++ b/crates/xtask/src/format.rs @@ -1,4 +1,5 @@ use anstyle::{AnsiColor, Style}; +use anyhow::{Context, Result, bail}; use clap::Args; use std::{ io::{ErrorKind, Write}, @@ -25,12 +26,12 @@ pub struct FormatArgs { paths: Vec, } -pub fn format(args: FormatArgs) { +pub fn format(args: FormatArgs) -> Result<()> { if !args.all && args.paths.is_empty() { println!( "{YELLOW}warning: No paths specified. Nothing to do. Use the \"--all\" flag to consider all eligible files.{YELLOW:#}" ); - return; + return Ok(()); } if !args.force && !args.check { match Command::new("git") @@ -39,16 +40,22 @@ pub fn format(args: FormatArgs) { { Ok(output) => { if !output.stdout.is_empty() { - std::io::stdout().write_all(&output.stdout).unwrap(); + std::io::stdout() + .write_all(&output.stdout) + .context("Could not write to stdout.")?; print!( "You have uncommitted changes (listed above). Are you sure you want to format? (y/N): " ); - std::io::stdout().flush().unwrap(); + std::io::stdout() + .flush() + .context("Could not flush stdout.")?; let mut response = String::new(); - std::io::stdin().read_line(&mut response).unwrap(); + std::io::stdin() + .read_line(&mut response) + .context("Could not read from stdin.")?; if response.trim_end() != "y" { println!("Exiting without formatting."); - return; + return Ok(()); } } } @@ -58,22 +65,25 @@ pub fn format(args: FormatArgs) { "{YELLOW}warning: Did not find git, will proceed without checking for unstaged changes.{YELLOW:#}" ) } else { - fail!("Failed to run git status:\n{e}") + bail!("Failed to run git status:\n{e}"); } } } } - format_fish(&args); - format_python(&args); - format_rust(&args); + format_fish(&args)?; + format_python(&args)?; + format_rust(&args)?; + Ok(()) } -fn run_formatter(formatter: &mut Command, name: &str) { +fn run_formatter(formatter: &mut Command, name: &str) -> Result<()> { println!("=== Running {GREEN}{name}{GREEN:#}"); match formatter.status() { Ok(exit_status) => { - if !exit_status.success() { - fail!("{name:?}: Files are not formatted correctly."); + if exit_status.success() { + Ok(()) + } else { + bail!("{name:?}: Files are not formatted correctly."); } } Err(e) => { @@ -81,15 +91,16 @@ fn run_formatter(formatter: &mut Command, name: &str) { eprintln!( "{YELLOW}Formatter not found: {name:?}. Skipping associated files.{YELLOW:#}" ); + Ok(()) } else { - fail!("Error occurred while running {name:?}:\n{e}") + Err(e).with_context(|| format!("Error occurred while running {name:?}")) } } } } -fn format_fish(args: &FormatArgs) { - let mut fish_paths = files_with_extension(&args.paths, "fish"); +fn format_fish(args: &FormatArgs) -> Result<()> { + let mut fish_paths = files_with_extension(&args.paths, "fish")?; if args.all { let workspace_root = fish_build_helper::workspace_root(); let fish_formatting_dirs = ["benchmarks", "build_tools", "etc", "share"]; @@ -98,10 +109,10 @@ fn format_fish(args: &FormatArgs) { .iter() .map(|dir_name| workspace_root.join(dir_name)), "fish", - )); + )?); }; if fish_paths.is_empty() { - return; + return Ok(()); } // TODO: make `fish_indent` available as a Rust library function, to avoid needing a // `fish_indent` binary in `$PATH`. @@ -113,40 +124,40 @@ fn format_fish(args: &FormatArgs) { } formatter.arg("--"); formatter.args(fish_paths); - run_formatter(&mut formatter, "fish_indent"); + run_formatter(&mut formatter, "fish_indent") } -fn format_python(args: &FormatArgs) { +fn format_python(args: &FormatArgs) -> Result<()> { let mut formatter = Command::new("ruff"); formatter.arg("format"); if args.check { formatter.arg("--check"); } - let mut python_files = files_with_extension(&args.paths, "py"); + let mut python_files = files_with_extension(&args.paths, "py")?; if args.all { python_files.push(fish_build_helper::workspace_root().to_owned()); }; if python_files.is_empty() { - return; + return Ok(()); } formatter.args(python_files); - run_formatter(&mut formatter, "ruff format"); + run_formatter(&mut formatter, "ruff format") } -fn format_rust(args: &FormatArgs) { +fn format_rust(args: &FormatArgs) -> Result<()> { let rustfmt_status = Command::new("cargo") .arg("fmt") .arg("--version") .stdout(Stdio::null()) .status() - .unwrap(); + .context("Failed to run cargo")?; if !rustfmt_status.success() { eprintln!( "{YELLOW}Please install \"rustfmt\" to format Rust, e.g. via:\n\ rustup component add rustfmt{YELLOW:#}" ); - return; + return Ok(()); } if args.all { let mut formatter = Command::new("cargo"); @@ -155,16 +166,17 @@ fn format_rust(args: &FormatArgs) { if args.check { formatter.arg("--check"); } - run_formatter(&mut formatter, "cargo fmt"); + run_formatter(&mut formatter, "cargo fmt")?; } - let rust_files = files_with_extension(&args.paths, "rs"); - if !rust_files.is_empty() { - let mut formatter = Command::new("rustfmt"); - if args.check { - formatter.arg("--check"); - formatter.arg("--files-with-diff"); - } - formatter.args(rust_files); - run_formatter(&mut formatter, "rustfmt"); + let rust_files = files_with_extension(&args.paths, "rs")?; + if rust_files.is_empty() { + return Ok(()); } + let mut formatter = Command::new("rustfmt"); + if args.check { + formatter.arg("--check"); + formatter.arg("--files-with-diff"); + } + formatter.args(rust_files); + run_formatter(&mut formatter, "rustfmt") } diff --git a/crates/xtask/src/lib.rs b/crates/xtask/src/lib.rs index aa12d95b3..b293c4664 100644 --- a/crates/xtask/src/lib.rs +++ b/crates/xtask/src/lib.rs @@ -4,68 +4,59 @@ process::Command, }; +use anyhow::{Context, Result, bail}; use walkdir::WalkDir; -macro_rules! fail { - ($($arg:tt)+) => {{ - eprintln!($($arg)+); - std::process::exit(1); - }} -} - pub mod format; pub mod shellcheck; pub trait CommandExt { - fn run_or_fail(&mut self); + fn run(&mut self) -> Result<()>; } impl CommandExt for Command { - fn run_or_fail(&mut self) { - match self.status() { - Ok(exit_status) => { - if !exit_status.success() { - fail!("Command did not run successfully: {:?}", self.get_program()) - } - } - Err(err) => { - fail!("Failed to run command: {err}") - } + fn run(&mut self) -> Result<()> { + if !self + .status() + .with_context(|| format!("Failed to run {:?}", self.get_program()))? + .success() + { + bail!("Command did not run successfully: {:?}", self.get_program()) } + Ok(()) } } -pub fn cargo(cargo_args: I) +pub fn cargo(cargo_args: I) -> Result<()> where I: IntoIterator, S: AsRef, { - Command::new(env!("CARGO")).args(cargo_args).run_or_fail(); + Command::new(env!("CARGO")).args(cargo_args).run() } fn get_matching_files, I: IntoIterator, M: Fn(&Path) -> bool>( all_paths: I, matcher: M, -) -> Vec { - all_paths - .into_iter() - .flat_map(WalkDir::new) - .filter_map(|res| { - let entry = res.unwrap(); - let path = entry.path(); - if entry.file_type().is_file() && matcher(path) { - Some(path.to_owned()) - } else { - None +) -> Result> { + let mut matching_files = vec![]; + for path in all_paths { + for dir_entry in WalkDir::new(path.as_ref()) { + let dir_entry = dir_entry + .with_context(|| format!("Failed to check paths at {:?}", path.as_ref()))?; + let path = dir_entry.path(); + if dir_entry.file_type().is_file() && matcher(path) { + matching_files.push(path.to_owned()); } - }) - .collect() + } + } + Ok(matching_files) } fn files_with_extension, I: IntoIterator>( all_paths: I, extension: &str, -) -> Vec { +) -> Result> { let matcher = |p: &Path| p.extension().is_some_and(|e| e == extension); get_matching_files(all_paths, matcher) } diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index 1e7c662b5..d328f89e0 100644 --- a/crates/xtask/src/main.rs +++ b/crates/xtask/src/main.rs @@ -1,7 +1,8 @@ +use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; use fish_build_helper::as_os_strs; use std::{path::PathBuf, process::Command}; -use xtask::{CommandExt as _, cargo, format::FormatArgs, shellcheck::shellcheck}; +use xtask::{CommandExt, cargo, format::FormatArgs, shellcheck::shellcheck}; #[derive(Parser)] #[command( @@ -33,7 +34,7 @@ enum Task { ShellCheck, } -fn main() { +fn main() -> Result<()> { let cli = Cli::parse(); match cli.task { Task::Check => run_checks(), @@ -44,41 +45,46 @@ fn main() { } } -fn run_checks() { +fn run_checks() -> Result<()> { let repo_root_dir = fish_build_helper::workspace_root(); let check_script = repo_root_dir.join("build_tools").join("check.sh"); - Command::new(check_script).run_or_fail(); + Command::new(check_script).run() } -fn build_html_docs(fish_indent: Option) { - let fish_indent_path = fish_indent.unwrap_or_else(|| { - // Build fish_indent if no existing one is specified. - cargo([ - "build", - "--bin", - "fish_indent", - "--profile", - "dev", - "--no-default-features", - ]); - fish_build_helper::fish_build_dir() - .join("debug") - .join("fish_indent") - }); +fn build_html_docs(fish_indent: Option) -> Result<()> { + let fish_indent_path = match fish_indent { + Some(path) => path, + None => { + // Build fish_indent if no existing one is specified. + cargo([ + "build", + "--bin", + "fish_indent", + "--profile", + "dev", + "--no-default-features", + ])?; + fish_build_helper::fish_build_dir() + .join("debug") + .join("fish_indent") + } + }; // Set path so `sphinx-build` can find `fish_indent`. // Create tempdir to store symlink to fish_indent. // This is done to avoid adding other binaries to the PATH. - let tempdir = fish_tempfile::new_dir().unwrap(); + let tempdir = fish_tempfile::new_dir().context("Failed to create tempdir")?; std::os::unix::fs::symlink( - std::fs::canonicalize(fish_indent_path).unwrap(), + std::fs::canonicalize(&fish_indent_path).with_context(|| { + format!("Failed to canonicalize path to `fish_indent`: {fish_indent_path:?}") + })?, tempdir.path().join("fish_indent"), ) - .unwrap(); - let new_path = format!( - "{}:{}", - tempdir.path().to_str().unwrap(), - fish_build_helper::env_var("PATH").unwrap() - ); + .context("Failed to create symlink for fish_indent")?; + let mut new_path = tempdir.path().as_os_str().to_owned(); + if let Some(current_path) = std::env::var_os("PATH") { + new_path.push(":"); + new_path.push(current_path); + } let doc_src_dir = fish_build_helper::workspace_root().join("doc_src"); let doctrees_dir = fish_build_helper::fish_doc_dir().join(".doctrees-html"); let html_dir = fish_build_helper::fish_doc_dir().join("html"); @@ -98,5 +104,5 @@ fn build_html_docs(fish_indent: Option) { Command::new(option_env!("FISH_SPHINX").unwrap_or("sphinx-build")) .env("PATH", new_path) .args(args) - .run_or_fail(); + .run() } diff --git a/crates/xtask/src/shellcheck.rs b/crates/xtask/src/shellcheck.rs index 79cace797..c6963d800 100644 --- a/crates/xtask/src/shellcheck.rs +++ b/crates/xtask/src/shellcheck.rs @@ -1,3 +1,4 @@ +use anyhow::{Context, Result}; use fish_build_helper::workspace_root; use ignore::Walk; use pcre2::bytes::Regex; @@ -9,40 +10,43 @@ sync::LazyLock, }; -pub fn shellcheck() { - let file_paths = files_to_check(); - match Command::new("shellcheck") - .args(file_paths) +use crate::CommandExt; + +pub fn shellcheck() -> Result<()> { + Command::new("shellcheck") + .args(files_to_check()?) .current_dir(workspace_root()) - .status() - { - Ok(status) => { - std::process::exit(status.code().unwrap_or(1)); - } - Err(e) => { - eprintln!("Failed to run shellcheck: {e}"); - std::process::exit(1); - } - } + .run() } -fn is_shell_script>(path: P) -> bool { - let file = File::open(&path).unwrap(); +fn is_shell_script>(path: P) -> Result { + let file = File::open(&path).with_context(|| format!("Failed to open {:?}", path.as_ref()))?; let mut first_line = String::new(); let Ok(_) = BufReader::new(file).read_line(&mut first_line) else { - return false; + return Ok(false); }; static SHEBANG_REGEX: LazyLock = LazyLock::new(|| Regex::new("^#!.*[^i]sh").unwrap()); - SHEBANG_REGEX + Ok(SHEBANG_REGEX .is_match(first_line.trim().as_bytes()) - .unwrap() + .unwrap()) } -fn files_to_check() -> Vec { - Walk::new(workspace_root()) - .map(|path| path.unwrap_or_else(|e| fail!("Error traversing workspace: {e}"))) - .filter(|path| path.file_type().unwrap().is_file()) - .map(|path| path.into_path()) - .filter(|path| is_shell_script(path)) - .collect() +fn files_to_check() -> Result> { + let mut files = vec![]; + for dir_entry in Walk::new(workspace_root()) { + let dir_entry = dir_entry.context("Error traversing workspace")?; + if !dir_entry + .file_type() + .with_context(|| format!("Failed to determine file type of {dir_entry:?}"))? + .is_file() + { + continue; + } + let path = dir_entry.into_path(); + if !is_shell_script(&path)? { + continue; + } + files.push(path); + } + Ok(files) }