h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-10 21:11:15 -03:00
Code Issues Packages Projects Releases Wiki Activity

refactor: use anyhow for xtask errors

Browse Source 
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<T>`, which is an alias for
`Result<T, anyhow::Error>`, 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
This commit is contained in:
Daniel Rainer
2026-04-24 19:39:09 +02:00
committed by Johannes Altmanninger
parent c80496fad1
commit 9b04300dc3
7 changed files with 146 additions and 124 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
Cargo.lock generated
View File

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

1
Cargo.toml
View File

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

1
crates/xtask/Cargo.toml
View File

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

84
crates/xtask/src/format.rs
View File

@@ -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<PathBuf>,
}
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")
}

59
crates/xtask/src/lib.rs
View File

@@ -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<I, S>(cargo_args: I)
pub fn cargo<I, S>(cargo_args: I) -> Result<()>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
{
Command::new(env!("CARGO")).args(cargo_args).run_or_fail();
Command::new(env!("CARGO")).args(cargo_args).run()
}
fn get_matching_files<P: AsRef<Path>, I: IntoIterator<Item = P>, M: Fn(&Path) -> bool>(
all_paths: I,
matcher: M,
) -> Vec<PathBuf> {
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<Vec<PathBuf>> {
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<P: AsRef<Path>, I: IntoIterator<Item = P>>(
all_paths: I,
extension: &str,
) -> Vec<PathBuf> {
) -> Result<Vec<PathBuf>> {
let matcher = |p: &Path| p.extension().is_some_and(|e| e == extension);
get_matching_files(all_paths, matcher)
}

62
crates/xtask/src/main.rs
View File

@@ -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<PathBuf>) {
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<PathBuf>) -> 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<PathBuf>) {
Command::new(option_env!("FISH_SPHINX").unwrap_or("sphinx-build"))
.env("PATH", new_path)
.args(args)
.run_or_fail();
.run()
}

56
crates/xtask/src/shellcheck.rs
View File

@@ -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<P: AsRef<Path>>(path: P) -> bool {
let file = File::open(&path).unwrap();
fn is_shell_script<P: AsRef<Path>>(path: P) -> Result<bool> {
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<Regex> = 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<PathBuf> {
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<Vec<PathBuf>> {
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)
}