From 12ae6a18dde86d634cf8f737e56df7030f3023d5 Mon Sep 17 00:00:00 2001 From: Leon Schuermann Date: Tue, 18 Jun 2024 14:34:32 -0400 Subject: [PATCH 1/2] Makefile: alias `format` to `cargo fmt`, add `format-check` command This removes the previous `make format` command which, in essence, runs some bash magic to achieve the exact same job that `cargo fmt` does, if its run within workspace instead of a particular crate. Besides this, many people's editors have a native rustfmt integration anyways, which makes this script and its prompt for unstaged files largely redundant. Instead, we replace it with the more useful `make format-check` and the associated `tools/check_format.sh`. This script does more than `cargo fmt --check` does, for instance also ensuring that no Rust source file contains tab characters (which rustfmt sometimes leaves behind). As part of `make prepush`, `check-format` is now more in line with the other jobs, such as `cargo check` or Clippy: it'll complain, but not touch any source code. To retain compatibility, we introduce a `make format` and `make fmt`, aliased to `cargo fmt` -- without prompting the user. This also removes an optimization that uses Make's logic to check for changed files in order to skip formatting. However, I doubt that this will meaningfully impact users (`make format-check` completes in about 2sec and does not take a meaningful amount of time compared to the other jobs executed as part of `prepush`). --- Makefile | 27 ++++++----- tools/check_format.sh | 53 +++++++++++++++++++++ tools/run_cargo_fmt.sh | 104 ----------------------------------------- 3 files changed, 68 insertions(+), 116 deletions(-) create mode 100755 tools/check_format.sh delete mode 100755 tools/run_cargo_fmt.sh diff --git a/Makefile b/Makefile index d10a28e0d7..e5be631827 100644 --- a/Makefile +++ b/Makefile @@ -52,9 +52,12 @@ usage: @echo " doc: Builds Tock documentation for all boards" @echo " stack: Prints a basic stack frame analysis for all boards" @echo " clean: Clean all builds" - @echo " format: Runs the rustfmt tool on all kernel sources" + @echo " format-check: Checks for formatting errors in kernel sources" @echo " list: Lists available boards" @echo + @echo "We also define the following aliases:" + @echo " format: cargo fmt" + @echo @echo "The make system also drives all continuous integration and testing:" @echo " $$(tput bold)prepush$$(tput sgr0): Fast checks to run before pushing changes upstream" @echo " ci-all: Run all continuous integration tests (possibly slow!)" @@ -175,14 +178,15 @@ clean: @echo "$$(tput bold)Clean ci-artifacts" && rm -rf tools/ci-artifacts .PHONY: fmt format -fmt format: tools/.format_fresh - $(call banner,Formatting complete) +fmt format: + $(call banner,Running \"cargo fmt\" -- for a complete format check run \"make format-check\") + cargo fmt -# Get a list of all rust source files (everything fmt operates on) -$(eval RUST_FILES_IN_TREE := $(shell (git ls-files | grep '\.rs$$') || find . -type f -name '*.rs')) -tools/.format_fresh: $(RUST_FILES_IN_TREE) - @./tools/run_cargo_fmt.sh $(TOCK_FORMAT_MODE) - @touch tools/.format_fresh +.PHONY: format-check +format-check: + $(call banner,Formatting checker) + @./tools/check_format.sh + $(call banner,Check for formatting complete) .PHONY: list list: @@ -215,7 +219,7 @@ ci-nosetup: # This is designed for developers, to be run often and before submitting code upstream. .PHONY: prepush prepush:\ - format\ + format-check\ ci-job-clippy\ ci-job-syntax\ licensecheck @@ -350,9 +354,8 @@ ci-runner-netlify:\ ### ci-runner-github-format jobs: .PHONY: ci-job-format -ci-job-format: licensecheck - $(call banner,CI-Job: Format Check) - @NOWARNINGS=true TOCK_FORMAT_MODE=diff $(MAKE) format +ci-job-format: licensecheck format-check + $(call banner,CI-Job: Format Check DONE) define ci_setup_markdown_toc $(call banner,CI-Setup: Install markdown-toc) diff --git a/tools/check_format.sh b/tools/check_format.sh new file mode 100755 index 0000000000..ff86171b66 --- /dev/null +++ b/tools/check_format.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 or the MIT License. +# SPDX-License-Identifier: Apache-2.0 OR MIT +# Copyright Tock Contributors 2023. +# +# Author: Leon Schuermann +# Author: Pat Pannuto +# Author: Brad Campbell +# +set -e + +# Verify that we're running in the base directory +if [ ! -x tools/check_format.sh ]; then + echo ERROR: $0 must be run from the tock repository root. + echo "" + exit 1 +fi + +set +e +let FAIL=0 +set -e + +# Run `cargo fmt --check` in the workspace root, which will check all +# crates in the workspace. +echo "Running \`cargo fmt --check\`..." +CARGO_FMT_CHECK_EXIT_CODE=0 +cargo fmt --check || CARGO_FMT_CHECK_EXIT_CODE=$? +if [[ $CARGO_FMT_CHECK_EXIT_CODE -ne 0 ]]; then + let FAIL=FAIL+1 +else + echo "\`cargo fmt --check\` suceeded." +fi +printf "\n" + +# Check for tab characters in Rust source files that haven't been +# removed by rustfmt +echo "Checking for Rust source files with tab characters..." +RUST_FILES_WITH_TABS="$(git grep --files-with-matches $'\t' -- '*.rs' || grep -lr --include '*.rs' $'\t' . || true)" +if [ "$RUST_FILES_WITH_TABS" != "" ]; then + echo "ERROR: The following files contain tab characters, please use spaces instead:" + echo "$RUST_FILES_WITH_TABS" | sed 's/^/ -> /' + let FAIL=FAIL+1 +else + echo "No Rust source file containing tab characters found." +fi + +if [[ $FAIL -ne 0 ]]; then + printf "\n" + echo "$(tput bold)Formatting errors.$(tput sgr0)" + echo "See above for details. You can try running \`cargo fmt\` to correct them." +fi +exit $FAIL diff --git a/tools/run_cargo_fmt.sh b/tools/run_cargo_fmt.sh deleted file mode 100755 index c6d12f8448..0000000000 --- a/tools/run_cargo_fmt.sh +++ /dev/null @@ -1,104 +0,0 @@ -#!/usr/bin/env bash - -# Licensed under the Apache License, Version 2.0 or the MIT License. -# SPDX-License-Identifier: Apache-2.0 OR MIT -# Copyright Tock Contributors 2023. -# -# Runs rustfmt on all subdirectories with a Cargo.toml file. Must be run from -# root Tock directory. Protects user from inadvertently overwriting files. -# -# Author: Pat Pannuto -# Author: Brad Campbell -# -set -e - -# Verify that we're running in the base directory -if [ ! -x tools/run_cargo_fmt.sh ]; then - echo ERROR: $0 must be run from the tock repository root. - echo "" - exit 1 -fi - -# Format overwrites changes, which is probably good, but it's nice to see -# what it has done -# -# `git status --porcelain` formats things for scripting -# | M changed file, unstaged -# |M changed file, staged (git add has run) -# |MM changed file, some staged and some unstaged changes (git add then changes) -# |?? untracked file -if [ "$1" != "diff" ]; then - if git status --porcelain | grep '^.M.*\.rs' -q; then - echo "$(tput bold)Warning: Formatting will overwrite files in place.$(tput sgr0)" - echo "While this is probably what you want, it's often useful to" - echo "stage all of your changes (git add ...) before format runs," - echo "just so you can double-check everything." - echo "" - echo "$(tput bold)git status:$(tput sgr0)" - git status - echo "" - read -p "Continue formatting with unstaged changes? [y/N] " response - if [[ ! ( "$(echo "$response" | tr :upper: :lower:)" == "y" ) ]]; then - exit 0 - fi - fi -fi - -set +e -let FAIL=0 -set -e - -# Get the list of files in the workspace to avoid formatting -# them twice -csplit Cargo.toml '/exclude = \[/' > /dev/null -rm xx01 - -# Find folders with Cargo.toml files in them and run `cargo fmt`. -for f in $(find . | grep Cargo.toml); do - if [ $f == './Cargo.toml' ]; then - printf "\rFormatting Workspace" - dir='.' - else - dir=$(dirname $f) - dir=${dir:2} # strip leading ./ - if grep -q '"'$dir'"' xx00; then - continue - fi - if [[ $dir == tools/qemu* ]]; then - continue - fi - - printf "\rFormatting %-$((39))s" $dir - fi - - pushd $dir > /dev/null - if [ "$1" == "diff" ]; then - # If diff mode, two-pass the check to make pretty-print work - if ! cargo-fmt -q -- --check; then - printf "<- Contains formatting errors!\n" - cargo-fmt -- --check || let FAIL=FAIL+1 - printf "\n" - fi - else - cargo-fmt - fi - popd > /dev/null -done -rm xx00 -printf "\rFormatting complete. %-$((39))s\n" "" - -# Check for tab characters in Rust source files that haven't been -# removed by rustfmt -RUST_FILES_WITH_TABS="$(git grep --files-with-matches $'\t' -- '*.rs' || grep -lr --include '*.rs' $'\t' . || true)" -if [ "$RUST_FILES_WITH_TABS" != "" ]; then - echo "ERROR: The following files contain tab characters, please use spaces instead:" - echo "$RUST_FILES_WITH_TABS" | sed 's/^/ -> /' - let FAIL=FAIL+1 -fi - -if [[ $FAIL -ne 0 ]]; then - echo - echo "$(tput bold)Formatting errors.$(tput sgr0)" - echo "See above for details" -fi -exit $FAIL From 5ee94cc9d33150ef54f6724af8f3dda755786d86 Mon Sep 17 00:00:00 2001 From: Leon Schuermann Date: Fri, 5 Jul 2024 13:54:49 -0400 Subject: [PATCH 2/2] tools/check_format.sh: add documentation Co-authored-by: Brad Campbell --- tools/check_format.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/check_format.sh b/tools/check_format.sh index ff86171b66..143a2f6ed5 100755 --- a/tools/check_format.sh +++ b/tools/check_format.sh @@ -4,6 +4,9 @@ # SPDX-License-Identifier: Apache-2.0 OR MIT # Copyright Tock Contributors 2023. # +# Runs `rustfmt --check` on the workspace and check for tabs in Rust files. +# Must be run from root Tock directory. +# # Author: Leon Schuermann # Author: Pat Pannuto # Author: Brad Campbell