From d1afb1fed6b2ea498b942b4db7c4bd46e62ca715 Mon Sep 17 00:00:00 2001 From: HanifKoh <76276251+HanifKoh@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:16:09 +0800 Subject: [PATCH] Use a System clang-tidy When Available and Make --fix Converge in One Pass (#16199) * Use a System clang-tidy When Available and Make --fix Converge in One Pass scripts/run_clang_tidy.sh only looked at CLANG_TIDY and the venv it creates, so a clang-tidy already on the system was never used. It is now the first choice: the pinned version outright, another version after a prompt that says results may differ slightly from CI, which -y and an existing pinned venv skip. Two problems in clang_tidy_diff.py made --fix need several runs and still leave the plain check failing: - A deleted #include orphans uses on unchanged lines. The plain check runs such a file whole and reports them, but --fix kept the line filter to the changed lines, so they were never fixed. Fix mode now runs the file whole first and then fixes exactly the changed lines plus the lines that run found wanting, so unrelated lines are still never rewritten. - clang-tidy exits non-zero for the findings it just fixed, so every fixed file was reported as failed and the user ran --fix again to see what was left. A file --fix changed is now checked again and the fixed files are listed separately from what --fix could not add. CI runs the script without --fix and is unchanged. * Keep the a/ b/ Diff Prefixes Whatever the User's Git Config Says parse_diff recognises a changed file by its +++ b/ header. With diff.noprefix or diff.mnemonicPrefix set, git prints +++ src/x.cpp or +++ w/src/x.cpp instead, every file was dropped, and the local check reported no changed C++ lines. The diff is now asked for the a/ and b/ prefixes outright, which overrides both settings. * Warn When No Remote Points at OrcaSlicer/OrcaSlicer Without one, run_clang_tidy.sh compares against origin/main. When origin is a fork whose main already holds the commits, the check finds nothing and says so, without hinting at why. The script now names the base it fell back to and how to point it at the upstream repository. --- scripts/clang_tidy_diff.py | 59 ++++++++++++++++++++------- scripts/run_clang_tidy.sh | 50 ++++++++++++++++++----- scripts/tests/test_clang_tidy_diff.py | 57 ++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 24 deletions(-) diff --git a/scripts/clang_tidy_diff.py b/scripts/clang_tidy_diff.py index b0fec730b2..130e4661df 100644 --- a/scripts/clang_tidy_diff.py +++ b/scripts/clang_tidy_diff.py @@ -8,6 +8,9 @@ include) fails wherever the error is. Headers are compiled on their own, and fail only on errors in their changed lines. Deleting an #include also fails on every use, changed or not, that now lacks the header it provided. +With -- --fix, clang-tidy adds the missing includes, on those lines only, and +each file it changed is checked again so that only what remains is reported. + The checks come from .clang-tidy at the repository root. The compile database must come from a configure with SLIC3R_PCH=OFF, or the precompiled header hides missing includes. @@ -102,9 +105,10 @@ def is_checked(path): def changed_files(merge_base): # Against the working tree, so a local run covers uncommitted edits too. - # core.quotePath=false keeps a non-ASCII path unquoted, so parse_diff sees its b/ prefix. + # core.quotePath=false keeps a non-ASCII path unquoted, and the explicit prefixes + # override diff.noprefix and diff.mnemonicPrefix, so parse_diff sees its b/ prefix. diff = subprocess.run(["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-color", "--no-ext-diff", - "--diff-filter=AMR", merge_base], + "--src-prefix=a/", "--dst-prefix=b/", "--diff-filter=AMR", merge_base], check=True, capture_output=True, **UTF8).stdout return {path: change for path, change in parse_diff(diff).items() if is_checked(path)} @@ -201,13 +205,19 @@ def errors_alone_at(revision, clang_tidy, build_dir, path): def check_file(clang_tidy, build_dir, merge_base, path, change, extra_args): - """Run clang-tidy on one file and return (failed, output, failing diagnostics).""" + """Run clang-tidy on one file and return (failed, output, failing diagnostics, fixed).""" + fixing = any(arg.startswith("--fix") for arg in extra_args) + if fixing: + with open(path, "rb") as f: + before = f.read() # A deleted include can orphan uses on unchanged lines, so such a file is - # checked whole and the findings narrowed here. --fix keeps the line filter - # so it never rewrites unrelated code. - whole = bool(change.removed_includes) and not extra_args + # checked whole and the findings narrowed here. --fix keeps a line filter so + # it never rewrites unrelated code, which for such a file means a second, + # fixing run limited to the lines the first one found wanting. + whole = bool(change.removed_includes) returncode, output, diagnostics = run_clang_tidy(clang_tidy, build_dir, path, - None if whole else change.lines, extra_args) + None if whole else change.lines, + [] if whole else extra_args) real = os.path.realpath(path) def introduced(d): @@ -223,14 +233,25 @@ def check_file(clang_tidy, build_dir, merge_base, path, change, extra_args): if errors and change.removed_includes: with open(path, encoding="utf-8") as f: text = f.read() - before = errors_alone_at(merge_base, clang_tidy, build_dir, path) + before_sites = errors_alone_at(merge_base, clang_tidy, build_dir, path) failing += [d for d in errors if d not in failing - and (error_sites([d], text) - before)] - return bool(failing), output, failing - if whole: + and (error_sites([d], text) - before_sites)] + failed = bool(failing) + elif whole: failing = [d for d in diagnostics if d.is_compile_error or introduced(d)] - return bool(failing), output, failing - return returncode != 0, output, diagnostics + failed = bool(failing) + else: + failing, failed = diagnostics, returncode != 0 + if not fixing: + return failed, output, failing, False + if whole and failing: + lines = change.lines + [[d.line, d.line] for d in failing if os.path.realpath(d.file) == real] + run_clang_tidy(clang_tidy, build_dir, path, lines, extra_args) + with open(path, "rb") as f: + if f.read() == before: + return failed, output, failing, False + # Checked again, so what is reported is what the fixes left. + return check_file(clang_tidy, build_dir, merge_base, path, change, [])[:3] + (True,) def main(): @@ -263,11 +284,14 @@ def main(): annotate = os.environ.get("GITHUB_ACTIONS") == "true" root = os.getcwd() + os.sep failed = [] + fixed = [] with ThreadPoolExecutor(max_workers=args.jobs) as pool: jobs = {path: pool.submit(check_file, args.clang_tidy, args.build_dir, merge_base, path, change, args.extra_args) for path, change in todo} for path, job in jobs.items(): - file_failed, output, diagnostics = job.result() + file_failed, output, diagnostics, file_fixed = job.result() + if file_fixed: + fixed.append(path) if not file_failed: continue failed.append(path) @@ -282,6 +306,13 @@ def main(): if len(diagnostics) > MAX_REPORTED: print(f"... and {len(diagnostics) - MAX_REPORTED} more") + if fixed: + print(f"\nAdded includes to {len(fixed)} file(s):") + for path in fixed: + print(f" {path}") + if failed and fixed: + print(f"\nclang-tidy still fails on {len(failed)} file(s); the findings above are what --fix could not add.") + return 1 if failed: print(f"\nclang-tidy failed on {len(failed)} file(s). Add the includes it names, or apply its " "suggestions locally with scripts/run_clang_tidy.sh --fix (scripts\\run_clang_tidy.ps1 -Fix on Windows).") diff --git a/scripts/run_clang_tidy.sh b/scripts/run_clang_tidy.sh index f8a935da38..70a723f3ac 100755 --- a/scripts/run_clang_tidy.sh +++ b/scripts/run_clang_tidy.sh @@ -6,8 +6,9 @@ # scripts/run_clang_tidy.sh --fix also add the missing includes it names # # It configures a separate build directory (build-tidy) without the precompiled -# header, installs the pinned clang-tidy into a virtual environment inside it, and -# runs scripts/clang_tidy_diff.py the way CI does. Uncommitted changes are checked too. +# header, uses the clang-tidy on your system or installs the pinned one into a +# virtual environment inside it, and runs scripts/clang_tidy_diff.py the way CI +# does. Uncommitted changes are checked too. set -euo pipefail @@ -23,7 +24,8 @@ Usage: scripts/run_clang_tidy.sh [options] deps/build/ on macOS) -j, --jobs N parallel clang-tidy runs (default: all cores) --fix apply clang-tidy's fixes (adds the missing includes) - -y, --yes install missing tools without asking + -y, --yes install missing tools without asking; another clang-tidy + version found on the system is then not offered -h, --help show this help EOF } @@ -136,12 +138,38 @@ REQUIREMENTS="$ROOT/scripts/clang_tidy_requirements.txt" PINNED=$(sed -n 's/^clang-tidy==//p' "$REQUIREMENTS") VENV="$BUILD_DIR/clang-tidy-venv" -if [ -n "${CLANG_TIDY:-}" ]; then +is_pinned() { + [ -x "$1" ] && "$1" --version 2>/dev/null | grep -q "version $PINNED" +} + +CLANG_TIDY="${CLANG_TIDY:-}" +if [ -n "$CLANG_TIDY" ]; then # Set by the caller: use it as is. - : + is_pinned "$CLANG_TIDY" || echo "Warning: $CLANG_TIDY is not clang-tidy $PINNED, so results may differ from CI." >&2 else + # One already on the system comes first: the pinned version outright, another + # version if the user accepts the difference. The pinned version is installed + # into a virtual environment otherwise. + INSTALLED="" + for candidate in $(command -v clang-tidy "clang-tidy-${PINNED%%.*}" || true) \ + "/usr/lib/llvm-${PINNED%%.*}/bin/clang-tidy" \ + "$(brew --prefix llvm 2>/dev/null || true)/bin/clang-tidy"; do + if is_pinned "$candidate"; then + CLANG_TIDY="$candidate" + break + fi + [ -z "$INSTALLED" ] && [ -x "$candidate" ] && INSTALLED="$candidate" + done + if [ -z "$CLANG_TIDY" ] && [ -n "$INSTALLED" ] && [ "$YES" = 0 ] && ! is_pinned "$VENV/bin/clang-tidy"; then + echo "Found $INSTALLED, which is $("$INSTALLED" --version | sed -n 's/.*version \([0-9.]*\).*/\1/p' | head -n 1), not the $PINNED CI uses, so results may differ slightly." + if ask "Use it anyway?"; then + CLANG_TIDY="$INSTALLED" + fi + fi +fi +if [ -z "$CLANG_TIDY" ]; then CLANG_TIDY="$VENV/bin/clang-tidy" - if [ ! -x "$CLANG_TIDY" ] || ! "$CLANG_TIDY" --version | grep -q "version $PINNED"; then + if ! is_pinned "$CLANG_TIDY"; then if ask "clang-tidy $PINNED (the version CI uses) is not installed. Install it into $VENV?"; then mkdir -p "$BUILD_DIR" if ! python3 -m venv "$VENV"; then @@ -158,9 +186,6 @@ else fi fi fi -if ! "$CLANG_TIDY" --version | grep -q "version $PINNED"; then - echo "Warning: $CLANG_TIDY is not clang-tidy $PINNED, so results may differ from CI." >&2 -fi # --- Dependencies ------------------------------------------------------------- @@ -217,7 +242,12 @@ cmake --build "$BUILD_DIR" --target git_commit_hash_header >/dev/null if [ -z "$BASE" ]; then REMOTE=$(git remote -v | awk '/github\.com[:\/]OrcaSlicer\/OrcaSlicer(\.git)? \(fetch\)/ { print $1; exit }') - REMOTE="${REMOTE:-origin}" + if [ -z "$REMOTE" ]; then + # Against a fork's main that already has the commits, nothing is checked. + echo "Warning: no remote points at github.com/OrcaSlicer/OrcaSlicer, so this compares against origin/main." >&2 + echo "If origin is your fork, add the upstream remote (git remote add upstream https://github.com/OrcaSlicer/OrcaSlicer.git) or pass --base." >&2 + REMOTE=origin + fi if [ "$FETCH" = 1 ]; then git fetch --quiet "$REMOTE" main fi diff --git a/scripts/tests/test_clang_tidy_diff.py b/scripts/tests/test_clang_tidy_diff.py index a9a1520d0f..a6551171b5 100644 --- a/scripts/tests/test_clang_tidy_diff.py +++ b/scripts/tests/test_clang_tidy_diff.py @@ -155,6 +155,8 @@ class TestSubprocessCalls(unittest.TestCase): diff = "+++ b/src/libslic3r/Über.cpp\n@@ -1,0 +2 @@\n+// 打印\n" files, call, _ = self.run_patched(clang_tidy_diff.changed_files, "base", stdout=diff) self.assertIn("core.quotePath=false", call.args[0]) + # Whatever diff.noprefix or diff.mnemonicPrefix a user has set. + self.assertIn("--dst-prefix=b/", call.args[0]) self.assertEqual(call.kwargs["encoding"], "utf-8") self.assertEqual(files["src/libslic3r/Über.cpp"].lines, [[2, 2]]) @@ -167,5 +169,60 @@ class TestSubprocessCalls(unittest.TestCase): self.assertEqual(call.kwargs["encoding"], "utf-8") +class TestCheckFileFix(unittest.TestCase): + """check_file with -- --fix: what clang-tidy is run on, and what is reported afterwards.""" + + def setUp(self): + self.dir = tempfile.TemporaryDirectory() + self.addCleanup(self.dir.cleanup) + self.path = os.path.join(self.dir.name, "Color.cpp") + with open(self.path, "w") as f: + f.write("int x;\n") + + def finding(self, line, include=""): + return clang_tidy_diff.Diagnostic(self.path, line, 1, "error", + 'no header providing "x" is directly included [misc-include-cleaner]', include) + + def check(self, change, results, fix_writes=None): + """Run check_file with run_clang_tidy answering from `results` in turn; the --fix run + rewrites the file with `fix_writes` when given. Returns (result, calls).""" + calls = [] + + def run(clang_tidy, build_dir, path, lines, extra_args): + calls.append((lines, extra_args)) + if "--fix" in extra_args and fix_writes is not None: + with open(path, "w") as f: + f.write(fix_writes) + return results[len(calls) - 1] + + with mock.patch.object(clang_tidy_diff, "run_clang_tidy", side_effect=run): + result = clang_tidy_diff.check_file("clang-tidy", "build", "base", self.path, change, ["--fix"]) + return result, calls + + def test_a_deleted_include_is_fixed_on_the_lines_it_orphaned_only(self): + change = clang_tidy_diff.FileChange(lines=[[4, 4]], removed_includes={"libslic3r/Point.hpp"}) + orphaned = self.finding(50, "") + unrelated = self.finding(60, "") + (failed, _, failing, fixed), calls = self.check( + change, [(1, "", [orphaned, unrelated]), (1, "", [orphaned]), (0, "", [])], fix_writes="#include \n") + self.assertEqual(calls, [(None, []), ([[4, 4], [50, 50]], ["--fix"]), (None, [])]) + self.assertEqual((failed, failing, fixed), (False, [], True)) + + def test_a_file_the_fix_did_not_change_keeps_its_findings(self): + change = clang_tidy_diff.FileChange(lines=[[4, 4]]) + error = clang_tidy_diff.Diagnostic(self.path, 4, 1, "error", "unknown type name 'Foo' [clang-diagnostic-error]") + (failed, _, failing, fixed), calls = self.check(change, [(1, "", [error])]) + self.assertEqual(calls, [([[4, 4]], ["--fix"])]) + self.assertEqual((failed, failing, fixed), (True, [error], False)) + + def test_a_changed_file_is_checked_again_and_reports_what_is_left(self): + change = clang_tidy_diff.FileChange(lines=[[4, 4]]) + error = clang_tidy_diff.Diagnostic(self.path, 4, 1, "error", "unknown type name 'Foo' [clang-diagnostic-error]") + (failed, _, failing, fixed), calls = self.check( + change, [(1, "", [self.finding(4, ""), error]), (1, "", [error])], fix_writes="#include \n") + self.assertEqual(calls, [([[4, 4]], ["--fix"]), ([[4, 4]], [])]) + self.assertEqual((failed, failing, fixed), (True, [error], True)) + + if __name__ == "__main__": unittest.main()