Skip to content

ci: add lint and format checks#2

Merged
tetsuo-cpp merged 1 commit intocanonfrom
ci/lint-checks
Feb 15, 2026
Merged

ci: add lint and format checks#2
tetsuo-cpp merged 1 commit intocanonfrom
ci/lint-checks

Conversation

@tetsuo-cpp
Copy link
Owner

@tetsuo-cpp tetsuo-cpp commented Feb 15, 2026

Summary

  • Add check-format CMake target that runs clang-format --dry-run --Werror
  • Search for versioned clang-format binary (e.g. clang-format-20) on Ubuntu
  • Add Ruff lint and format check step for Python code

Test plan

  • cmake --build build --target check-format passes locally
  • CI pipeline passes on this PR

Copy link
Owner Author

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Overview

Small, well-scoped PR that adds two CI steps:

  1. A check-format CMake target that verifies C++ files comply with clang-format (dry-run with --Werror)
  2. A Ruff lint + format check step for the Python code in gpu_test/

Also fixes the clang-format binary lookup on Ubuntu by searching for the versioned name (e.g. clang-format-20) first.

Analysis

Correctness — looks good overall.

  • clang-format binary search order (CMakeLists.txt): Searching for clang-format-${LLVM_VERSION_MAJOR} before clang-format is the right call for Ubuntu where apt installs versioned binaries. Ensures the format check uses the same LLVM version as the build, avoiding style drift.

  • CI step ordering (ci.yml): Format and lint checks run after build and test. This means a formatting-only failure still requires a full build cycle to surface. Consider moving these checks earlier or into a separate lightweight job — it doesn't need MLIR installed, so it could run on a bare image with just clang-format and uv. This would be faster and give clearer signal.

  • check-format target has no clang-format fallback (CMakeLists.txt): If clang-format isn't found, both format and check-format are silently unavailable. The CI step would then fail with a confusing "no rule to make target" error. Consider either:

    • Adding a message(WARNING ...) when clang-format isn't found, or
    • Explicitly installing clang-format-20 in the CI apt step to make the dependency explicit rather than relying on it being pulled in transitively by llvm-20-dev.
  • Ruff step: Using && to chain ruff check and ruff format --check is fine — if lint fails, format won't run, which is reasonable.

Suggestions

  1. Consider a separate CI job for lint/format — doesn't need MLIR, would be much faster and gives independent signal.
  2. Install clang-format explicitly in CI — add clang-format-${LLVM_VERSION} to the apt-get install line.
  3. Minor: file(GLOB_RECURSE ...) already recurses, so ** in the glob paths is redundant (though harmless).

Verdict

Clean, minimal PR that adds useful CI guardrails. Main suggestion is a separate faster job for lint checks. Otherwise, LGTM.

@tetsuo-cpp tetsuo-cpp merged commit 45425da into canon Feb 15, 2026
1 check passed
@tetsuo-cpp tetsuo-cpp deleted the ci/lint-checks branch February 15, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant