diff --git a/AGENTS.md b/AGENTS.md index e5552dc..7bef9d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,8 +23,8 @@ 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` | Installed-wheel check, PyPI version resolution + pip install (prefix matching, offline fallback) | +| `pyproject.toml` | Build config, deps (`pip>=26.1`, `filelock>=3.16`), dev deps, entry points | +| `cpp_linter_hooks/util.py` | Installed-wheel check, PyPI version resolution + pip install (prefix matching, offline fallback), all under a `filelock` lock on the hook environment so parallel hook processes install once | | `cpp_linter_hooks/clang_format.py` | clang-format wrapper (`--verbose`, `--dry-run`) | | `cpp_linter_hooks/clang_tidy.py` | clang-tidy wrapper (`--compile-commands`, missing-database policy, `--jobs`, `--fix`, `--all-lines`, `-quiet` + status-line filter, `--checks` repair, error hints) | | `cpp_linter_hooks/changed_lines.py` | Added line ranges per file from `git diff` (staged changes, or `PRE_COMMIT_FROM_REF...PRE_COMMIT_TO_REF`) | diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index dac3565..d129cb2 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -1,5 +1,6 @@ """Shared helpers for resolving and installing clang tool wheels.""" +import contextlib import json import logging import os @@ -162,7 +163,9 @@ def _installed_wheel_version(tool: str) -> str | None: 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. + METADATA file the way ``importlib.metadata.version`` does. A wheel counts + only once its ``RECORD`` file exists: pip writes it last, so a wheel another + hook process is still installing is not used half-written. """ prefix = f"{re.sub(r'[-_.]+', '_', tool).lower()}-" suffix = ".dist-info" @@ -175,7 +178,11 @@ def _installed_wheel_version(tool: str) -> str | None: continue for name in names: lower = name.lower() - if lower.startswith(prefix) and lower.endswith(suffix): + if ( + lower.startswith(prefix) + and lower.endswith(suffix) + and os.path.isfile(os.path.join(site_dir, name, "RECORD")) + ): return name[len(prefix) : -len(suffix)] return None @@ -207,6 +214,38 @@ def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | return Path(path), installed +# Seconds to wait for another hook process to finish installing. +_INSTALL_LOCK_TIMEOUT = 300 + + +@contextlib.contextmanager +def _install_lock(): + """Let one hook process at a time install tools into this environment. + + pre-commit and prek run the hook on batches of files in parallel. Without + the lock, every batch runs ``pip install`` into the same directory at once. + If the lock is not free within 5 minutes, or the file system does not support + locks (some NFS mounts), the hook runs unlocked, as before. The limit also + covers filelock 3.x, which keeps retrying when the file system refuses the + lock, and a stale lock file left by a crashed process. + filelock is imported here because it is only needed to install, and + importing it takes longer than a whole hook run that finds the tool. + """ + from filelock import FileLock + + lock = FileLock(os.path.join(sys.prefix, ".cpp-linter-hooks-install.lock")) + try: + lock.acquire(timeout=_INSTALL_LOCK_TIMEOUT) + except (OSError, NotImplementedError) as exc: # filelock.Timeout is an OSError + LOG.warning("Installing without a lock: %s", exc) + lock = None + try: + yield + finally: + if lock is not None: + lock.release() + + def _install_tool(tool: str, version: str) -> Path | None: """Install a tool using pip, logging output on failure.""" result = subprocess.run( @@ -233,17 +272,30 @@ def resolve_install_with_diagnostics( ``additional_dependencies`` are kept and the hook works offline. Otherwise the version is resolved dynamically from PyPI — no hardcoded list is maintained in-tree. + + Installing runs under :func:`_install_lock`, so when several hook processes + start at once, one installs and the others wait and then use its wheel. """ 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 + if installed_wheel is None: + with _install_lock(): + # Another hook process may have installed it while this one waited. + installed_wheel = _find_installed_wheel(tool, version) + if installed_wheel is None: + return _resolve_and_install(tool, version, verbose) + path, installed = installed_wheel + if verbose: + print( + f"Using installed {tool} Python wheel version {installed}", + file=sys.stderr, + ) + return path, None + +def _resolve_and_install( + tool: str, version: str | None, verbose: bool +) -> tuple[Path | None, str | None]: + """Resolve *version* on PyPI and install it, unless it is already on PATH.""" user_version, error = _resolve_version_from_pypi(tool, version) if error is not None: return None, error diff --git a/pyproject.toml b/pyproject.toml index 09c542d..89225d7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ classifiers = [ "Topic :: Software Development :: Build Tools", ] dependencies = [ + "filelock>=3.16", # One hook process at a time installs clang tools "pip>=26.1", # Required to install clang-format and clang-tidy ] dynamic = ["version"] diff --git a/tests/test_util.py b/tests/test_util.py index 11d0d0c..563141b 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -1,10 +1,12 @@ """Tests for cpp_linter_hooks.util -- dynamic PyPI version resolution.""" +import errno import json import logging import subprocess import sys import sysconfig +import textwrap from pathlib import Path from unittest.mock import patch @@ -15,6 +17,7 @@ _env_paths, _find_installed_wheel, _get_pypi_versions, + _install_lock, _install_tool, _installed_wheel_version, _is_version_installed, @@ -740,6 +743,12 @@ def test_env_paths_match_sysconfig(): assert _env_paths() == sysconfig.get_paths() +def _installed(dist_info: Path) -> None: + """Create a dist-info directory the way pip leaves it after an install.""" + dist_info.mkdir(parents=True) + (dist_info / "RECORD").touch() + + def test_installed_wheel_version_reads_dist_info_name(tmp_path): for name in ( "clang_tidy_extra-1.0.dist-info", @@ -747,7 +756,7 @@ def test_installed_wheel_version_reads_dist_info_name(tmp_path): "clang_tidy", "Clang_Tidy-21.1.6.dist-info", ): - (tmp_path / name).mkdir() + _installed(tmp_path / name) 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" @@ -756,14 +765,21 @@ def test_installed_wheel_version_reads_dist_info_name(tmp_path): 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) + _installed(platlib / "clang_tidy-21.1.6.dist-info") 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): +def test_installed_wheel_version_ignores_unfinished_install(tmp_path): + """pip writes RECORD last; without it another process is still installing.""" (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-format") is None + + +def test_installed_wheel_version_missing_distribution(tmp_path): + _installed(tmp_path / "clang_format-22.1.8.dist-info") with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): assert _installed_wheel_version("clang-tidy") is None with patch( @@ -912,3 +928,145 @@ def test_resolve_install_verbose_reports_installed_wheel(capsys): "Using installed clang-format Python wheel version 21.1.8" in capsys.readouterr().err ) + + +# ═══════════════════════════════════════════════════════════════════════ +# _install_lock +# ═══════════════════════════════════════════════════════════════════════ + +LOCK_WORKER = textwrap.dedent( + """ + import os, sys, time + sys.prefix = sys.argv[1] + from cpp_linter_hooks.util import _install_lock + + log = sys.argv[2] + with _install_lock(): + with open(log, "a") as f: + f.write("start\\n") + time.sleep(0.2) + with open(log, "a") as f: + f.write("end\\n") + """ +) + + +def test_install_lock_serializes_processes(tmp_path): + """Hook processes started at once take turns instead of installing together.""" + log = tmp_path / "log.txt" + workers = [ + subprocess.Popen( + [sys.executable, "-c", LOCK_WORKER, str(tmp_path), str(log)], + ) + for _ in range(3) + ] + for worker in workers: + assert worker.wait(timeout=60) == 0 + + assert log.read_text().split() == ["start", "end"] * 3 + assert (tmp_path / ".cpp-linter-hooks-install.lock").is_file() + + +def test_install_lock_unlocks_on_error(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with pytest.raises(RuntimeError), _install_lock(): + _crash() + with _install_lock(): + pass + + +def _crash(): + raise RuntimeError("pip crashed") + + +@pytest.mark.parametrize( + "error", + [ + OSError(errno.ENOLCK, "No locks available"), + NotImplementedError("FileSystem does not appear to support flock"), + TimeoutError("The file lock could not be acquired"), + ], +) +def test_install_lock_runs_unlocked_without_lock_support( + tmp_path, monkeypatch, caplog, error +): + """File systems without lock support (e.g. some NFS mounts) run unlocked.""" + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with ( + patch("filelock.FileLock.acquire", side_effect=error), + caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), + _install_lock(), + ): + entered = True + assert entered + assert "Installing without a lock" in caplog.text + + +def test_install_lock_waits_a_limited_time(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with ( + patch("filelock.FileLock.acquire") as acquire, + patch("filelock.FileLock.release"), + _install_lock(), + ): + pass + acquire.assert_called_once_with(timeout=300) + + +def test_resolve_install_runs_under_install_lock(): + events = [] + + class Lock: + def __enter__(self): + events.append("lock") + + def __exit__(self, *exc): + events.append("unlock") + + def install(tool, version): + events.append("install") + return Path("/venv/bin/clang-format") + + with ( + patch("cpp_linter_hooks.util._install_lock", return_value=Lock()), + 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", side_effect=install), + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + assert events == ["lock", "install", "unlock"] + + +def test_resolve_install_skips_lock_when_installed(): + with ( + patch( + "cpp_linter_hooks.util._find_installed_wheel", + return_value=(Path("/venv/bin/clang-format"), "20.1.8"), + ), + patch("cpp_linter_hooks.util._install_lock") as lock, + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + lock.assert_not_called() + + +def test_resolve_install_uses_wheel_installed_while_waiting(): + """Another hook process installed the wheel while this one waited for the lock.""" + with ( + patch( + "cpp_linter_hooks.util._find_installed_wheel", + side_effect=[None, (Path("/venv/bin/clang-format"), "20.1.8")], + ), + patch("cpp_linter_hooks.util._get_pypi_versions") as pypi, + patch("cpp_linter_hooks.util._install_tool") as install, + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + pypi.assert_not_called() + install.assert_not_called() diff --git a/uv.lock b/uv.lock index 27d611b..6c34416 100644 --- a/uv.lock +++ b/uv.lock @@ -152,6 +152,7 @@ wheels = [ name = "cpp-linter-hooks" source = { editable = "." } dependencies = [ + { name = "filelock" }, { name = "pip" }, ] @@ -164,7 +165,10 @@ dev = [ ] [package.metadata] -requires-dist = [{ name = "pip", specifier = ">=26.1" }] +requires-dist = [ + { name = "filelock", specifier = ">=3.16" }, + { name = "pip", specifier = ">=26.1" }, +] [package.metadata.requires-dev] dev = [