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()