Fix the clang-tidy check on Windows and for unusual file paths (#16163)

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.
2026-10-05 23:01:17 +00:00 · 2026-10-06 00:35:03 +08:00
parent 1496906939
commit c1e6de7e4e
4 changed files with 86 additions and 14 deletions
@@ -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/."
@@ -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()
@@ -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 ------------------------------------------------------------

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