From c8edb29ddc0903918c7742decc53497eb180db6f Mon Sep 17 00:00:00 2001 From: HanifKoh <76276251+HanifKoh@users.noreply.github.com> Date: Mon, 5 Oct 2026 18:27:22 +0800 Subject: [PATCH] Gate Pull Requests on clang-tidy Missing-Include Checks (#16154) A Linux job configures without the precompiled header and runs clang-tidy over the C++ lines a pull request changes. The only check for now is misc-include-cleaner for missing includes; .clang-tidy is where further checks get enabled. scripts/run_clang_tidy.sh (Linux, macOS) and scripts/run_clang_tidy.ps1 (Windows) run the same check locally: the same configure, the clang-tidy version pinned in scripts/clang_tidy_requirements.txt, and the same comparison against OrcaSlicer's main. They offer to install what is missing, or print the command to do it by hand. --- .clang-tidy | 10 +- .github/workflows/clang_tidy.yml | 89 ++++++++ AGENTS.md | 1 + scripts/clang_tidy_diff.py | 285 ++++++++++++++++++++++++++ scripts/clang_tidy_requirements.txt | 1 + scripts/run_clang_tidy.ps1 | 225 ++++++++++++++++++++ scripts/run_clang_tidy.sh | 233 +++++++++++++++++++++ scripts/tests/test_clang_tidy_diff.py | 132 ++++++++++++ 8 files changed, 973 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/clang_tidy.yml create mode 100644 scripts/clang_tidy_diff.py create mode 100644 scripts/clang_tidy_requirements.txt create mode 100644 scripts/run_clang_tidy.ps1 create mode 100755 scripts/run_clang_tidy.sh create mode 100644 scripts/tests/test_clang_tidy_diff.py diff --git a/.clang-tidy b/.clang-tidy index 3f0aaa8bde..0acb13bb49 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,7 +1,11 @@ -# clang-tidy configuration. Only missing includes are reported for now: a file -# should include the header for every symbol it uses, not rely on the -# precompiled header or another header's includes. Run with --fix to add them. +# clang-tidy configuration, enforced by the clang-tidy CI job on the lines a pull +# request changes (scripts/clang_tidy_diff.py). Only missing includes are reported +# for now: a file should include the header for every symbol it uses, not rely on +# the precompiled header or another header's includes. Run with --fix to add them. +# Every check listed here gates pull requests, so enable a new one only once the +# code it flags on touched lines is reasonable to fix in passing. Checks: '-*,misc-include-cleaner' +WarningsAsErrors: '*' CheckOptions: # Missing includes only. Builds without the precompiled header break on these. misc-include-cleaner.UnusedIncludes: false diff --git a/.github/workflows/clang_tidy.yml b/.github/workflows/clang_tidy.yml new file mode 100644 index 0000000000..de268e64ca --- /dev/null +++ b/.github/workflows/clang_tidy.yml @@ -0,0 +1,89 @@ +name: clang-tidy + +# Runs clang-tidy, with the checks in .clang-tidy, over the C++ lines a pull +# request changes (scripts/clang_tidy_diff.py). The compile database is +# configured without the precompiled header, so code that only builds because +# the PCH supplied an include fails here. +# +# No paths filter: the job is a required check, and a workflow skipped by a +# paths filter leaves a required check pending forever. A PR with no C++ +# changes finishes after the first step. +on: + pull_request: + branches: + - main + - release/* + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + clang_tidy: + # Branch protection requires this check by name. Renaming the job disables + # that gate. + name: clang-tidy + runs-on: ${{ vars.SELF_HOSTED && 'orca-lnx-server' || 'ubuntu-24.04' }} + steps: + - name: Checkout + uses: actions/checkout@v7 + with: + lfs: 'false' + # The PR merge commit plus its first parent, the base it is diffed against. + fetch-depth: 2 + + - name: Look for changed C++ files + id: changes + run: | + if git diff --name-only HEAD^1 -- src tests | grep -qE '\.(cpp|cc|cxx|hpp|h|hxx)$'; then + echo "cpp=true" >> "$GITHUB_OUTPUT" + else + echo "No C++ changes under src/ or tests/." + fi + + # Parsing needs the dependency headers, not a build of this PR's deps/, so + # a PR that changes deps/ falls back to the newest cache main has. + - name: Restore cached deps + if: steps.changes.outputs.cpp == 'true' + uses: actions/cache/restore@v6 + with: + path: ${{ github.workspace }}/deps/build/OrcaSlicer_dep + key: linux-clang-cache-orcaslicer_deps-build-${{ hashFiles('deps/**') }} + restore-keys: linux-clang-cache-orcaslicer_deps-build- + fail-on-cache-miss: true + + - name: Apt-Install Dependencies + if: steps.changes.outputs.cpp == 'true' && !vars.SELF_HOSTED + uses: ./.github/actions/apt-install-deps + + - name: Install clang-tidy + if: steps.changes.outputs.cpp == 'true' + run: | + python3 -m venv "$RUNNER_TEMP/clang-tidy" + "$RUNNER_TEMP/clang-tidy/bin/pip" install --quiet -r scripts/clang_tidy_requirements.txt + + # DEP_BUILD_DIR is named outright: CMake would otherwise derive it from the build + # directory's name and look for the dependencies in deps/build-tidy. + - name: Configure without the precompiled header + if: steps.changes.outputs.cpp == 'true' + run: > + cmake -S . -B build-tidy -G Ninja + -DCMAKE_BUILD_TYPE=Release + -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ + -DCMAKE_EXPORT_COMPILE_COMMANDS=ON + -DSLIC3R_PCH=OFF -DORCA_TOOLS=ON -DBUILD_TESTS=ON + -DDEP_BUILD_DIR=${{ github.workspace }}/deps/build + + # The one header the build generates rather than the configure. + - name: Generate git_commit_hash.h + if: steps.changes.outputs.cpp == 'true' + run: cmake --build build-tidy --target git_commit_hash_header + + - name: Run clang-tidy on the changed lines + if: steps.changes.outputs.cpp == 'true' + run: > + python3 scripts/clang_tidy_diff.py -p build-tidy --base HEAD^1 + --clang-tidy "$RUNNER_TEMP/clang-tidy/bin/clang-tidy" -j "$(nproc)" diff --git a/AGENTS.md b/AGENTS.md index 3d0a61b303..07c4f5f211 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,7 @@ ctest --test-dir ./tests/fff_print -C Release - C++17, selective C++20. PascalCase classes, snake_case functions/variables - `#pragma once` for headers. Smart pointers and RAII preferred +- Include what you use: include the header for every symbol a file uses, and keep headers compilable on their own. Never rely on the precompiled header or a transitive include. The `clang-tidy` CI job enforces this on changed lines; run the same check locally with `scripts/run_clang_tidy.sh` (`scripts\run_clang_tidy.ps1` on Windows), which sets up everything it needs - Parallelization via TBB — be mindful of shared state - Always use `SetSizerAndFit(sizer)` instead of `SetSizer(sizer)` on top level window. Unless `SetSizer` must be called before the full layout is built, call `sizer->SetSizeHints(window)` afterwards in this case. diff --git a/scripts/clang_tidy_diff.py b/scripts/clang_tidy_diff.py new file mode 100644 index 0000000000..787d1a0632 --- /dev/null +++ b/scripts/clang_tidy_diff.py @@ -0,0 +1,285 @@ +#!/usr/bin/env python3 +"""Run clang-tidy on the C++ files changed since a base revision. + +Check findings are reported only on added or modified lines, so existing code +is not held to checks it predates. A changed source file that does not compile +(for example one that only built because the precompiled header supplied an +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. + +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. + + python3 scripts/clang_tidy_diff.py -p build-tidy --base origin/main +""" + +import argparse +import json +import os +import re +import subprocess +import sys +import tempfile +from collections import Counter +from concurrent.futures import ThreadPoolExecutor +from dataclasses import dataclass, field +from pathlib import PurePosixPath + +SOURCE_EXTENSIONS = {".cpp", ".cc", ".cxx"} +HEADER_EXTENSIONS = {".hpp", ".h", ".hxx"} +CHECKED_DIRS = ("src/", "tests/") +# Vendored code inside the checked directories. +EXCLUDED_DIRS = ("src/glad/", "tests/catch2/") + +# Per file. A deleted include can leave hundreds of follow-on errors. +MAX_REPORTED = 30 + +HUNK_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +DIAGNOSTIC_RE = re.compile(r"^(.+?):(\d+):(\d+): (error|warning): (.*)$") +FIX_MESSAGE_RE = re.compile(r"^\s+Message:\s+(['\"])(.*)\1$") +FIX_INCLUDE_RE = re.compile(r"^\s+ReplacementText:\s+['\"]#include ([^\\'\"]*)") +REMOVED_INCLUDE_RE = re.compile(r'^-\s*#\s*include\s*[<"]([^>"]+)[>"]') + + +@dataclass +class FileChange: + lines: list = field(default_factory=list) # [first, last] ranges of added lines + removed_includes: set = field(default_factory=set) # header names whose #include was deleted + + +@dataclass +class Diagnostic: + file: str + line: int + col: int + level: str + message: str + include: str = "" # header named by the fix, when the fix adds an #include + + @property + def is_compile_error(self): + return self.message.endswith("[clang-diagnostic-error]") + + def __str__(self): + return self.message + (f" (add #include {self.include})" if self.include else "") + + +def parse_diff(diff): + """Map each file in a `git diff -U0` to the lines it adds and the includes it deletes.""" + changes = {} + change = None + for line in diff.splitlines(): + if line.startswith("+++ "): + target = line[4:] + change = changes.setdefault(target[2:], FileChange()) if target.startswith("b/") else None + continue + if change is None: + continue + match = HUNK_RE.match(line) + if match: + first = int(match.group(1)) + count = int(match.group(2) or 1) + if count > 0: + change.lines.append([first, first + count - 1]) + continue + match = REMOVED_INCLUDE_RE.match(line) + if match: + change.removed_includes.add(match.group(1)) + return changes + + +def is_checked(path): + if not path.startswith(CHECKED_DIRS) or path.startswith(EXCLUDED_DIRS): + return False + return PurePosixPath(path).suffix in SOURCE_EXTENSIONS | HEADER_EXTENSIONS + + +def changed_files(merge_base): + # Against the working tree, so a local run covers uncommitted edits too. + diff = subprocess.run(["git", "diff", "-U0", "--no-color", "--no-ext-diff", "--diff-filter=AMR", merge_base], + check=True, capture_output=True, text=True).stdout + return {path: change for path, change in parse_diff(diff).items() if is_checked(path)} + + +def compiled_sources(build_dir): + with open(os.path.join(build_dir, "compile_commands.json"), encoding="utf-8") as f: + return {os.path.realpath(os.path.join(entry["directory"], entry["file"])) for entry in json.load(f)} + + +def run_clang_tidy(clang_tidy, build_dir, path, lines, extra_args): + """Run clang-tidy on one file, limited to `lines` unless it is None.""" + with tempfile.TemporaryDirectory() as tmp: + fixes = os.path.join(tmp, "fixes.yaml") + # The compile database comes from the system clang, which may know warning + # flags a newer clang-tidy has dropped. + cmd = [clang_tidy, "-p", build_dir, "--quiet", "--export-fixes=" + fixes, + "--extra-arg=-Wno-unknown-warning-option", "--extra-arg=-ferror-limit=0", *extra_args, path] + if lines is not None: + cmd.insert(1, "--line-filter=" + json.dumps([{"name": path, "lines": lines}])) + result = subprocess.run(cmd, capture_output=True, text=True) + suggestions = parse_suggested_includes(fixes) + output = result.stdout + result.stderr + return result.returncode, output, parse_diagnostics(output, suggestions) + + +def parse_suggested_includes(fixes_path): + """Map each message in an --export-fixes file to the header its fix includes.""" + if not os.path.exists(fixes_path): + return {} + suggestions = {} + message = None + with open(fixes_path, encoding="utf-8") as f: + for line in f: + match = FIX_MESSAGE_RE.match(line) + if match: + message = match.group(2).replace("''", "'") + continue + match = FIX_INCLUDE_RE.match(line) + if match and message: + suggestions[message] = match.group(1) + message = None + return suggestions + + +def parse_diagnostics(output, suggestions): + diagnostics = [] + for line in output.splitlines(): + match = DIAGNOSTIC_RE.match(line) + if match: + file, row, col, level, message = match.groups() + include = suggestions.get(re.sub(r" \[[^]]*\]$", "", message), "") + diagnostics.append(Diagnostic(file, int(row), int(col), level, message, include)) + return diagnostics + + +def in_ranges(line, ranges): + return any(first <= line <= last for first, last in ranges) + + +def names_removed_include(include, removed): + """Whether `include` () is one of the deleted includes, however it was spelled.""" + name = include.strip('<>"') + return bool(name) and any(name == r or name.endswith("/" + r) or r.endswith("/" + name) for r in removed) + + +def own_errors(diagnostics, path): + real = os.path.realpath(path) + return [d for d in diagnostics if d.is_compile_error and os.path.realpath(d.file) == real] + + +def error_sites(errors, text): + """Where each error points, as (source line, column): stable across edits to other lines, + unlike line numbers, and across a removed include, unlike clang's wording.""" + lines = text.splitlines() + return Counter((lines[d.line - 1].strip() if d.line <= len(lines) else "", d.col) for d in errors) + + +def errors_alone_at(revision, clang_tidy, build_dir, path): + """The error sites a header had when compiled on its own at `revision`.""" + shown = subprocess.run(["git", "show", f"{revision}:{path}"], capture_output=True, text=True) + if shown.returncode != 0: + return Counter() + # Beside the original, so its quoted includes resolve the same way. + p = PurePosixPath(path) + copy = str(p.with_name(f".{p.stem}.clang-tidy-base{p.suffix}")) + try: + with open(copy, "w", encoding="utf-8") as f: + f.write(shown.stdout) + _, _, diagnostics = run_clang_tidy(clang_tidy, build_dir, copy, None, []) + finally: + os.remove(copy) + return error_sites(own_errors(diagnostics, copy), shown.stdout) + + +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).""" + # 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 + returncode, output, diagnostics = run_clang_tidy(clang_tidy, build_dir, path, + None if whole else change.lines, extra_args) + real = os.path.realpath(path) + + def introduced(d): + return os.path.realpath(d.file) == real and ( + in_ranges(d.line, change.lines) or names_removed_include(d.include, change.removed_includes)) + + if PurePosixPath(path).suffix in HEADER_EXTENSIONS: + # Many existing headers only compile after what their includers include + # first, so a header is held to errors it introduces: on its changed + # lines, or anywhere a deleted include leaves it with new errors. + failing = [d for d in diagnostics if introduced(d)] + errors = own_errors(diagnostics, path) + 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) + failing += [d for d in errors if d not in failing + and (error_sites([d], text) - before)] + return bool(failing), output, failing + if 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 + + +def main(): + parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + parser.add_argument("-p", "--build-dir", required=True, help="directory holding compile_commands.json") + parser.add_argument("--base", default="origin/main", help="revision to diff against (default: origin/main)") + parser.add_argument("--clang-tidy", default="clang-tidy", help="clang-tidy executable") + parser.add_argument("-j", "--jobs", type=int, default=os.cpu_count()) + parser.add_argument("extra_args", nargs="*", help="passed to clang-tidy after --, e.g. -- --fix") + args = parser.parse_args() + + merge_base = subprocess.run(["git", "merge-base", args.base, "HEAD"], check=True, + capture_output=True, text=True).stdout.strip() + files = changed_files(merge_base) + sources = compiled_sources(args.build_dir) + todo = [] + for path, change in sorted(files.items()): + is_source = PurePosixPath(path).suffix in SOURCE_EXTENSIONS + if is_source and os.path.realpath(path) not in sources: + print(f"Skipping {path}: not compiled in this configuration") + elif change.lines or change.removed_includes: + todo.append((path, change)) + if not todo: + print("No changed C++ lines to check.") + return 0 + + print(f"Checking {len(todo)} file(s) with {args.clang_tidy}") + annotate = os.environ.get("GITHUB_ACTIONS") == "true" + root = os.getcwd() + os.sep + failed = [] + 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() + if not file_failed: + continue + failed.append(path) + print(f"\n==== {path}") + if not diagnostics: + print(output, end="") + for d in diagnostics[:MAX_REPORTED]: + file = d.file.removeprefix(root) + print(f"{file}:{d.line}:{d.col}: {d.level}: {d}") + if annotate: + print(f"::{d.level} file={file},line={d.line},col={d.col}::{d}") + if len(diagnostics) > MAX_REPORTED: + print(f"... and {len(diagnostics) - MAX_REPORTED} more") + + 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).") + return 1 + print("clang-tidy passed.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/clang_tidy_requirements.txt b/scripts/clang_tidy_requirements.txt new file mode 100644 index 0000000000..434fc94b78 --- /dev/null +++ b/scripts/clang_tidy_requirements.txt @@ -0,0 +1 @@ +clang-tidy==22.1.8 diff --git a/scripts/run_clang_tidy.ps1 b/scripts/run_clang_tidy.ps1 new file mode 100644 index 0000000000..228e84b33b --- /dev/null +++ b/scripts/run_clang_tidy.ps1 @@ -0,0 +1,225 @@ +<# +.SYNOPSIS +Runs the clang-tidy check that gates pull requests (.github/workflows/clang_tidy.yml) on +your branch. Windows. + +.DESCRIPTION +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. + +CI runs on Linux. Here clang-tidy also sees Windows-only code and MSVC's standard library, +so it can report findings CI does not; a change that passes here and on Linux passes CI. + +.EXAMPLE +powershell -ExecutionPolicy Bypass -File scripts\run_clang_tidy.ps1 +.EXAMPLE +powershell -ExecutionPolicy Bypass -File scripts\run_clang_tidy.ps1 -Fix +#> +param( + # Revision to compare against (default: main of the remote that points at + # OrcaSlicer/OrcaSlicer, else origin/main). + [string]$Base = "", + # Do not fetch that remote's main first. + [switch]$NoFetch, + # Build directory for the compile database. + [string]$BuildDir = "build-tidy", + # Dependency build directory (default: the deps\build* tree build_win.bat made). + [string]$DepsDir = "", + # x64 or arm64 (default: this machine's). + [string]$Arch = "", + # Parallel clang-tidy runs (default: all cores). + [int]$Jobs = 0, + # Apply clang-tidy's fixes (adds the missing includes). + [switch]$Fix, + # Install missing tools without asking. + [switch]$Yes +) + +$ErrorActionPreference = "Stop" +$Root = Split-Path -Parent $PSScriptRoot +Set-Location $Root +if (-not [System.IO.Path]::IsPathRooted($BuildDir)) { $BuildDir = Join-Path $Root $BuildDir } + +# Ask before installing anything. Without a console, or with "no", print the command +# instead so it can be run by hand. +function Ask([string]$Question) { + if ($Yes) { return $true } + if ([Console]::IsInputRedirected) { return $false } + $reply = Read-Host "$Question [y/N]" + return $reply -match '^[Yy]' +} + +function Has([string]$Command) { + return [bool](Get-Command $Command -ErrorAction SilentlyContinue) +} + +function Request-Install([string]$What, [string]$Command) { + Write-Host "Missing: $What" + if (Ask "Install it now with: $Command ?") { + & cmd /c $Command + if ($LASTEXITCODE -ne 0) { throw "Installing $What failed." } + Write-Host "Installed. Open a new terminal so PATH picks it up, then run this again." + } else { + Write-Host "To install it yourself, run:" + Write-Host " $Command" + } + exit 1 +} + +if (-not $Arch) { + if ($env:PROCESSOR_ARCHITECTURE -eq "ARM64") { $Arch = "arm64" } else { $Arch = "x64" } +} + +# --- System tools ------------------------------------------------------------- + +if (-not (Has "git")) { Request-Install "Git" "build_win.bat --install-deps" } + +# Python: the py launcher, else a python.exe that is not the Microsoft Store stub. +$Python = $null +if (Has "py") { + & py -3 --version *> $null + if ($LASTEXITCODE -eq 0) { $Python = @("py", "-3") } +} +if (-not $Python -and (Has "python")) { + & python --version *> $null + if ($LASTEXITCODE -eq 0) { $Python = @("python") } +} +if (-not $Python) { Request-Install "Python 3" "winget install -e --id Python.Python.3.12" } +$PyExe = $Python[0] +$PyArgs = @($Python | Select-Object -Skip 1) + +# Visual Studio provides the compiler, and its CMake component provides CMake and Ninja. +$Vswhere = Join-Path ${env:ProgramFiles(x86)} "Microsoft Visual Studio\Installer\vswhere.exe" +$VsPath = $null +if (Test-Path $Vswhere) { + $VsPath = & $Vswhere -latest -products * -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath +} +if (-not $VsPath) { Request-Install "Visual Studio with the C++ tools" "build_win.bat --install-vs buildtools" } + +# Load the developer environment, as build_win.bat does. +$HostArch = if ($env:PROCESSOR_ARCHITECTURE -eq "ARM64") { "arm64" } else { "x64" } +$VsDevCmd = Join-Path $VsPath "Common7\Tools\VsDevCmd.bat" +$envLines = & cmd /c "`"$VsDevCmd`" -arch=$Arch -host_arch=$HostArch -no_logo >nul && set" +foreach ($line in $envLines) { + $i = $line.IndexOf("=") + if ($i -gt 0) { Set-Item -Path ("env:" + $line.Substring(0, $i)) -Value $line.Substring($i + 1) } +} +if (-not (Has "cmake")) { Request-Install "CMake" "build_win.bat --install-deps" } +if (-not (Has "ninja")) { Request-Install "Ninja" "winget install -e --id Ninja-build.Ninja" } + +# --- clang-tidy --------------------------------------------------------------- + +$Requirements = Join-Path $Root "scripts\clang_tidy_requirements.txt" +$Pinned = ((Get-Content $Requirements | Select-String '^clang-tidy==(.+)$').Matches[0].Groups[1].Value).Trim() +$Venv = Join-Path $BuildDir "clang-tidy-venv" +$ClangTidy = $env:CLANG_TIDY + +function Test-Pinned([string]$Exe) { + if (-not $Exe -or -not (Test-Path $Exe)) { return $false } + return [bool]((& $Exe --version) -match "version $([regex]::Escape($Pinned))") +} + +if (-not $ClangTidy) { + $ClangTidy = Join-Path $Venv "Scripts\clang-tidy.exe" + if (-not (Test-Pinned $ClangTidy)) { + if (Ask "clang-tidy $Pinned (the version CI uses) is not installed. Install it into $Venv?") { + New-Item -ItemType Directory -Force -Path $BuildDir | Out-Null + & $PyExe @PyArgs -m venv $Venv + if ($LASTEXITCODE -ne 0) { throw "Could not create a Python virtual environment." } + & "$Venv\Scripts\python.exe" -m pip install --quiet --upgrade pip + & "$Venv\Scripts\python.exe" -m pip install --quiet -r $Requirements + if ($LASTEXITCODE -ne 0) { throw "Installing clang-tidy $Pinned failed." } + } else { + Write-Host "To install it yourself, run:" + Write-Host " $($Python -join ' ') -m venv $Venv" + Write-Host " $Venv\Scripts\python.exe -m pip install -r scripts\clang_tidy_requirements.txt" + exit 1 + } + } +} +if (-not (Test-Pinned $ClangTidy)) { + Write-Warning "$ClangTidy is not clang-tidy $Pinned, so results may differ from CI." +} + +# --- Dependencies ------------------------------------------------------------- + +# build_win.bat names the tree after the compiler and architecture: deps\build for cl, +# deps\build-clang for clang-cl, with -arm64 appended on ARM64. +$Suffix = if ($Arch -eq "arm64") { "-arm64" } else { "" } +$Compiler = "cl" +# A configured build directory remembers where its dependencies are. +$Cache = Join-Path $BuildDir "CMakeCache.txt" +if (-not $DepsDir -and (Test-Path $Cache)) { + $m = Select-String -Path $Cache -Pattern '^DEP_BUILD_DIR:[A-Z]*=(.+)$' | Select-Object -First 1 + if ($m) { $DepsDir = $m.Matches[0].Groups[1].Value } +} +if (-not $DepsDir) { + if (Test-Path "deps\build$Suffix\OrcaSlicer_dep") { + $DepsDir = "deps\build$Suffix" + } elseif (Test-Path "deps\build-clang$Suffix\OrcaSlicer_dep") { + $DepsDir = "deps\build-clang$Suffix" + } +} +if ($DepsDir -and ($DepsDir -match 'clang')) { $Compiler = "clang-cl" } +if (-not $DepsDir -or -not (Test-Path (Join-Path $DepsDir "OrcaSlicer_dep"))) { + $BuildDeps = "build_win.bat -d --arch $Arch" + Write-Host "OrcaSlicer's dependencies are not built." + if (Ask "Build them now with $BuildDeps? This takes a while.") { + & cmd /c $BuildDeps + if ($LASTEXITCODE -ne 0) { throw "Building the dependencies failed." } + $DepsDir = "deps\build$Suffix" + } else { + Write-Host "Build them with $BuildDeps, or point to an existing build with -DepsDir." + exit 1 + } +} +$DepsDir = (Resolve-Path $DepsDir).Path +if ($Compiler -eq "clang-cl" -and -not (Has "clang-cl")) { + Request-Install "clang-cl (the Visual Studio C++ Clang tools)" "build_win.bat --install-vs buildtools" +} + +# --- Compile database --------------------------------------------------------- + +# The same configure as CI, with Ninja so CMake writes compile_commands.json. Forward +# slashes keep CMake from reading a backslash as an escape. +$cmakeArgs = @("-S", ".", "-B", $BuildDir, "-G", "Ninja", "-DCMAKE_BUILD_TYPE=Release", + "-DCMAKE_C_COMPILER=$Compiler", "-DCMAKE_CXX_COMPILER=$Compiler", + "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-DSLIC3R_PCH=OFF", "-DORCA_TOOLS=ON", "-DBUILD_TESTS=ON", + "-DDEP_BUILD_DIR=$($DepsDir -replace '\\', '/')") +Write-Host "Configuring $BuildDir with $Compiler" +New-Item -ItemType Directory -Force -Path $BuildDir | Out-Null +$Log = Join-Path $BuildDir "configure.log" +& cmake @cmakeArgs *> $Log +if ($LASTEXITCODE -ne 0) { + Get-Content $Log -Tail 20 + throw "Configuring failed; the full log is in $Log." +} +& cmake --build $BuildDir --target git_commit_hash_header *> $null +if ($LASTEXITCODE -ne 0) { throw "Generating git_commit_hash.h failed." } + +# --- Base revision ------------------------------------------------------------ + +if (-not $Base) { + $Remote = "origin" + foreach ($line in (& git remote -v)) { + if ($line -match '^(\S+)\s+\S*github\.com[:/]OrcaSlicer/OrcaSlicer(\.git)?\s+\(fetch\)') { + $Remote = $Matches[1] + break + } + } + if (-not $NoFetch) { + & git fetch --quiet $Remote main + if ($LASTEXITCODE -ne 0) { throw "git fetch $Remote main failed." } + } + $Base = "$Remote/main" +} +Write-Host "Comparing against $Base" + +# --- Run ---------------------------------------------------------------------- + +$diffArgs = @("scripts\clang_tidy_diff.py", "-p", $BuildDir, "--base", $Base, "--clang-tidy", $ClangTidy) +if ($Jobs -gt 0) { $diffArgs += @("-j", "$Jobs") } +if ($Fix) { $diffArgs += @("--", "--fix") } +& $PyExe @PyArgs @diffArgs +exit $LASTEXITCODE diff --git a/scripts/run_clang_tidy.sh b/scripts/run_clang_tidy.sh new file mode 100755 index 0000000000..f8a935da38 --- /dev/null +++ b/scripts/run_clang_tidy.sh @@ -0,0 +1,233 @@ +#!/usr/bin/env bash +# Runs the clang-tidy check that gates pull requests (.github/workflows/clang_tidy.yml) +# on your branch, so its findings match what CI reports. Linux and macOS. +# +# scripts/run_clang_tidy.sh check your changes against OrcaSlicer's main +# 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. + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: scripts/run_clang_tidy.sh [options] + + -b, --base REV revision to compare against (default: main of the remote that + points at OrcaSlicer/OrcaSlicer, else origin/main) + --no-fetch do not fetch that remote's main first + -B, --build-dir DIR build directory for the compile database (default: build-tidy) + -d, --deps-dir DIR dependency build directory (default: deps/build on Linux, + 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 + -h, --help show this help +EOF +} + +ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +cd "$ROOT" + +BASE="" +FETCH=1 +BUILD_DIR=build-tidy +DEPS_DIR="" +JOBS="" +FIX=0 +YES=0 +while [ $# -gt 0 ]; do + case "$1" in + -b|--base) BASE="$2"; shift 2 ;; + --no-fetch) FETCH=0; shift ;; + -B|--build-dir) BUILD_DIR="$2"; shift 2 ;; + -d|--deps-dir) DEPS_DIR="$2"; shift 2 ;; + -j|--jobs) JOBS="$2"; shift 2 ;; + --fix) FIX=1; shift ;; + -y|--yes) YES=1; shift ;; + -h|--help) usage; exit 0 ;; + *) echo "Unknown option: $1" >&2; usage >&2; exit 2 ;; + esac +done + +case "$BUILD_DIR" in + /*) ;; + *) BUILD_DIR="$ROOT/$BUILD_DIR" ;; +esac + +OS=$(uname -s) +case "$OS" in + Linux) ;; + Darwin) ;; + *) echo "Unsupported system $OS. On Windows, use scripts/run_clang_tidy.ps1." >&2; exit 1 ;; +esac + +# Ask before installing anything. Without a terminal, or with "no", print the +# command instead so it can be run by hand. +ask() { + [ "$YES" = 1 ] && return 0 + [ -t 0 ] || return 1 + local reply + read -r -p "$1 [y/N] " reply + [[ "$reply" =~ ^[Yy] ]] +} + +# --- System tools ------------------------------------------------------------- + +missing=() +command -v git >/dev/null || missing+=(git) +command -v python3 >/dev/null || missing+=(python3) +command -v cmake >/dev/null || missing+=(cmake) +command -v ninja >/dev/null || missing+=(ninja) +if [ "$OS" = Linux ]; then + # CI builds its compile database with clang; another compiler's flags change what + # clang-tidy sees. + command -v clang >/dev/null && command -v clang++ >/dev/null || missing+=(clang) +fi + +if [ ${#missing[@]} -gt 0 ]; then + echo "Missing: ${missing[*]}" + install_cmd="" + if [ "$OS" = Darwin ]; then + pkgs=() + for m in "${missing[@]}"; do + case "$m" in + git|python3|cmake|ninja) pkgs+=("${m/python3/python}") ;; + esac + done + if command -v brew >/dev/null; then + install_cmd="brew install ${pkgs[*]}" + else + echo "Install Homebrew from https://brew.sh, then run: brew install ${pkgs[*]}" >&2 + exit 1 + fi + elif command -v apt-get >/dev/null; then + pkgs=() + for m in "${missing[@]}"; do + case "$m" in + ninja) pkgs+=(ninja-build) ;; + python3) pkgs+=(python3 python3-venv) ;; + *) pkgs+=("$m") ;; + esac + done + install_cmd="sudo apt-get install -y ${pkgs[*]}" + elif command -v dnf >/dev/null; then + install_cmd="sudo dnf install -y ${missing[*]/ninja/ninja-build}" + elif command -v pacman >/dev/null; then + install_cmd="sudo pacman -S --needed ${missing[*]/python3/python}" + else + echo "Install ${missing[*]} with your package manager and run this again." >&2 + exit 1 + fi + if ask "Install them now with: $install_cmd ?"; then + $install_cmd + else + echo "To install them yourself, run:" >&2 + echo " $install_cmd" >&2 + exit 1 + fi +fi + +# --- clang-tidy --------------------------------------------------------------- + +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 + # Set by the caller: use it as is. + : +else + CLANG_TIDY="$VENV/bin/clang-tidy" + if [ ! -x "$CLANG_TIDY" ] || ! "$CLANG_TIDY" --version | grep -q "version $PINNED"; 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 + echo "Could not create a Python virtual environment." >&2 + [ "$OS" = Linux ] && echo "On Debian or Ubuntu, install it with: sudo apt-get install -y python3-venv" >&2 + exit 1 + fi + "$VENV/bin/pip" install --quiet --upgrade pip + "$VENV/bin/pip" install --quiet -r "$REQUIREMENTS" + else + echo "To install it yourself, run:" >&2 + echo " python3 -m venv $VENV && $VENV/bin/pip install -r scripts/clang_tidy_requirements.txt" >&2 + exit 1 + 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 ------------------------------------------------------------- + +# A configured build directory remembers where its dependencies are. +if [ -z "$DEPS_DIR" ] && [ -f "$BUILD_DIR/CMakeCache.txt" ]; then + DEPS_DIR=$(sed -n 's/^DEP_BUILD_DIR:[A-Z]*=//p' "$BUILD_DIR/CMakeCache.txt") +fi +if [ "$OS" = Darwin ]; then + ARCH=$(uname -m) + DEPS_DIR="${DEPS_DIR:-deps/build/$ARCH}" + BUILD_DEPS_CMD="./build_release_macos.sh -d -a $ARCH" +else + DEPS_DIR="${DEPS_DIR:-deps/build}" + BUILD_DEPS_CMD="./build_linux.sh -d" +fi +case "$DEPS_DIR" in + /*) ;; + *) DEPS_DIR="$ROOT/$DEPS_DIR" ;; +esac + +if [ ! -d "$DEPS_DIR/OrcaSlicer_dep/usr/local" ]; then + echo "OrcaSlicer's dependencies are not built in $DEPS_DIR." + if ask "Build them now with $BUILD_DEPS_CMD? This takes a while."; then + $BUILD_DEPS_CMD + else + echo "Build them with $BUILD_DEPS_CMD, or point to an existing build with --deps-dir." >&2 + exit 1 + fi +fi + +# --- Compile database --------------------------------------------------------- + +# The same configure as CI. DEP_BUILD_DIR is named outright: CMake would otherwise +# derive it from the build directory's name. +cmake_args=(-S . -B "$BUILD_DIR" -G Ninja -DCMAKE_BUILD_TYPE=Release + -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSLIC3R_PCH=OFF -DORCA_TOOLS=ON -DBUILD_TESTS=ON + -DDEP_BUILD_DIR="$DEPS_DIR") +if [ "$OS" = Darwin ]; then + cmake_args+=(-DCMAKE_OSX_ARCHITECTURES="$ARCH" "-DCMAKE_IGNORE_PREFIX_PATH=/opt/local;/usr/local;/opt/homebrew") +else + cmake_args+=(-DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++) +fi +echo "Configuring $BUILD_DIR" +mkdir -p "$BUILD_DIR" +if ! cmake "${cmake_args[@]}" >"$BUILD_DIR/configure.log" 2>&1; then + tail -n 20 "$BUILD_DIR/configure.log" >&2 + echo "Configuring failed; the full log is in $BUILD_DIR/configure.log." >&2 + [ "$OS" = Linux ] && echo "Missing system libraries? Install them with: ./build_linux.sh -u" >&2 + exit 1 +fi +cmake --build "$BUILD_DIR" --target git_commit_hash_header >/dev/null + +# --- Base revision ------------------------------------------------------------ + +if [ -z "$BASE" ]; then + REMOTE=$(git remote -v | awk '/github\.com[:\/]OrcaSlicer\/OrcaSlicer(\.git)? \(fetch\)/ { print $1; exit }') + REMOTE="${REMOTE:-origin}" + if [ "$FETCH" = 1 ]; then + git fetch --quiet "$REMOTE" main + fi + BASE="$REMOTE/main" +fi +echo "Comparing against $BASE" + +# --- Run ---------------------------------------------------------------------- + +args=(-p "$BUILD_DIR" --base "$BASE" --clang-tidy "$CLANG_TIDY") +[ -n "$JOBS" ] && args+=(-j "$JOBS") +[ "$FIX" = 1 ] && args+=(-- --fix) +exec python3 scripts/clang_tidy_diff.py "${args[@]}" diff --git a/scripts/tests/test_clang_tidy_diff.py b/scripts/tests/test_clang_tidy_diff.py new file mode 100644 index 0000000000..f6b3abf8f9 --- /dev/null +++ b/scripts/tests/test_clang_tidy_diff.py @@ -0,0 +1,132 @@ +#!/usr/bin/env python3 +"""Tests for the diff handling in scripts/clang_tidy_diff.py (stdlib unittest, no +external deps). + +Run from the repo root: python -m unittest discover -s scripts/tests -v +""" + +import os +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) + +import clang_tidy_diff # noqa: E402 + +DIFF = """\ +diff --git a/src/libslic3r/Color.cpp b/src/libslic3r/Color.cpp +index 1111111..2222222 100644 +--- a/src/libslic3r/Color.cpp ++++ b/src/libslic3r/Color.cpp +@@ -3,0 +4 @@ ++#include +@@ -8 +8,0 @@ +-#include "libslic3r/Point.hpp" +@@ -20,2 +21,3 @@ void f() +- old(); +- old(); ++ a(); ++ b(); ++ c(); +@@ -40 +42,0 @@ void g() +- gone(); +diff --git a/src/libslic3r/New.hpp b/src/libslic3r/New.hpp +new file mode 100644 +--- /dev/null ++++ b/src/libslic3r/New.hpp +@@ -0,0 +1,2 @@ ++#pragma once ++int f(); +diff --git a/src/libslic3r/Old.cpp b/src/libslic3r/Renamed.cpp +similarity index 100% +rename from src/libslic3r/Old.cpp +rename to src/libslic3r/Renamed.cpp +""" + + +class TestParseChangedLines(unittest.TestCase): + def setUp(self): + self.changed = clang_tidy_diff.parse_diff(DIFF) + + def test_added_and_replaced_hunks_become_line_ranges(self): + self.assertEqual(self.changed["src/libslic3r/Color.cpp"].lines, [[4, 4], [21, 23]]) + + def test_deleted_includes_are_collected(self): + self.assertEqual(self.changed["src/libslic3r/Color.cpp"].removed_includes, {"libslic3r/Point.hpp"}) + self.assertEqual(self.changed["src/libslic3r/New.hpp"].removed_includes, set()) + + def test_new_file_covers_every_line(self): + self.assertEqual(self.changed["src/libslic3r/New.hpp"].lines, [[1, 2]]) + + def test_pure_rename_has_no_changed_lines(self): + self.assertNotIn("src/libslic3r/Renamed.cpp", self.changed) + + +class TestNamesRemovedInclude(unittest.TestCase): + def test_matches_however_the_include_was_spelled(self): + removed = {"Point.hpp", "cassert"} + self.assertTrue(clang_tidy_diff.names_removed_include("", removed)) + self.assertTrue(clang_tidy_diff.names_removed_include("", removed)) + self.assertTrue(clang_tidy_diff.names_removed_include('"Point.hpp"', {"libslic3r/Point.hpp"})) + + def test_other_headers_and_no_suggestion_do_not_match(self): + removed = {"libslic3r/Point.hpp"} + self.assertFalse(clang_tidy_diff.names_removed_include("", removed)) + self.assertFalse(clang_tidy_diff.names_removed_include("", removed)) + self.assertFalse(clang_tidy_diff.names_removed_include("", removed)) + + +class TestIsChecked(unittest.TestCase): + def test_project_sources_and_headers(self): + for path in ("src/libslic3r/Color.cpp", "src/slic3r/GUI/Tab.hpp", "tests/fff_print/test_flow.cpp", + "src/libslic3r/Format/bbs_3mf.h"): + self.assertTrue(clang_tidy_diff.is_checked(path), path) + + def test_vendored_code_other_languages_and_other_dirs(self): + for path in ("src/glad/src/gl.c", "src/glad/include/glad/gl.h", "tests/catch2/src/catch.hpp", + "deps_src/imgui/imgui.cpp", "src/slic3r/Utils/MacUtils.mm", "src/CMakeLists.txt"): + self.assertFalse(clang_tidy_diff.is_checked(path), path) + + +FIXES = """\ +--- +MainSourceFile: '/repo/src/libslic3r/Color.cpp' +Diagnostics: + - DiagnosticName: misc-include-cleaner + DiagnosticMessage: + Message: 'no header providing "assert" is directly included' + FilePath: '/repo/src/libslic3r/Color.cpp' + FileOffset: 1974 + Replacements: + - FilePath: '/repo/src/libslic3r/Color.cpp' + Offset: 39 + Length: 0 + ReplacementText: "#include \\n" + Level: Error + - DiagnosticName: misc-include-cleaner + DiagnosticMessage: + Message: 'no header providing "Slic3r::comExpert" is directly included' + FilePath: '/repo/src/libslic3r/Color.cpp' + FileOffset: 2148 + Replacements: [] + Level: Error +... +""" + + +class TestParseSuggestedIncludes(unittest.TestCase): + def test_maps_each_message_to_the_include_its_fix_inserts(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "fixes.yaml") + with open(path, "w", encoding="utf-8") as f: + f.write(FIXES) + self.assertEqual(clang_tidy_diff.parse_suggested_includes(path), + {'no header providing "assert" is directly included': ""}) + + def test_missing_file_means_no_suggestions(self): + self.assertEqual(clang_tidy_diff.parse_suggested_includes("/nonexistent/fixes.yaml"), {}) + + +if __name__ == "__main__": + unittest.main()