From c1e6de7e4e1636a957772bb4d46d354d9b81b267 Mon Sep 17 00:00:00 2001 From: Kris Austin Date: Mon, 5 Oct 2026 11:35:03 -0500 Subject: [PATCH] Fix the clang-tidy check on Windows and for unusual file paths (#16163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_clang_tidy.ps1 had not been run on Windows before. - Run native commands through Invoke-Quiet. Under $ErrorActionPreference = "Stop", Windows PowerShell made CMake's first stderr line fatal, so configure always failed. - Pass the --line-filter name with native separators. clang-tidy matches it against the end of the file's native path, so on Windows every misc-include-cleaner finding was dropped. - Decode subprocess output as UTF-8 and let stdout replace characters it cannot encode. A changed line with text such as 打印 crashed the script under cp1252. - Check VCToolsInstallDir and WindowsSdkDir in VsDevCmd's output before applying it, so a failure names the command to run and leaves the calling shell untouched. - Log the git_commit_hash_header build, use -LiteralPath for logs, and hide VsDevCmd's stderr as build_win.bat does. On every platform, git quotes non-ASCII paths and appends a tab to a +++ header whose path contains a space, and parse_diff dropped both. changed_files and the workflow's changed-files step now pass core.quotePath=false, and parse_diff strips the tab. --- .github/workflows/clang_tidy.yml | 2 +- scripts/clang_tidy_diff.py | 21 ++++++++++----- scripts/run_clang_tidy.ps1 | 38 +++++++++++++++++++++----- scripts/tests/test_clang_tidy_diff.py | 39 +++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 14 deletions(-) diff --git a/.github/workflows/clang_tidy.yml b/.github/workflows/clang_tidy.yml index de268e64ca..2f27a0eaff 100644 --- a/.github/workflows/clang_tidy.yml +++ b/.github/workflows/clang_tidy.yml @@ -38,7 +38,7 @@ jobs: - 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 + if git -c core.quotePath=false 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/." diff --git a/scripts/clang_tidy_diff.py b/scripts/clang_tidy_diff.py index 787d1a0632..b0fec730b2 100644 --- a/scripts/clang_tidy_diff.py +++ b/scripts/clang_tidy_diff.py @@ -36,6 +36,9 @@ EXCLUDED_DIRS = ("src/glad/", "tests/catch2/") # Per file. A deleted include can leave hundreds of follow-on errors. MAX_REPORTED = 30 +# Subprocess output is UTF-8 whatever the locale, which is cp1252 on Windows. +UTF8 = {"encoding": "utf-8", "errors": "replace"} + 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$") @@ -72,7 +75,8 @@ def parse_diff(diff): change = None for line in diff.splitlines(): if line.startswith("+++ "): - target = line[4:] + # git appends a tab to the header of a path that contains a space. + target = line[4:].removesuffix("\t") change = changes.setdefault(target[2:], FileChange()) if target.startswith("b/") else None continue if change is None: @@ -98,8 +102,10 @@ def is_checked(path): 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 + # core.quotePath=false keeps a non-ASCII path unquoted, 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], + check=True, capture_output=True, **UTF8).stdout return {path: change for path, change in parse_diff(diff).items() if is_checked(path)} @@ -117,8 +123,9 @@ def run_clang_tidy(clang_tidy, build_dir, path, lines, extra_args): 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) + # clang-tidy matches the name against the end of the file's native path. + cmd.insert(1, "--line-filter=" + json.dumps([{"name": os.path.normpath(path), "lines": lines}])) + result = subprocess.run(cmd, capture_output=True, **UTF8) suggestions = parse_suggested_includes(fixes) output = result.stdout + result.stderr return result.returncode, output, parse_diagnostics(output, suggestions) @@ -178,7 +185,7 @@ def error_sites(errors, text): 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) + shown = subprocess.run(["git", "show", f"{revision}:{path}"], capture_output=True, **UTF8) if shown.returncode != 0: return Counter() # Beside the original, so its quoted includes resolve the same way. @@ -234,6 +241,8 @@ def main(): 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() + # A piped stdout on Windows is cp1252, which cannot encode every character clang-tidy prints. + sys.stdout.reconfigure(errors="replace") merge_base = subprocess.run(["git", "merge-base", args.base, "HEAD"], check=True, capture_output=True, text=True).stdout.strip() diff --git a/scripts/run_clang_tidy.ps1 b/scripts/run_clang_tidy.ps1 index 228e84b33b..13bd4f4be4 100644 --- a/scripts/run_clang_tidy.ps1 +++ b/scripts/run_clang_tidy.ps1 @@ -54,6 +54,21 @@ function Has([string]$Command) { return [bool](Get-Command $Command -ErrorAction SilentlyContinue) } +# Runs a native command with its output, stderr included, streamed to $Log or dropped. +function Invoke-Quiet([scriptblock]$Command, [string]$Log) { + # Under "Stop", 2>&1 turns every stderr line of a native command into a terminating error. + $ErrorActionPreference = "Continue" + $lines = { + & $Command 2>&1 | ForEach-Object { + # "$_" turns a blank stderr line into the text System.Management.Automation.RemoteException. + if ($_ -isnot [System.Management.Automation.ErrorRecord]) { $_ } + elseif ($null -ne $_.TargetObject) { $_.TargetObject } + else { $_.Exception.Message } + } + } + if ($Log) { & $lines | Out-File -Encoding utf8 -LiteralPath $Log } else { & $lines | Out-Null } +} + function Request-Install([string]$What, [string]$Command) { Write-Host "Missing: $What" if (Ask "Install it now with: $Command ?") { @@ -78,11 +93,11 @@ 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 + Invoke-Quiet { & py -3 --version } if ($LASTEXITCODE -eq 0) { $Python = @("py", "-3") } } if (-not $Python -and (Has "python")) { - & python --version *> $null + Invoke-Quiet { & python --version } if ($LASTEXITCODE -eq 0) { $Python = @("python") } } if (-not $Python) { Request-Install "Python 3" "winget install -e --id Python.Python.3.12" } @@ -100,7 +115,12 @@ if (-not $VsPath) { Request-Install "Visual Studio with the C++ tools" "build_wi # 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" +# Ignores VsDevCmd's exit code, as build_win.bat does, and checks the variables it sets +# before applying any, cleared in cmd so values inherited from a developer shell do not count. +$envLines = & cmd /c "set VCToolsInstallDir=& set WindowsSdkDir=& `"$VsDevCmd`" -arch=$Arch -host_arch=$HostArch -no_logo >nul 2>nul & set" +if (-not ($envLines -match '^VCToolsInstallDir=.') -or -not ($envLines -match '^WindowsSdkDir=.')) { + throw "Loading the Visual Studio $Arch environment failed. Run `"$VsDevCmd`" -arch=$Arch -host_arch=$HostArch in cmd to see why." +} foreach ($line in $envLines) { $i = $line.IndexOf("=") if ($i -gt 0) { Set-Item -Path ("env:" + $line.Substring(0, $i)) -Value $line.Substring($i + 1) } @@ -190,13 +210,17 @@ $cmakeArgs = @("-S", ".", "-B", $BuildDir, "-G", "Ninja", "-DCMAKE_BUILD_TYPE=Re Write-Host "Configuring $BuildDir with $Compiler" New-Item -ItemType Directory -Force -Path $BuildDir | Out-Null $Log = Join-Path $BuildDir "configure.log" -& cmake @cmakeArgs *> $Log +Invoke-Quiet { & cmake @cmakeArgs } $Log if ($LASTEXITCODE -ne 0) { - Get-Content $Log -Tail 20 + Get-Content -LiteralPath $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." } +$HashLog = Join-Path $BuildDir "git_commit_hash.log" +Invoke-Quiet { & cmake --build $BuildDir --target git_commit_hash_header } $HashLog +if ($LASTEXITCODE -ne 0) { + Get-Content -LiteralPath $HashLog -Tail 20 + throw "Generating git_commit_hash.h failed; the full log is in $HashLog." +} # --- Base revision ------------------------------------------------------------ diff --git a/scripts/tests/test_clang_tidy_diff.py b/scripts/tests/test_clang_tidy_diff.py index f6b3abf8f9..a9a1520d0f 100644 --- a/scripts/tests/test_clang_tidy_diff.py +++ b/scripts/tests/test_clang_tidy_diff.py @@ -5,10 +5,13 @@ external deps). Run from the repo root: python -m unittest discover -s scripts/tests -v """ +import json import os +import subprocess import sys import tempfile import unittest +from unittest import mock sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -62,6 +65,10 @@ class TestParseChangedLines(unittest.TestCase): def test_pure_rename_has_no_changed_lines(self): self.assertNotIn("src/libslic3r/Renamed.cpp", self.changed) + def test_path_with_a_space_drops_the_tab_git_appends(self): + changed = clang_tidy_diff.parse_diff("+++ b/src/libslic3r/Foo Bar.cpp\t\n@@ -1,0 +2 @@\n+int x;\n") + self.assertEqual(changed["src/libslic3r/Foo Bar.cpp"].lines, [[2, 2]]) + class TestNamesRemovedInclude(unittest.TestCase): def test_matches_however_the_include_was_spelled(self): @@ -128,5 +135,37 @@ class TestParseSuggestedIncludes(unittest.TestCase): self.assertEqual(clang_tidy_diff.parse_suggested_includes("/nonexistent/fixes.yaml"), {}) +class TestSubprocessCalls(unittest.TestCase): + def run_patched(self, function, *args, returncode=0, stdout=""): + done = subprocess.CompletedProcess([], returncode, stdout, "") + with mock.patch.object(clang_tidy_diff.subprocess, "run", return_value=done) as run, \ + mock.patch.object(clang_tidy_diff.os.path, "normpath", wraps=os.path.normpath) as normpath: + result = function(*args) + return result, run.call_args, normpath + + def test_line_filter_names_the_file_with_native_separators(self): + _, call, normpath = self.run_patched(clang_tidy_diff.run_clang_tidy, "clang-tidy", "build", + "src/libslic3r/Color.cpp", [[4, 4]], []) + line_filter = next(arg for arg in call.args[0] if arg.startswith("--line-filter=")) + self.assertEqual(json.loads(line_filter.split("=", 1)[1]), + [{"name": os.path.join("src", "libslic3r", "Color.cpp"), "lines": [[4, 4]]}]) + normpath.assert_any_call("src/libslic3r/Color.cpp") + + def test_changed_files_reads_non_ascii_paths_and_text_as_utf8(self): + 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]) + self.assertEqual(call.kwargs["encoding"], "utf-8") + self.assertEqual(files["src/libslic3r/Über.cpp"].lines, [[2, 2]]) + + def test_clang_tidy_and_git_show_output_is_decoded_as_utf8(self): + _, call, _ = self.run_patched(clang_tidy_diff.run_clang_tidy, "clang-tidy", "build", + "src/libslic3r/Color.cpp", None, []) + self.assertEqual(call.kwargs["encoding"], "utf-8") + _, call, _ = self.run_patched(clang_tidy_diff.errors_alone_at, "base", "clang-tidy", "build", + "src/libslic3r/Color.hpp", returncode=128) + self.assertEqual(call.kwargs["encoding"], "utf-8") + + if __name__ == "__main__": unittest.main()