diff --git a/AGENTS.md b/AGENTS.md index a1fd690..c84bc81 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,9 +24,9 @@ A pre-commit hook repo that auto-installs and runs `clang-format` and `clang-tid | File | Purpose | |------|---------| | `pyproject.toml` | Build config, deps (`pip>=26.1`), dev deps, entry points | -| `cpp_linter_hooks/util.py` | PyPI version resolution + pip install (prefix matching, offline fallback) | +| `cpp_linter_hooks/util.py` | Installed-wheel check, PyPI version resolution + pip install (prefix matching, offline fallback) | | `cpp_linter_hooks/clang_format.py` | clang-format wrapper (`--verbose`, `--dry-run`) | -| `cpp_linter_hooks/clang_tidy.py` | clang-tidy wrapper (`--compile-commands`, `--jobs`, `--fix`, error hints) | +| `cpp_linter_hooks/clang_tidy.py` | clang-tidy wrapper (`--compile-commands`, `--jobs`, `--fix`, `-quiet` + status-line filter, `--checks` repair, error hints) | | `.pre-commit-hooks.yaml` | Hook metadata for pre-commit framework | | `testing/` | Test fixtures (`main.c`, `good.c`, `.clang-format`, `.clang-tidy`, CMakeLists) | | `examples/` | Example configs (CMake, Meson, large-project scoping) | @@ -42,10 +42,12 @@ bash testing/run.sh # Integration tests (full pre-commit pipeline) ## Core Patterns -- **Version resolution**: Dynamic from PyPI at runtime. No hardcoded version lists. Prefix match supported (`--version=20` → `20.1.8`). +- **Version resolution**: A wheel already installed in the hook environment (found by its `-.dist-info` directory in site-packages, e.g. from `additional_dependencies`, with its executable in the environment's scripts directory) is used without network access when it matches `--version` (exact or prefix) or when no `--version` is given. Otherwise the version is resolved dynamically from PyPI at runtime. No hardcoded version lists. Prefix match supported (`--version=20` → `20.1.8`). - **Return values**: All hooks return `tuple[int, str]` — `(retval, output)`, where `retval` is the tool's exit code (`0` on success, non-zero on failure; `--dry-run` auto-adds `--Werror` so formatting issues fail). - **clang-tidy compile DB**: Auto-detected from `build/`, `out/`, `cmake-build-debug/`, `_build/`. Override with `--compile-commands=`, disable with `--no-compile-commands`. - **clang-tidy parallel**: `--jobs=N` runs per-file in parallel. Forced serial when `--export-fixes`, `-fix`, or `-fix-errors` is used. +- **clang-tidy output**: Without `--verbose`, the hook adds `-quiet` (unless the user set it) and strips status lines (`N warnings generated.`, `Suppressed N warnings ...`, `Use -header-filter=...`, `[1/N] Processing file ...`). With `--verbose`, output is unchanged. +- **clang-tidy check lists**: `--checks=` / `--warnings-as-errors=` lists that YAML split at commas are joined again, quotes inside the value are removed, and `--checks=` (e.g. `.clang-tidy`) gets a note. Notes are appended to the output, which pre-commit shows when the hook fails. ## CI diff --git a/README.md b/README.md index 83773b8..30a0c70 100644 --- a/README.md +++ b/README.md @@ -40,18 +40,39 @@ leave it out if you only run clang-tidy in CI, for example with > [!TIP] > The `rev` tag (e.g. `v1.6.1`) is the **project** version, not the clang tool version. Without -> `--version`, each hook installs the newest clang-format or clang-tidy wheel on PyPI at the time it -> runs, so the tool version can change without any change to your configuration, and the two hooks -> can run different LLVM versions. For production use, always pin the tool version explicitly -> with `--version`. +> `--version`, each hook installs the newest clang-format or clang-tidy wheel on PyPI the first time +> it runs and keeps it until pre-commit rebuilds the hook environment, so two machines can run +> different LLVM versions with the same configuration. For production use, always pin the tool +> version explicitly with `--version`. - `--version=21` installs the newest 21.x wheel, and `--version=21.1.8` pins an exact release. clang-tidy wheels are released separately from clang-format wheels and skip some releases, so give clang-tidy the major version. - clang-format wheels cover LLVM 6 to 23 and clang-tidy wheels LLVM 13 to 23. For a version without a wheel, the hook fails and lists some of the versions that exist. -- The hook looks the version up on pypi.org every time it runs. Without network access it fails - when `--version` is set, and otherwise uses the clang-format or clang-tidy already installed. +- When the hook environment already has a wheel that matches `--version` (or any wheel, without + `--version`), the hook uses it and does not contact pypi.org. Otherwise it looks the version up + on pypi.org and installs it, so that run needs network access. + +### Offline and pre-commit.ci + +To install the clang tool together with the hook, list the exact wheel in +`additional_dependencies`: + +```yaml + - id: clang-format + args: [--style=file, --version=21] + additional_dependencies: [clang-format==21.1.8] + - id: clang-tidy + args: [--version=21] + additional_dependencies: [clang-tidy==21.1.6] +``` + +pre-commit installs `additional_dependencies` when it creates the hook environment (for example +with `pre-commit install-hooks`), so the hooks need no network access when they run. Use this on +[pre-commit.ci](https://pre-commit.ci/), which runs hooks without network access, and behind a +firewall. The pinned wheel must match `--version`; otherwise the hook installs the `--version` one +at run time. ### clang-format @@ -115,7 +136,9 @@ int main() {for (;;) break; printf("Hello world!\n");return 0;} ### clang-tidy To set the checks in `args` instead of your `.clang-tidy` file, quote the whole option: inside -`[...]`, YAML splits an unquoted value at each comma. +`[...]`, YAML splits an unquoted value at each comma. When that happens, the hook joins the pieces +again and adds a note on how to fix the configuration. `--checks` takes check names, not a file: +clang-tidy reads the nearest `.clang-tidy` file by itself, so `--checks=.clang-tidy` does nothing. ```yaml - id: clang-tidy @@ -129,9 +152,6 @@ clang-tidy...............................................................Failed - hook id: clang-tidy - exit code: 1 -522 warnings generated. -Suppressed 521 warnings (521 in non-user code). -Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well. /home/runner/work/cpp-linter-hooks/cpp-linter-hooks/testing/main.c:4:13: warning: statement should be inside braces [readability-braces-around-statements] for (;;) ^ @@ -139,6 +159,10 @@ Use -header-filter=.* to display errors from all non-system headers. Use -system ``` +The hook passes `-quiet` to clang-tidy and drops status lines such as `522 warnings generated.`, +which count warnings in system headers that clang-tidy does not show. Add `--verbose` to `args` to +keep clang-tidy's full output. + > [!NOTE] > Add `--fix` to `args` to automatically apply clang-tidy fixes in place (equivalent to > passing `-fix` to clang-tidy directly). This is **opt-in** and **not the default** because @@ -253,7 +277,8 @@ This approach ensures that only modified files are checked, further speeding up > [!NOTE] > Use `-v` or `--verbose` in `args` to enable verbose output. > For `clang-format`, it shows the list of processed files. -> For `clang-tidy`, it prints which `compile_commands.json` is being used (when auto-detected or explicitly set). +> For `clang-tidy`, it prints which `compile_commands.json` is being used (when auto-detected or explicitly set) +> and keeps clang-tidy's full output, without `-quiet`. > pre-commit shows this output only when the hook fails; add `verbose: true` to the hook to see it on every run. ```yaml diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index d390651..f96f4d3 100644 --- a/cpp_linter_hooks/clang_format.py +++ b/cpp_linter_hooks/clang_format.py @@ -23,12 +23,13 @@ def run_clang_format(args=None) -> tuple[int, str]: """Run clang-format with hook-specific arguments removed.""" hook_args, other_args = parser.parse_known_args(args) - _, version_error = resolve_install_with_diagnostics( + tool_path, version_error = resolve_install_with_diagnostics( "clang-format", hook_args.version, hook_args.verbose ) if version_error is not None: return 1, version_error - command = ["clang-format", "-i"] + # Run the executable that was resolved, not whatever comes first on PATH. + command = [str(tool_path) if tool_path else "clang-format", "-i"] # Add verbose flag if requested if hook_args.verbose: diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py index 92bcebb..f8cc2b5 100644 --- a/cpp_linter_hooks/clang_tidy.py +++ b/cpp_linter_hooks/clang_tidy.py @@ -1,5 +1,6 @@ """Pre-commit hook wrapper for clang-tidy.""" +import re import subprocess import sys from argparse import ArgumentParser, ArgumentTypeError @@ -43,6 +44,64 @@ - If using CMake, generate compile_commands.json from the same toolchain.""" +# Status lines clang-tidy prints around the diagnostics. They say nothing about +# the files being checked (e.g. "537 warnings generated." comes from system +# headers), so the hook drops them unless --verbose is given. clang-tidy 18 and +# older print the "generated" lines even with -quiet. +NOISE_LINE_PATTERNS = ( + re.compile(r"^\d+ (?:warning|error)s?(?: and \d+ errors?)? generated\.$"), + re.compile(r"^Suppressed \d+ warnings? \(.*\)\.$"), + re.compile( + r"^Use -header-filter=.* to display errors from all non-system headers\." + ), + re.compile(r"^\[\d+/\d+\] Processing file .*\.$"), +) + +# Options whose value is a comma-separated list of check globs. +CHECK_GLOB_OPTIONS = ( + "--checks=", + "-checks=", + "--warnings-as-errors=", + "-warnings-as-errors=", +) +CHECK_MODULES = ( + "abseil", + "altera", + "android", + "boost", + "bugprone", + "cert", + "clang-analyzer", + "clang-diagnostic", + "concurrency", + "cppcoreguidelines", + "darwin", + "fuchsia", + "google", + "hicpp", + "linuxkernel", + "llvm", + "llvmlibc", + "misc", + "modernize", + "mpi", + "objc", + "openmp", + "performance", + "portability", + "readability", + "zircon", +) +# A single check glob such as "bugprone-*", "-modernize-use-trailing-return-type" +# or "*", optionally carrying a stray YAML quote. +CHECK_GLOB_FRAGMENT = re.compile( + r"""^['"]?-?(?:\*[\w.*-]*|(?:""" + + "|".join(re.escape(module) for module in CHECK_MODULES) + + r""")-[\w.*-]*)['"]?$""" +) +QUOTES = ("'", '"') + + def _positive_int(value: str) -> int: """Parse a positive integer for the --jobs option.""" jobs = int(value) @@ -210,6 +269,100 @@ def _exec_clang_tidy(command) -> tuple[int, str]: return 1, str(e) +def _strip_noise(output: str) -> str: + """Drop clang-tidy status lines that do not point at a problem.""" + lines = output.splitlines() + kept = [ + line + for line in lines + if not any(pattern.match(line) for pattern in NOISE_LINE_PATTERNS) + ] + if len(kept) == len(lines): + return output + return "\n".join(kept) + + +def _has_quiet_option(args: list[str]) -> bool: + """Return whether the user already set clang-tidy's -quiet option.""" + return any( + arg in ("-quiet", "--quiet") or arg.startswith(("-quiet=", "--quiet=")) + for arg in args + ) + + +def _unquote(value: str) -> str: + """Remove one pair of matching quotes around a value.""" + if len(value) >= 2 and value[0] == value[-1] and value[0] in QUOTES: + return value[1:-1] + return value + + +def _looks_like_config_file(value: str) -> bool: + """Return whether a --checks value names a file instead of check globs.""" + name = Path(value).name + return name.startswith(".") or name.endswith((".yaml", ".yml")) or "/" in value + + +def _repair_check_args(args: list[str]) -> tuple[list[str], list[str]]: + """Fix check lists that YAML mangled in .pre-commit-config.yaml. + + Returns (args, notes). Inside ``args: [...]`` YAML splits an unquoted + ``--checks=-*,bugprone-*`` at each comma, and clang-tidy would then treat + ``bugprone-*`` as a source file. Quotes written inside the value reach + clang-tidy as part of the glob. Both are repaired here; the notes tell the + user how to fix the configuration. + """ + repaired: list[str] = [] + notes: list[str] = [] + idx = 0 + while idx < len(args): + arg = args[idx] + idx += 1 + option = next((o for o in CHECK_GLOB_OPTIONS if arg.startswith(o)), None) + if option is None: + repaired.append(arg) + continue + + fragments = [arg[len(option) :]] + while idx < len(args) and CHECK_GLOB_FRAGMENT.match(args[idx]): + fragments.append(args[idx]) + idx += 1 + value = ",".join(fragments) + unquoted = _unquote(value) + fixed = f"{option}{unquoted}" + repaired.append(fixed) + + if len(fragments) > 1: + notes.append( + f"Note: YAML split {option}{value} at the commas; the hook joined " + "the pieces again. Quote the whole option in " + f'.pre-commit-config.yaml: args: ["{fixed}"]' + ) + elif unquoted != value: + notes.append( + f"Note: removed the quotes inside {arg}; clang-tidy would read them " + "as part of the check names. Quote the whole option instead: " + f'args: ["{fixed}"]' + ) + + if option.endswith("checks=") and _looks_like_config_file(unquoted): + notes.append( + f"Note: {fixed} has no effect: --checks takes check names, not a " + "file. clang-tidy already reads the nearest .clang-tidy file for " + "each source file, so remove this option, or use " + "--config-file= to pick a specific file." + ) + return repaired, notes + + +def _append_notes(output: str, notes: list[str]) -> str: + """Append configuration notes after clang-tidy's output.""" + if not notes: + return output + separator = "\n\n" if output.rstrip("\n") else "" + return output.rstrip("\n") + separator + "\n".join(notes) + + def _looks_like_source_file(path: str) -> bool: """Return whether a path has a recognized C or C++ source suffix.""" return Path(path).suffix.lower() in SOURCE_FILE_SUFFIXES @@ -251,11 +404,13 @@ def run_file(source_file: str) -> tuple[int, str]: def run_clang_tidy(args=None) -> tuple[int, str]: """Run clang-tidy with hook-specific argument handling.""" hook_args, other_args = parser.parse_known_args(args) - _, version_error = resolve_install_with_diagnostics( + tool_path, version_error = resolve_install_with_diagnostics( "clang-tidy", hook_args.version, hook_args.verbose ) if version_error is not None: return 1, version_error + # Run the executable that was resolved, not whatever comes first on PATH. + executable = str(tool_path) if tool_path else "clang-tidy" compile_db_path, error = _resolve_compile_db(hook_args, other_args) if error is not None: @@ -273,6 +428,12 @@ def run_clang_tidy(args=None) -> tuple[int, str]: print(_compile_commands_not_found_message(), file=sys.stderr) clang_tidy_args, source_files = _split_source_files(other_args) + clang_tidy_args, notes = _repair_check_args(clang_tidy_args) + + # -quiet hides clang-tidy's statistics about suppressed warnings; --verbose + # keeps clang-tidy's full output. + if not hook_args.verbose and not _has_quiet_option(clang_tidy_args): + clang_tidy_args.insert(0, "-quiet") if ( hook_args.fix @@ -293,11 +454,15 @@ def run_clang_tidy(args=None) -> tuple[int, str]: ) if hook_args.jobs > 1 and len(source_files) > 1 and not unsafe_parallel: - return _exec_parallel_clang_tidy( - ["clang-tidy"] + clang_tidy_args, source_files, hook_args.jobs + retval, output = _exec_parallel_clang_tidy( + [executable] + clang_tidy_args, source_files, hook_args.jobs ) + else: + retval, output = _exec_clang_tidy([executable] + clang_tidy_args + source_files) - return _exec_clang_tidy(["clang-tidy"] + clang_tidy_args + source_files) + if not hook_args.verbose: + output = _strip_noise(output) + return retval, _append_notes(output, notes) def main() -> int: diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 1868c9f..dac3565 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -2,10 +2,12 @@ import json import logging +import os import re import shutil import subprocess import sys +import sysconfig import urllib.error import urllib.request from functools import lru_cache @@ -147,6 +149,64 @@ def _is_version_installed(tool: str, version: str) -> Path | None: return None +@lru_cache(maxsize=1) +def _env_paths() -> dict[str, str]: + """Return this Python environment's install paths, which never change at runtime.""" + return sysconfig.get_paths() + + +def _installed_wheel_version(tool: str) -> str | None: + """Return the version of the *tool* wheel installed in this Python environment. + + pre-commit installs ``additional_dependencies`` (and every wheel this hook + installed on an earlier run) into the hook's own environment. The version + comes from the wheel's ``-.dist-info`` directory name, which + needs neither the network nor a subprocess, and avoids parsing the large + METADATA file the way ``importlib.metadata.version`` does. + """ + prefix = f"{re.sub(r'[-_.]+', '_', tool).lower()}-" + suffix = ".dist-info" + paths = _env_paths() + site_dirs = dict.fromkeys((paths["purelib"], paths["platlib"])) + for site_dir in site_dirs: + try: + names = os.listdir(site_dir) + except OSError: + continue + for name in names: + lower = name.lower() + if lower.startswith(prefix) and lower.endswith(suffix): + return name[len(prefix) : -len(suffix)] + return None + + +def _version_satisfies(installed: str, requested: str | None) -> bool: + """Return whether *installed* matches ``--version`` (exact or prefix).""" + if requested is None: + return True + return installed == requested or installed.startswith(f"{requested}.") + + +def _env_executable(tool: str) -> str | None: + """Return *tool* from this Python environment's scripts directory, if any.""" + return shutil.which(tool, path=_env_paths()["scripts"]) + + +def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | None: + """Return (path, version) of an installed wheel that satisfies *version*. + + Only the executable the wheel installed into this environment counts: a + tool found elsewhere on PATH may be a different version. + """ + installed = _installed_wheel_version(tool) + if installed is None or not _version_satisfies(installed, version): + return None + path = _env_executable(tool) + if path is None: + return None + return Path(path), installed + + def _install_tool(tool: str, version: str) -> Path | None: """Install a tool using pip, logging output on failure.""" result = subprocess.run( @@ -156,7 +216,7 @@ def _install_tool(tool: str, version: str) -> Path | None: check=False, ) if result.returncode == 0: - return shutil.which(tool) + return _env_executable(tool) or shutil.which(tool) LOG.error("pip failed to install %s %s", tool, version) LOG.error(result.stdout) LOG.error(result.stderr) @@ -168,9 +228,22 @@ def resolve_install_with_diagnostics( ) -> tuple[Path | None, str | None]: """Resolve/install a tool, returning a user-facing error for bad versions. - Tool versions are resolved dynamically from PyPI — no hardcoded + A wheel already installed in the hook's environment is used as is when it + matches ``--version`` (or when no version was given), so pinned + ``additional_dependencies`` are kept and the hook works offline. + Otherwise the version is resolved dynamically from PyPI — no hardcoded list is maintained in-tree. """ + installed_wheel = _find_installed_wheel(tool, version) + if installed_wheel is not None: + path, installed = installed_wheel + if verbose: + print( + f"Using installed {tool} Python wheel version {installed}", + file=sys.stderr, + ) + return path, None + user_version, error = _resolve_version_from_pypi(tool, version) if error is not None: return None, error diff --git a/testing/pre-commit-config-parallel.yaml b/testing/pre-commit-config-parallel.yaml index ef764e1..a553143 100644 --- a/testing/pre-commit-config-parallel.yaml +++ b/testing/pre-commit-config-parallel.yaml @@ -3,4 +3,4 @@ repos: rev: HEAD hooks: - id: clang-tidy - args: [--checks=.clang-tidy, --jobs=2] + args: [--jobs=2] diff --git a/testing/pre-commit-config-style.yaml b/testing/pre-commit-config-style.yaml index 568e275..51b2dbe 100644 --- a/testing/pre-commit-config-style.yaml +++ b/testing/pre-commit-config-style.yaml @@ -5,39 +5,39 @@ repos: - id: clang-format args: [--style=LLVM, --version=16] # to load .clang-format - id: clang-tidy - args: [--checks=.clang-tidy, --version=16] # path/to/.clang-tidy + args: [--version=16] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=Google, --version=17] - id: clang-tidy - args: [--checks=.clang-tidy, --version=17] + args: [--version=17] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=Microsoft, --version=18] - id: clang-tidy - args: [--checks=.clang-tidy, --version=18] + args: [--version=18] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=WebKit, --version=19] - id: clang-tidy - args: [--checks=.clang-tidy, --version=19] + args: [--version=19] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=Mozilla, --version=20] - id: clang-tidy - args: [--checks=.clang-tidy, --version=20] + args: [--version=20] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=Chromium, --version=21] - id: clang-tidy - args: [--checks=.clang-tidy, --version=21] + args: [--version=21] diff --git a/testing/pre-commit-config-version.yaml b/testing/pre-commit-config-version.yaml index 269608c..1ede24a 100644 --- a/testing/pre-commit-config-version.yaml +++ b/testing/pre-commit-config-version.yaml @@ -5,46 +5,46 @@ repos: - id: clang-format args: [--style=file, --version=16] # to load .clang-format - id: clang-tidy - args: [--checks=.clang-tidy, --version=16] # path/to/.clang-tidy + args: [--version=16] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=17] - id: clang-tidy - args: [--checks=.clang-tidy, --version=17] + args: [--version=17] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=18] - id: clang-tidy - args: [--checks=.clang-tidy, --version=18] + args: [--version=18] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=19] - id: clang-tidy - args: [--checks=.clang-tidy, --version=19] + args: [--version=19] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=20] - id: clang-tidy - args: [--checks=.clang-tidy, --version=20] + args: [--version=20] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=21] - id: clang-tidy - args: [--checks=.clang-tidy, --version=21] + args: [--version=21] - repo: . rev: HEAD hooks: - id: clang-format args: [--style=file, --version=22] - id: clang-tidy - args: [--checks=.clang-tidy, --version=22] + args: [--version=22] diff --git a/testing/pre-commit-config.yaml b/testing/pre-commit-config.yaml index f92efec..22dcc7f 100644 --- a/testing/pre-commit-config.yaml +++ b/testing/pre-commit-config.yaml @@ -5,4 +5,3 @@ repos: - id: clang-format args: [--style=file] # to load .clang-format - id: clang-tidy - args: [--checks=.clang-tidy] # path/to/.clang-tidy diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py index a81f529..7fc3288 100644 --- a/tests/test_clang_format.py +++ b/tests/test_clang_format.py @@ -317,3 +317,14 @@ def test_main_does_not_print_blank_failure_output(monkeypatch, capsys): assert main() == 1 assert capsys.readouterr().out == "" + + +def test_run_clang_format_uses_the_resolved_executable(mock_clang_format): + """The version check and the run must use the same clang-format.""" + mock_resolve, mock_run = mock_clang_format + tool = Path("/venv/bin/clang-format") + mock_resolve.return_value = (tool, None) + + run_clang_format(["a.cpp"]) + + assert mock_run.call_args.args[0] == [str(tool), "-i", "a.cpp"] diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py index d5595fa..0666b37 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -16,7 +16,9 @@ _looks_like_compile_db_error, _looks_like_msvc_error, _positive_int, + _repair_check_args, _split_source_files, + _strip_noise, main, parser, run_clang_tidy, @@ -369,7 +371,9 @@ def test_jobs_one_keeps_single_invocation(): ): run_clang_tidy(["--jobs=1", "-p", "./build", "a.cpp", "b.cpp"]) - mock_exec.assert_called_once_with(["clang-tidy", "-p", "./build", "a.cpp", "b.cpp"]) + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "-p", "./build", "a.cpp", "b.cpp"] + ) def test_jobs_parallelizes_source_files_and_preserves_output_order(): @@ -425,8 +429,8 @@ def test_jobs_parallelizes_only_trailing_source_files(): commands = {tuple(call.args[0]) for call in mock_exec.call_args_list} assert commands == { - ("clang-tidy", "-p", "./build", "--header-filter=.*", "a.cpp"), - ("clang-tidy", "-p", "./build", "--header-filter=.*", "b.hpp"), + ("clang-tidy", "-quiet", "-p", "./build", "--header-filter=.*", "a.cpp"), + ("clang-tidy", "-quiet", "-p", "./build", "--header-filter=.*", "b.hpp"), } @@ -455,6 +459,7 @@ def test_jobs_with_export_fixes_forces_serial_execution(): mock_exec.assert_called_once_with( [ "clang-tidy", + "-quiet", "-p", "./build", "--export-fixes", @@ -723,7 +728,9 @@ def test_hook_options_are_not_passed_to_clang_tidy(mock_resolve, mock_exec, caps def test_jobs_with_single_source_file_runs_once(mock_exec): run_clang_tidy(["--jobs=4", "-p", "./build", "a.cpp"]) - mock_exec.assert_called_once_with(["clang-tidy", "-p", "./build", "a.cpp"]) + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "-p", "./build", "a.cpp"] + ) def test_jobs_parallel_run_succeeds_when_every_file_passes(mock_exec): @@ -743,7 +750,15 @@ def test_jobs_with_export_fixes_equals_form_forces_serial_execution(mock_exec): ) mock_exec.assert_called_once_with( - ["clang-tidy", "-p", "./build", "--export-fixes=fixes.yaml", "a.cpp", "b.cpp"] + [ + "clang-tidy", + "-quiet", + "-p", + "./build", + "--export-fixes=fixes.yaml", + "a.cpp", + "b.cpp", + ] ) @@ -751,7 +766,7 @@ def test_fix_flag_is_inserted_before_source_files(mock_exec): run_clang_tidy(["--fix", "-p", "./build", "a.cpp", "b.cpp"]) mock_exec.assert_called_once_with( - ["clang-tidy", "-p", "./build", "-fix", "a.cpp", "b.cpp"] + ["clang-tidy", "-quiet", "-p", "./build", "-fix", "a.cpp", "b.cpp"] ) @@ -760,7 +775,7 @@ def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): run_clang_tidy(["--fix", "-p", "./build", existing, "a.cpp"]) mock_exec.assert_called_once_with( - ["clang-tidy", "-p", "./build", existing, "a.cpp"] + ["clang-tidy", "-quiet", "-p", "./build", existing, "a.cpp"] ) @@ -774,7 +789,7 @@ def test_compile_commands_auto_detect_each_search_dir( run_clang_tidy(["a.cpp"]) - mock_exec.assert_called_once_with(["clang-tidy", "-p", db_dir, "a.cpp"]) + mock_exec.assert_called_once_with(["clang-tidy", "-quiet", "-p", db_dir, "a.cpp"]) def test_compile_commands_auto_detect_prefers_first_search_dir( @@ -788,7 +803,7 @@ def test_compile_commands_auto_detect_prefers_first_search_dir( run_clang_tidy(["a.cpp"]) mock_exec.assert_called_once_with( - ["clang-tidy", "-p", COMPILE_DB_SEARCH_DIRS[0], "a.cpp"] + ["clang-tidy", "-quiet", "-p", COMPILE_DB_SEARCH_DIRS[0], "a.cpp"] ) @@ -801,7 +816,7 @@ def test_compile_commands_p_equals_form_disables_auto_detect( run_clang_tidy(["-p=./custom", "a.cpp"]) - mock_exec.assert_called_once_with(["clang-tidy", "-p=./custom", "a.cpp"]) + mock_exec.assert_called_once_with(["clang-tidy", "-quiet", "-p=./custom", "a.cpp"]) def test_compile_commands_explicit_path_must_be_a_directory(tmp_path, mock_exec): @@ -856,4 +871,211 @@ def test_main_reads_arguments_from_command_line(monkeypatch, mock_resolve, mock_ assert main() == 0 mock_resolve.assert_called_once_with("clang-tidy", "21", False) + mock_exec.assert_called_once_with(["clang-tidy", "-quiet", "a.cpp"]) + + +# --- quiet output --- + + +@pytest.mark.parametrize("existing", ["-quiet", "--quiet", "--quiet=false"]) +def test_quiet_is_not_added_twice(mock_exec, existing): + run_clang_tidy([existing, "a.cpp"]) + + mock_exec.assert_called_once_with(["clang-tidy", existing, "a.cpp"]) + + +def test_verbose_does_not_add_quiet(mock_exec): + run_clang_tidy(["--verbose", "--no-compile-commands", "a.cpp"]) + mock_exec.assert_called_once_with(["clang-tidy", "a.cpp"]) + + +@pytest.mark.parametrize( + "line", + [ + "1 warning generated.", + "537 warnings generated.", + "1 error generated.", + "14 warnings and 6 errors generated.", + "1 warning and 1 error generated.", + "Suppressed 521 warnings (521 in non-user code).", + "Suppressed 3 warnings (2 in non-user code, 1 NOLINT).", + ( + "Use -header-filter=.* to display errors from all non-system headers. " + "Use -system-headers to display errors from system headers as well." + ), + ( + "Use -header-filter=.* or leave it as default to display errors from all " + "non-system headers. Use -system-headers to display errors from system " + "headers as well." + ), + "[1/2] Processing file /src/a.cpp.", + ], +) +def test_strip_noise_removes_status_lines(line): + diagnostic = "a.cpp:4:13: warning: statement should be inside braces [x]" + + assert _strip_noise(f"{line}\n{diagnostic}\n") == diagnostic + + +@pytest.mark.parametrize( + "output", + [ + "", + "a.cpp:1:1: warning: 2 warnings generated. [x]\n", + "Error while processing /src/a.cpp.\n", + "Error while trying to load a compilation database:\nRunning without flags.\n", + ], +) +def test_strip_noise_keeps_everything_else(output): + assert _strip_noise(output) == output + + +def test_run_strips_noise_unless_verbose(mock_resolve): + raw = ( + "537 warnings generated.\n" + "Suppressed 536 warnings (536 in non-user code).\n" + "a.cpp:4:13: warning: statement should be inside braces [x]\n" + ) + with patch("cpp_linter_hooks.clang_tidy._exec_clang_tidy", return_value=(1, raw)): + assert run_clang_tidy(["a.cpp"]) == ( + 1, + "a.cpp:4:13: warning: statement should be inside braces [x]", + ) + assert run_clang_tidy(["--verbose", "--no-compile-commands", "a.cpp"]) == ( + 1, + raw, + ) + + +# --- check lists mangled by YAML in .pre-commit-config.yaml --- + + +@pytest.mark.parametrize( + ("args", "expected"), + [ + ( + ["--checks=-*", "bugprone-*", "performance-*"], + ["--checks=-*,bugprone-*,performance-*"], + ), + ( + ["--checks=boost-*", "clang-analyzer-*", "cppcoreguidelines-*", "-p", "b"], + ["--checks=boost-*,clang-analyzer-*,cppcoreguidelines-*", "-p", "b"], + ), + ( + ["--checks=*", "-modernize-use-trailing-return-type", "-llvmlibc-*"], + ["--checks=*,-modernize-use-trailing-return-type,-llvmlibc-*"], + ), + ( + ["--checks='-*", "cppcoreguidelines-*'"], + ["--checks=-*,cppcoreguidelines-*"], + ), + ( + ["-checks=-*", "readability-*", "--header-filter=.*"], + ["-checks=-*,readability-*", "--header-filter=.*"], + ), + ( + ["--warnings-as-errors=*", "-misc-*"], + ["--warnings-as-errors=*,-misc-*"], + ), + ], +) +def test_repair_check_args_joins_split_check_lists(args, expected): + repaired, notes = _repair_check_args(args) + + assert repaired == expected + assert len(notes) == 1 + assert "YAML split" in notes[0] + assert f'args: ["{expected[0]}"]' in notes[0] + + +@pytest.mark.parametrize( + ("arg", "expected"), + [('--checks="boost-*"', "--checks=boost-*"), ("--checks='-*'", "--checks=-*")], +) +def test_repair_check_args_removes_quotes_inside_the_value(arg, expected): + repaired, notes = _repair_check_args([arg]) + + assert repaired == [expected] + assert len(notes) == 1 + assert "removed the quotes" in notes[0] + assert f'args: ["{expected}"]' in notes[0] + + +@pytest.mark.parametrize( + "arg", + [ + "--checks=.clang-tidy", + "--checks=.clang-dity", + "--checks=./.clang-tidy", + "-checks=config/clang-tidy.yaml", + ], +) +def test_repair_check_args_flags_a_config_file_given_as_checks(arg): + repaired, notes = _repair_check_args([arg]) + + assert repaired == [arg] + assert len(notes) == 1 + assert "has no effect" in notes[0] + assert "--config-file" in notes[0] + + +@pytest.mark.parametrize( + "args", + [ + [], + ["--checks=-*,bugprone-*", "-p", "build"], + ["-p", "build", "--header-filter=.*", "--extra-arg=-std=c++17"], + ["bugprone-*"], + ["--config-file=.clang-tidy", "--line-filter=[]"], + ], +) +def test_repair_check_args_leaves_valid_arguments_alone(args): + assert _repair_check_args(args) == (args, []) + + +def test_run_joins_split_checks_and_explains_why(mock_resolve): + with patch( + "cpp_linter_hooks.clang_tidy._exec_clang_tidy", + return_value=(1, "a.cpp:1:1: warning: x [bugprone-y]\n"), + ) as mock_exec: + ret, output = run_clang_tidy(["--checks=-*", "bugprone-*", "a.cpp"]) + + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "--checks=-*,bugprone-*", "a.cpp"] + ) + assert ret == 1 + assert output.startswith("a.cpp:1:1: warning: x [bugprone-y]\n\nNote: YAML split") + + +def test_run_clang_tidy_with_split_checks_runs_the_real_tool(tmp_path): + """End to end: the pieces YAML split off are not treated as files.""" + # No #include, so the result does not depend on system headers being found. + source = tmp_path / "braces.cpp" + source.write_text("int f(int x) {\n if (x)\n return 1;\n return 0;\n}\n") + + ret, output = run_clang_tidy( + ["--no-compile-commands", "--checks=-*", "readability-braces-*", str(source)] + ) + + assert ret == 1 + assert "readability-braces-around-statements" in output + assert "Error while processing" not in output + assert "warnings generated" not in output + assert "YAML split" in output + + +def test_run_uses_the_resolved_executable(mock_resolve): + """The version check and the run must use the same clang-tidy.""" + tool = Path("/venv/bin/clang-tidy") + mock_resolve.return_value = (tool, None) + with patch( + "cpp_linter_hooks.clang_tidy._exec_clang_tidy", return_value=(0, "") + ) as mock_exec: + run_clang_tidy(["--no-compile-commands", "a.cpp"]) + run_clang_tidy(["--no-compile-commands", "--jobs=2", "a.cpp", "b.cpp"]) + + commands = [call.args[0] for call in mock_exec.call_args_list] + assert commands[0] == [str(tool), "-quiet", "a.cpp"] + assert sorted(command[-1] for command in commands[1:]) == ["a.cpp", "b.cpp"] + assert all(command[0] == str(tool) for command in commands) diff --git a/tests/test_util.py b/tests/test_util.py index ec16fcb..11d0d0c 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -4,6 +4,7 @@ import logging import subprocess import sys +import sysconfig from pathlib import Path from unittest.mock import patch @@ -11,10 +12,14 @@ from cpp_linter_hooks.util import ( _detect_installed_version, + _env_paths, + _find_installed_wheel, _get_pypi_versions, _install_tool, + _installed_wheel_version, _is_version_installed, _resolve_version_from_pypi, + _version_satisfies, resolve_install, resolve_install_with_diagnostics, ) @@ -25,6 +30,17 @@ MOCK_PYPI_TIDY = ("21.1.6", ["21.1.6", "21.1.1", "20.1.0", "19.1.0.1", "18.1.8"]) +@pytest.fixture(autouse=True) +def no_installed_wheel(): + """Hide wheels installed in the test environment from the resolver. + + Tests that need an installed wheel patch ``_installed_wheel_version`` or + ``_find_installed_wheel`` themselves. + """ + with patch("cpp_linter_hooks.util._installed_wheel_version", return_value=None): + yield + + def _pypi_side_effect(tool: str): """Side-effect that maps tool names to canned PyPI responses.""" mapping = { @@ -689,3 +705,210 @@ def test_resolve_version_from_pypi_prefix_stops_at_component_boundary( return_value=(versions[0], versions), ): assert _resolve_version_from_pypi("clang-tidy", user_input) == (expected, None) + + +# ═══════════════════════════════════════════════════════════════════════ +# Installed wheel in the hook environment (additional_dependencies, offline) +# ═══════════════════════════════════════════════════════════════════════ + + +@pytest.mark.parametrize( + ("installed", "requested", "expected"), + [ + ("21.1.6", None, True), + ("21.1.6", "21", True), + ("21.1.6", "21.1", True), + ("21.1.6", "21.1.6", True), + ("19.1.0.1", "19.1.0", True), + ("21.1.6", "2", False), + ("21.1.6", "21.1.1", False), + ("21.10.0", "21.1", False), + ("23.1.3", "21", False), + ], +) +def test_version_satisfies(installed, requested, expected): + assert _version_satisfies(installed, requested) is expected + + +def _paths(directory) -> dict[str, str]: + """Environment paths that all point at one directory.""" + return dict.fromkeys(("purelib", "platlib", "scripts"), str(directory)) + + +def test_env_paths_match_sysconfig(): + _env_paths.cache_clear() + assert _env_paths() == sysconfig.get_paths() + + +def test_installed_wheel_version_reads_dist_info_name(tmp_path): + for name in ( + "clang_tidy_extra-1.0.dist-info", + "clang_format-22.1.8.dist-info", + "clang_tidy", + "Clang_Tidy-21.1.6.dist-info", + ): + (tmp_path / name).mkdir() + with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): + assert _installed_wheel_version("clang-tidy") == "21.1.6" + assert _installed_wheel_version("clang-format") == "22.1.8" + + +def test_installed_wheel_version_checks_platlib(tmp_path): + purelib, platlib = tmp_path / "purelib", tmp_path / "platlib" + purelib.mkdir() + (platlib / "clang_tidy-21.1.6.dist-info").mkdir(parents=True) + paths = {"purelib": str(purelib), "platlib": str(platlib), "scripts": ""} + with patch("cpp_linter_hooks.util._env_paths", return_value=paths): + assert _installed_wheel_version("clang-tidy") == "21.1.6" + + +def test_installed_wheel_version_missing_distribution(tmp_path): + (tmp_path / "clang_format-22.1.8.dist-info").mkdir() + with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): + assert _installed_wheel_version("clang-tidy") is None + with patch( + "cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path / "missing") + ): + assert _installed_wheel_version("clang-tidy") is None + + +def test_find_installed_wheel_prefers_environment_scripts_dir(): + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="21.1.6"), + patch("cpp_linter_hooks.util._env_paths", return_value=_paths("/venv/bin")), + patch("shutil.which", return_value="/venv/bin/clang-tidy") as mock_which, + ): + assert _find_installed_wheel("clang-tidy", "21") == ( + Path("/venv/bin/clang-tidy"), + "21.1.6", + ) + mock_which.assert_called_once_with("clang-tidy", path="/venv/bin") + + +def test_find_installed_wheel_ignores_other_tools_on_path(): + """A clang-tidy elsewhere on PATH may be another version than the wheel.""" + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="21.1.6"), + patch("cpp_linter_hooks.util._env_paths", return_value=_paths("/venv/bin")), + patch( + "shutil.which", + side_effect=lambda tool, path=None: ( + None if path == "/venv/bin" else "/usr/bin/clang-tidy" + ), + ), + ): + assert _find_installed_wheel("clang-tidy", None) is None + + +def test_install_tool_prefers_environment_scripts_dir(): + completed = subprocess.CompletedProcess(args=[], returncode=0) + with ( + patch("subprocess.run", return_value=completed), + patch("cpp_linter_hooks.util._env_paths", return_value=_paths("/venv/bin")), + patch( + "shutil.which", + side_effect=lambda tool, path=None: ( + "/venv/bin/clang-tidy" if path == "/venv/bin" else "/usr/bin/clang-tidy" + ), + ), + ): + assert _install_tool("clang-tidy", "21.1.6") == "/venv/bin/clang-tidy" + + +@pytest.mark.parametrize( + ("installed", "requested"), [(None, "21"), ("23.1.3", "21"), ("21.1.6", "21.1.1")] +) +def test_find_installed_wheel_rejects_missing_or_other_version(installed, requested): + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value=installed), + patch("shutil.which", return_value="/venv/bin/clang-tidy"), + ): + assert _find_installed_wheel("clang-tidy", requested) is None + + +def test_find_installed_wheel_without_executable(): + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="21.1.6"), + patch("shutil.which", return_value=None), + ): + assert _find_installed_wheel("clang-tidy", "21") is None + + +@pytest.mark.parametrize("requested", [None, "21", "21.1.6"]) +def test_resolve_install_uses_matching_wheel_without_pypi(requested): + """A matching wheel (e.g. from additional_dependencies) needs no network.""" + with ( + patch( + "cpp_linter_hooks.util._find_installed_wheel", + return_value=(Path("/venv/bin/clang-tidy"), "21.1.6"), + ), + patch("cpp_linter_hooks.util._get_pypi_versions") as mock_pypi, + patch("cpp_linter_hooks.util._install_tool") as mock_install, + ): + result = resolve_install_with_diagnostics("clang-tidy", requested) + + assert result == (Path("/venv/bin/clang-tidy"), None) + mock_pypi.assert_not_called() + mock_install.assert_not_called() + + +def test_resolve_install_unpinned_keeps_installed_wheel_when_pypi_is_newer(): + """Without --version an installed wheel is not upgraded behind the user's back.""" + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="21.1.6"), + patch("shutil.which", return_value="/venv/bin/clang-tidy"), + patch( + "cpp_linter_hooks.util._get_pypi_versions", + return_value=("23.1.3", ["23.1.3", "21.1.6"]), + ) as mock_pypi, + patch("cpp_linter_hooks.util._install_tool") as mock_install, + ): + path, error = resolve_install_with_diagnostics("clang-tidy", None) + + assert (path, error) == (Path("/venv/bin/clang-tidy"), None) + mock_pypi.assert_not_called() + mock_install.assert_not_called() + + +def test_resolve_install_offline_with_pinned_installed_wheel(): + """--version works offline when the hook environment already has the wheel.""" + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="21.1.6"), + patch("shutil.which", return_value="/venv/bin/clang-tidy"), + patch("urllib.request.urlopen", side_effect=OSError("network down")), + ): + assert resolve_install_with_diagnostics("clang-tidy", "21") == ( + Path("/venv/bin/clang-tidy"), + None, + ) + + +def test_resolve_install_other_installed_wheel_resolves_from_pypi(): + with ( + patch("cpp_linter_hooks.util._installed_wheel_version", return_value="23.1.3"), + patch("shutil.which", return_value=None), + patch( + "cpp_linter_hooks.util._get_pypi_versions", side_effect=_pypi_side_effect + ), + patch( + "cpp_linter_hooks.util._install_tool", + return_value=Path("/venv/bin/clang-tidy"), + ) as mock_install, + ): + result = resolve_install_with_diagnostics("clang-tidy", "21") + + assert result == (Path("/venv/bin/clang-tidy"), None) + mock_install.assert_called_once_with("clang-tidy", "21.1.6") + + +def test_resolve_install_verbose_reports_installed_wheel(capsys): + with patch( + "cpp_linter_hooks.util._find_installed_wheel", + return_value=(Path("/venv/bin/clang-format"), "21.1.8"), + ): + resolve_install_with_diagnostics("clang-format", "21", True) + + assert ( + "Using installed clang-format Python wheel version 21.1.8" + in capsys.readouterr().err + )