From 3e7e57945c912510fc618dca2b4176ce1fcb3a83 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 20 Feb 2026 15:29:07 +0100 Subject: [PATCH] format: replace `style.fish` by xtask Replace the `build_tools/style.fish` script by an xtask. This eliminates the need for a fish binary for performing the formatting/checking. The `fish_indent` binary is still needed. Eventually, this should be made available as a library function, so the xtask can use that instead of requiring a `fish_indent` binary in the `$PATH`. The new xtask is called `format` rather than `style`, because that's a more fitting description of what it does (and what the script it replaces did). The old script's behavior is not replicated exactly: - Specifying `--all` and explicit paths is supported within a single invocation. - Explicit arguments no longer have to be files. If a directory is specified, all files within it will be considered. - The git check for un-staged changes is no longer filtered by file names, mainly to simplify the implementation. - A warning is now printed if neither the `--all` flag nor a path are provided as arguments. The reason for this is that one might assume that omitting these arguments would default to formatting everything in the current directory, but instead no formatting will happen in this case. - The wording of some messages is different. The design of the new code tries to make it easy to add formatters for additional languages, or change the ones we already have. This is achieved by separating the code into one function per language, which can be modified without touching the code for the other languages. Adding support for a new formatter/language only requires adding a function which builds the formatter command line based on the arguments to the xtask, and calling that function from the main `format` function. Closes #12467 --- .github/workflows/lint.yml | 4 +- CONTRIBUTING.rst | 6 +- Cargo.lock | 2 + Cargo.toml | 2 + build_tools/check.sh | 2 +- build_tools/style.fish | 123 ----------------------- crates/xtask/Cargo.toml | 2 + crates/xtask/src/format.rs | 195 +++++++++++++++++++++++++++++++++++++ crates/xtask/src/lib.rs | 2 + crates/xtask/src/main.rs | 5 +- 10 files changed, 213 insertions(+), 130 deletions(-) delete mode 100755 build_tools/style.fish create mode 100644 crates/xtask/src/format.rs diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 385642249..83a64da0e 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -16,9 +16,9 @@ jobs: - name: install dependencies run: pip install ruff - name: build fish - run: cargo build + run: cargo build --bin fish_indent - name: check format - run: PATH="target/debug:$PATH" build_tools/style.fish --all --check + run: PATH="target/debug:$PATH" cargo xtask format --all --check - name: check rustfmt run: find build.rs crates src -type f -name '*.rs' | xargs rustfmt --check diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 920fd2843..d79c41948 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -133,12 +133,12 @@ For formatting, we use: - ``fish_indent`` (shipped with fish) for fish script - ``ruff format`` for Python -To reformat files, there is a script +To reformat files, there is an xtask :: - build_tools/style.fish --all - build_tools/style.fish somefile.rs some.fish + cargo xtask format --all + cargo xtask format somefile.rs some.fish Fish Script Style Guide ----------------------- diff --git a/Cargo.lock b/Cargo.lock index b36c8215c..b371b3b5e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1148,9 +1148,11 @@ checksum = "f17a85883d4e6d00e8a97c586de764dabcc06133f7f1d55dce5cdc070ad7fe59" name = "xtask" version = "0.0.0" dependencies = [ + "anstyle", "clap", "fish-build-helper", "fish-tempfile", + "walkdir", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d89f83d02..14cc50478 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,7 @@ repository = "https://github.com/fish-shell/fish-shell" license = "GPL-2.0-only AND LGPL-2.0-or-later AND MIT AND PSF-2.0" [workspace.dependencies] +anstyle = "1.0.13" assert_matches = "1.5.0" bitflags = "2.5.0" cc = "1.0.94" @@ -76,6 +77,7 @@ widestring = "1.2.0" unicode-segmentation = "1.12.0" unicode-width = "0.2.0" unix_path = "1.0.1" +walkdir = "2.5.0" xterm-color = "1.0.1" [profile.release] diff --git a/build_tools/check.sh b/build_tools/check.sh index 4f70e035e..c3fbf577e 100755 --- a/build_tools/check.sh +++ b/build_tools/check.sh @@ -78,7 +78,7 @@ if $lint; then if command -v cargo-deny >/dev/null; then cargo deny --all-features --locked --exclude-dev check licenses fi - PATH="$build_dir:$PATH" "$workspace_root/build_tools/style.fish" --all --check + PATH="$build_dir:$PATH" cargo xtask format --all --check for features in "" --no-default-features; do cargo clippy --workspace --all-targets $features done diff --git a/build_tools/style.fish b/build_tools/style.fish deleted file mode 100755 index b8fd1f8a0..000000000 --- a/build_tools/style.fish +++ /dev/null @@ -1,123 +0,0 @@ -#!/usr/bin/env fish -# -# This runs Python files, fish scripts (*.fish), and Rust files -# through their respective code formatting programs. -# -# `--all`: Format all eligible files instead of the ones specified as arguments. -# `--check`: Instead of reformatting, fail if a file is not formatted correctly. -# `--force`: Proceed without asking if uncommitted changes are detected. -# Only relevant if `--all` is specified but `--check` is not specified. - -set -l fish_files -set -l python_files -set -l rust_files -set -l all no - -argparse all check force -- $argv -or exit $status - -if set -l -q _flag_all - set all yes - if set -q argv[1] - echo "Unexpected arguments: '$argv'" - exit 1 - end -end - -set -l workspace_root (realpath (status dirname)/..) - -if test $all = yes - if not set -l -q _flag_force; and not set -l -q _flag_check - # Potential for false positives: Not all fish files are formatted, see the `fish_files` - # definition below. - set -l relevant_uncommitted_changes (git status --porcelain --short --untracked-files=all | sed -e 's/^ *[^ ]* *//' | grep -E '.*\.(fish|py|rs)$') - if set -q relevant_uncommitted_changes[1] - for changed_file in $relevant_uncommitted_changes - echo $changed_file - end - echo - echo 'You have uncommitted changes (listed above). Are you sure you want to restyle?' - read -P 'y/N? ' -n1 -l ans - if not string match -qi y -- $ans - exit 1 - end - end - end - set fish_files $workspace_root/{benchmarks,build_tools,etc,share}/**.fish - set python_files $workspace_root -else - # Format the files specified as arguments. - set -l files $argv - set fish_files (string match -r '^.*\.fish$' -- $files) - set python_files (string match -r '^.*\.py$' -- $files) - set rust_files (string match -r '^.*\.rs$' -- $files) -end - -set -l red (set_color red) -set -l green (set_color green) -set -l yellow (set_color yellow) -set -l normal (set_color normal) - -function die -V red -V normal - echo $red$argv[1]$normal - exit 1 -end - -if set -q fish_files[1] - if not type -q fish_indent - echo - echo $yellow'Could not find `fish_indent` in `$PATH`.'$normal - exit 127 - end - echo === Running "$green"fish_indent"$normal" - if set -l -q _flag_check - fish_indent --check -- $fish_files - or die "Fish files are not formatted correctly." - else - fish_indent -w -- $fish_files - end -end - -if set -q python_files[1] - if not type -q ruff - echo - echo $yellow'Please install `ruff` to style python'$normal - exit 127 - end - echo === Running "$green"ruff format"$normal" - if set -l -q _flag_check - ruff format --check $python_files - or die "Python files are not formatted correctly." - else - ruff format $python_files - end -end - -if test $all = yes; or set -q rust_files[1] - if not cargo fmt --version >/dev/null - echo - echo $yellow'Please install "rustfmt" to style Rust, e.g. via:' - echo "rustup component add rustfmt"$normal - exit 127 - end - - set -l edition_spec string match -r '^edition\s*=.*' - test "$($edition_spec , +} + +pub fn format(args: FormatArgs) { + 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; + } + if !args.force && !args.check { + match Command::new("git") + .args(["status", "--porcelain", "--short", "--untracked-files=all"]) + .output() + { + Ok(output) => { + if !output.stdout.is_empty() { + std::io::stdout().write_all(&output.stdout).unwrap(); + print!( + "You have uncommitted changes (listed above). Are you sure you want to format? (y/N): " + ); + std::io::stdout().flush().unwrap(); + let mut response = String::new(); + std::io::stdin().read_line(&mut response).unwrap(); + if response.trim_end() != "y" { + println!("Exiting without formatting."); + return; + } + } + } + Err(e) => { + if e.kind() == ErrorKind::NotFound { + println!( + "{YELLOW}warning: Did not find git, will proceed without checking for unstaged changes.{YELLOW:#}" + ) + } else { + panic!("Failed to run git status:\n{e}") + } + } + } + } + format_fish(&args); + format_python(&args); + format_rust(&args); +} + +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 + } + }) + .collect() +} +fn files_with_extension, I: IntoIterator>( + all_paths: I, + extension: &str, +) -> Vec { + let matcher = |p: &Path| p.extension().is_some_and(|e| e == extension); + get_matching_files(all_paths, matcher) +} + +fn run_formatter(formatter: &mut Command, name: &str) { + println!("=== Running {GREEN}{name}{GREEN:#}"); + match formatter.status() { + Ok(exit_status) => { + if !exit_status.success() { + panic!("{name:?}: Files are not formatted correctly."); + } + } + Err(e) => { + if e.kind() == std::io::ErrorKind::NotFound { + eprintln!( + "{YELLOW}Formatter not found: {name:?}. Skipping associated files.{YELLOW:#}" + ); + } else { + panic!("Error occurred while running {name:?}:\n{e}") + } + } + } +} + +fn format_fish(args: &FormatArgs) { + 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"]; + fish_paths.extend(files_with_extension( + fish_formatting_dirs + .iter() + .map(|dir_name| workspace_root.join(dir_name)), + "fish", + )); + }; + if fish_paths.is_empty() { + return; + } + // TODO: make `fish_indent` available as a Rust library function, to avoid needing a + // `fish_indent` binary in `$PATH`. + let mut formatter = Command::new("fish_indent"); + if args.check { + formatter.arg("--check"); + } else { + formatter.arg("-w"); + } + formatter.arg("--"); + formatter.args(fish_paths); + run_formatter(&mut formatter, "fish_indent"); +} + +fn format_python(args: &FormatArgs) { + 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"); + + if args.all { + python_files.push(fish_build_helper::workspace_root().to_owned()); + }; + if python_files.is_empty() { + return; + } + formatter.args(python_files); + run_formatter(&mut formatter, "ruff format"); +} + +fn format_rust(args: &FormatArgs) { + let rustfmt_status = Command::new("cargo") + .arg("fmt") + .arg("--version") + .stdout(Stdio::null()) + .status() + .unwrap(); + if !rustfmt_status.success() { + eprintln!( + "{YELLOW}Please install \"rustfmt\" to format Rust, e.g. via:\n\ + rustup component add rustfmt{YELLOW:#}" + ); + return; + } + if args.all { + let mut formatter = Command::new("cargo"); + formatter.arg("fmt"); + formatter.arg("--all"); + if args.check { + formatter.arg("--check"); + } + 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"); + } +} diff --git a/crates/xtask/src/lib.rs b/crates/xtask/src/lib.rs index f6c0d3f22..e2f23eb8a 100644 --- a/crates/xtask/src/lib.rs +++ b/crates/xtask/src/lib.rs @@ -1,5 +1,7 @@ use std::{ffi::OsStr, process::Command}; +pub mod format; + pub trait CommandExt { fn run_or_panic(&mut self); } diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index ee7b1c4e0..af405cd69 100644 --- a/crates/xtask/src/main.rs +++ b/crates/xtask/src/main.rs @@ -1,7 +1,7 @@ use clap::{Parser, Subcommand}; use fish_build_helper::as_os_strs; use std::{path::PathBuf, process::Command}; -use xtask::{CommandExt as _, cargo}; +use xtask::{CommandExt as _, cargo, format::FormatArgs}; #[derive(Parser)] #[command( @@ -18,6 +18,8 @@ struct Cli { enum Task { /// Run various checks on the repo. Check, + /// Format files or check if they are correctly formatted. + Format(FormatArgs), /// Build HTML docs HtmlDocs { /// Path to a fish_indent executable. If none is specified, fish_indent will be built. @@ -32,6 +34,7 @@ fn main() { let cli = Cli::parse(); match cli.task { Task::Check => run_checks(), + Task::Format(format_args) => xtask::format::format(format_args), Task::HtmlDocs { fish_indent } => build_html_docs(fish_indent), Task::ManPages => cargo(["build", "--package", "fish-build-man-pages"]), }