From c478a45217557725019a2c8a6a9c2e2a4900c491 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 27 Aug 2026 06:58:43 +0000 Subject: [PATCH] build: add format targets and a pre-commit format hook CI rejects unformatted C++ and Python, but nothing caught it before a push, so the first signal was a red PR. Three entry points now check the same things. tools/format.sh is the single implementation. CI calls it, the CMake format/format-check targets call it, and the pre-commit hook calls it. Sharing one script is the entire point: a local check that has drifted from CI is worse than no local check, because it is trusted. CI's inline clang-format and ruff blocks are replaced by a call to it; mypy stays inline, since type checking is not a formatting concern and is too slow for a commit hook. The hook checks staged content via `git show :file`, not the file on disk. A file can be staged in one state and edited further before committing, and checking the working tree would let a bad commit through -- or block a good one. Both directions are covered. It lives in .githooks rather than .git/hooks so it is version controlled, and the CMake configure step points core.hooksPath at it. That writes to the developer's local git config, so it is announced in the configure output rather than done silently, and -DINSTALL_GIT_HOOKS=OFF opts out. `git commit --no-verify` bypasses a single commit. clang-format is pinned to 18 to match CI and the dev container, with a warning if a different major version is found -- an unpinned binary reintroduces exactly the local/CI split this is meant to prevent. cmake/format.cmake is lowercase to match cmake/python_venv.cmake. CamelCase is the convention for CMake's own upstream and find-modules, not for a project's own. Co-Authored-By: Claude Opus 5 (1M context) --- .githooks/pre-commit | 27 +++++++ .github/workflows/ci.yml | 32 ++++---- CLAUDE.md | 15 +++- CMakeLists.txt | 5 ++ cmake/format.cmake | 62 ++++++++++++++++ tools/format.sh | 154 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 274 insertions(+), 21 deletions(-) create mode 100755 .githooks/pre-commit create mode 100644 cmake/format.cmake create mode 100755 tools/format.sh diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000..8ee9715 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# +# Rejects a commit whose staged content is not formatted the way CI expects. +# +# Installed by pointing core.hooksPath at this directory, which the CMake +# configure step does automatically (-DINSTALL_GIT_HOOKS=OFF opts out). Unlike +# .git/hooks, this directory is version controlled, so the hook arrives with a +# clone instead of relying on everyone remembering to copy it in. +# +# All the logic lives in tools/format.sh, which CI and the CMake targets also +# call. A hook with its own copy of the rules is a hook that eventually +# disagrees with CI. +# +# Bypass with `git commit --no-verify` when you genuinely need to. + +set -euo pipefail + +repo_root="$(git rev-parse --show-toplevel)" +formatter="${repo_root}/tools/format.sh" + +# Not an error: a branch predating the script, or a partial checkout. Refusing +# to commit because the checker is missing would be worse than not checking. +if [[ ! -x "${formatter}" ]]; then + exit 0 +fi + +exec "${formatter}" --staged diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 65572ae..14695df 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,16 +34,8 @@ jobs: # Pin to clang-format-18 to match the version in the dev container # (Dockerfile aliases clang-format -> clang-format-18). An unpinned # clang-format can disagree with local formatting. - - name: Check C++ formatting - run: | - sudo apt-get update && sudo apt-get install -y clang-format-18 - - if ! find src \( -name '*.cpp' -o -name '*.hpp' \) -print0 \ - | xargs -r0 clang-format-18 --dry-run --Werror; then - echo "::error::C++ formatting errors. Fix with:" - echo " find src \( -name '*.cpp' -o -name '*.hpp' \) | xargs clang-format -i" - exit 1 - fi + - name: Install clang-format + run: sudo apt-get update && sudo apt-get install -y clang-format-18 # uv resolves ruff/mypy/Python from py/host-emulator/uv.lock and # .python-version, so CI runs the exact versions the container does. @@ -53,19 +45,21 @@ jobs: enable-cache: true cache-dependency-glob: py/host-emulator/uv.lock - - name: Lint and type-check Python - working-directory: py/host-emulator + # The same script the pre-commit hook and the `format-check` CMake target + # run. Sharing one implementation is the point: a local check that has + # drifted from CI is worse than no local check, because it is trusted. + - name: Check formatting run: | - if ! uv run --frozen ruff check .; then - echo "::error::Python lint errors. Fix with: uv run ruff check --fix ." - exit 1 - fi - - if ! uv run --frozen ruff format --check .; then - echo "::error::Python formatting errors. Fix with: uv run ruff format ." + if ! tools/format.sh --check; then + echo "::error::Formatting errors. Fix with: tools/format.sh --fix" exit 1 fi + # Not part of format.sh: type checking is not a formatting concern, and is + # too slow to belong in a commit hook. + - name: Type-check Python + working-directory: py/host-emulator + run: | if ! uv run --frozen mypy src; then echo "::error::Python type errors. Reproduce with: uv run mypy src" exit 1 diff --git a/CLAUDE.md b/CLAUDE.md index bfbaf2b..a7d82c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,8 +25,19 @@ ctest --preset=host-debug -R host_emulator_test cd py/host-emulator && uv run pytest tests/test_blinky.py -v \ --blinky=../../build/host/bin/Debug/blinky -# Python lint / type-check -cd py/host-emulator && uv run ruff check . && uv run mypy src +# Formatting (C++ and Python). CI, the pre-commit hook, and these targets all +# call tools/format.sh, so they cannot disagree about what "formatted" means. +tools/format.sh --fix # Reformat in place +tools/format.sh --check # Verify, exactly as CI does +cmake --build build/host --target format # Same, via CMake +cmake --build build/host --target format-check + +# The pre-commit hook is installed by the CMake configure step, which points +# core.hooksPath at .githooks. Opt out with -DINSTALL_GIT_HOOKS=OFF; bypass a +# single commit with `git commit --no-verify`. + +# Python type-check (not covered by format.sh - types are not formatting) +cd py/host-emulator && uv run mypy src # Cross-compile for ARM - not yet functional. Presets and toolchain files exist, # but src/libs/mcu/CMakeLists.txt does add_subdirectory(${EMBEDDED_CPP_MCU}) and diff --git a/CMakeLists.txt b/CMakeLists.txt index 63e1cf2..69fc388 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -115,6 +115,11 @@ if(CMAKE_PRESET STREQUAL "arm-cm7") FetchContent_MakeAvailable(stm32cubef7) endif() +# `format` / `format-check` targets, and the pre-commit hook. Included for every +# preset: formatting is not host-specific, and a cross-compiling developer +# should get the same guard rails. +include("${PROJECT_SOURCE_DIR}/cmake/format.cmake") + clang_tidy("-header-filter=${CMAKE_CURRENT_SOURCE_DIR}/src/.*}") add_subdirectory(src) add_subdirectory(test) diff --git a/cmake/format.cmake b/cmake/format.cmake new file mode 100644 index 0000000..8de2356 --- /dev/null +++ b/cmake/format.cmake @@ -0,0 +1,62 @@ +# Formatting targets, and installation of the pre-commit hook. +# +# Both delegate to tools/format.sh rather than reimplementing the commands, so +# `cmake --build ... --target format-check`, the pre-commit hook, and CI are +# guaranteed to agree. Anything that duplicates the rules eventually disagrees +# with CI, which is the failure mode this file exists to prevent. + +set(FORMAT_SCRIPT "${CMAKE_SOURCE_DIR}/tools/format.sh") + +if(NOT EXISTS "${FORMAT_SCRIPT}") + message(WARNING "tools/format.sh not found; format targets unavailable") + return() +endif() + +add_custom_target(format + COMMAND "${FORMAT_SCRIPT}" --fix + WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" + COMMENT "Reformatting C++ and Python sources in place" + USES_TERMINAL + VERBATIM +) + +add_custom_target(format-check + COMMAND "${FORMAT_SCRIPT}" --check + WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" + COMMENT "Checking formatting (same checks as CI)" + USES_TERMINAL + VERBATIM +) + +# Install the pre-commit hook by pointing core.hooksPath at the version +# controlled .githooks directory. +# +# This writes to the developer's local git config, which is why it is announced +# rather than done silently, and why it is opt-out. It is skipped outside a git +# work tree so that tarball builds and CI checkouts are unaffected. +option(INSTALL_GIT_HOOKS "Point core.hooksPath at .githooks during configure" ON) + +if(INSTALL_GIT_HOOKS AND EXISTS "${CMAKE_SOURCE_DIR}/.git") + find_program(GIT_EXECUTABLE git) + if(GIT_EXECUTABLE) + execute_process( + COMMAND "${GIT_EXECUTABLE}" config --get core.hooksPath + WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" + OUTPUT_VARIABLE current_hooks_path + OUTPUT_STRIP_TRAILING_WHITESPACE + ERROR_QUIET + ) + if(NOT current_hooks_path STREQUAL ".githooks") + execute_process( + COMMAND "${GIT_EXECUTABLE}" config core.hooksPath .githooks + WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" + RESULT_VARIABLE hooks_result + ERROR_QUIET + ) + if(hooks_result EQUAL 0) + message(STATUS "Set git core.hooksPath to .githooks (pre-commit format check)") + message(STATUS " Opt out with -DINSTALL_GIT_HOOKS=OFF; bypass once with git commit --no-verify") + endif() + endif() + endif() +endif() diff --git a/tools/format.sh b/tools/format.sh new file mode 100755 index 0000000..14fe7ab --- /dev/null +++ b/tools/format.sh @@ -0,0 +1,154 @@ +#!/usr/bin/env bash +# +# Single source of truth for formatting. CI, the CMake `format`/`format-check` +# targets, and the pre-commit hook all call this script, so the three cannot +# drift apart -- which is the whole point. A local "it's formatted" that CI +# disagrees with is worse than no check at all. +# +# Usage: +# tools/format.sh --check Verify the whole tree. What CI runs. +# tools/format.sh --fix Reformat in place, and apply safe lint fixes. +# tools/format.sh --staged Verify only staged content. What the hook runs. +# +# Covers what can be fixed automatically: clang-format for C++, ruff format and +# ruff's fixable lint rules for Python. Type checking (mypy) is deliberately not +# here -- it is not a formatting concern and is too slow for a commit hook. + +set -uo pipefail + +readonly REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly PY_DIR="${REPO_ROOT}/py/host-emulator" + +# Pinned to match .github/workflows/ci.yml and the dev container, which aliases +# clang-format -> clang-format-18. Versions disagree about formatting, so an +# unpinned binary produces exactly the local/CI split this script exists to +# prevent. +readonly REQUIRED_CLANG_FORMAT_MAJOR=18 + +red() { printf '\033[31m%s\033[0m\n' "$*"; } +green() { printf '\033[32m%s\033[0m\n' "$*"; } +bold() { printf '\033[1m%s\033[0m\n' "$*"; } + +find_clang_format() { + local candidate + for candidate in "clang-format-${REQUIRED_CLANG_FORMAT_MAJOR}" clang-format; do + if command -v "${candidate}" >/dev/null 2>&1; then + local version + version="$("${candidate}" --version | grep -oE '[0-9]+' | head -1)" + if [[ "${version}" != "${REQUIRED_CLANG_FORMAT_MAJOR}" ]]; then + red "warning: ${candidate} is version ${version}, expected ${REQUIRED_CLANG_FORMAT_MAJOR}." >&2 + red " Formatting may disagree with CI." >&2 + fi + echo "${candidate}" + return 0 + fi + done + red "error: clang-format not found (want clang-format-${REQUIRED_CLANG_FORMAT_MAJOR})" >&2 + return 1 +} + +cpp_sources() { + find "${REPO_ROOT}/src" \( -name '*.cpp' -o -name '*.hpp' \) -print0 +} + +check_all() { + local clang_format status=0 + clang_format="$(find_clang_format)" || return 1 + + bold "Checking C++ formatting (${clang_format})" + if ! cpp_sources | xargs -r0 "${clang_format}" --dry-run --Werror; then + red "C++ formatting errors. Fix with: tools/format.sh --fix" + status=1 + fi + + bold "Checking Python formatting and lint (ruff)" + if ! (cd "${PY_DIR}" && uv run --frozen ruff format --check .); then + red "Python formatting errors. Fix with: tools/format.sh --fix" + status=1 + fi + if ! (cd "${PY_DIR}" && uv run --frozen ruff check .); then + red "Python lint errors. Fix with: tools/format.sh --fix" + status=1 + fi + + [[ ${status} -eq 0 ]] && green "All formatting checks passed." + return ${status} +} + +fix_all() { + local clang_format + clang_format="$(find_clang_format)" || return 1 + + bold "Formatting C++ (${clang_format})" + cpp_sources | xargs -r0 "${clang_format}" -i + + bold "Formatting Python and applying safe lint fixes (ruff)" + (cd "${PY_DIR}" && uv run --frozen ruff format .) + # --fix applies only rules ruff considers safe; anything else still needs a + # human, and --check will still fail on it. + (cd "${PY_DIR}" && uv run --frozen ruff check --fix .) + + green "Done. Review the changes before committing." +} + +# Checks the *staged* content, not the working tree. A file can be staged in one +# state and edited further on disk; committing the staged version while checking +# the file on disk would pass a commit that CI then rejects. +check_staged() { + local clang_format status=0 file staged_files + staged_files="$(git diff --cached --name-only --diff-filter=ACMR)" + [[ -z "${staged_files}" ]] && return 0 + + local cpp_failed=() py_failed=() + + while IFS= read -r file; do + [[ -z "${file}" ]] && continue + case "${file}" in + src/*.cpp|src/*.hpp|src/**/*.cpp|src/**/*.hpp) + if [[ -z "${clang_format:-}" ]]; then + clang_format="$(find_clang_format)" || return 1 + fi + if ! git show ":${file}" \ + | "${clang_format}" --assume-filename="${file}" --dry-run --Werror \ + >/dev/null 2>&1; then + cpp_failed+=("${file}") + status=1 + fi + ;; + *.py) + if ! git show ":${file}" \ + | (cd "${PY_DIR}" && uv run --frozen ruff format --check \ + --stdin-filename "${file}" -) >/dev/null 2>&1; then + py_failed+=("${file}") + status=1 + elif ! git show ":${file}" \ + | (cd "${PY_DIR}" && uv run --frozen ruff check \ + --stdin-filename "${file}" -) >/dev/null 2>&1; then + py_failed+=("${file}") + status=1 + fi + ;; + esac + done <<< "${staged_files}" + + if [[ ${status} -ne 0 ]]; then + red "Formatting problems in staged files:" + for file in "${cpp_failed[@]:-}" "${py_failed[@]:-}"; do + [[ -n "${file}" ]] && printf ' %s\n' "${file}" + done + printf '\n' + printf ' Fix: tools/format.sh --fix && git add -u\n' + printf ' Bypass: git commit --no-verify\n' + fi + return ${status} +} + +case "${1:---check}" in + --check) check_all ;; + --fix) fix_all ;; + --staged) check_staged ;; + *) + red "usage: $0 [--check|--fix|--staged]" >&2 + exit 2 + ;; +esac