Repository: cpp-linter/cpp-linter-hooks (language: python)

Issue:
clang-tools are installed into home directory, making pre-commit installation mutate system
By running `clang-tools install`, the pre-commit hooks installs the tools globally for the user (on my system into ~/.local/bin).

This is bad: pre-commit hooks should install their software into the virtual environments that are created for them and not mutate the system outside of that.


Discussion / hints:

Fix patch:
diff --git a/.gitignore b/.gitignore
index ab736da..104d3fd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,6 +5,7 @@ cpp_linter_hooks/__pycache__/
 tests/.coverage
 tests/__pycache__
 .coverage
+coverage.xml
 __pycache__
 venv
 result.txt
diff --git a/cpp_linter_hooks/__init__.py b/cpp_linter_hooks/__init__.py
index 8769504..e69de29 100644
--- a/cpp_linter_hooks/__init__.py
+++ b/cpp_linter_hooks/__init__.py
@@ -1,19 +0,0 @@
-import sys
-
-from cpp_linter_hooks.util import check_installed
-from cpp_linter_hooks.util import get_expect_version
-
-
-clang_tools = ['clang-format', 'clang-tidy']
-args = list(sys.argv[1:])
-
-expect_version = get_expect_version(args)
-
-for tool in clang_tools:
-    if expect_version:
-        retval = check_installed(tool, version=expect_version)
-    else:
-        retval = check_installed(tool)
-
-    if retval != 0:
-        raise SystemError("clang_tools not found. exit!")
diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py
index 438da87..969f2ec 100644
--- a/cpp_linter_hooks/clang_format.py
+++ b/cpp_linter_hooks/clang_format.py
@@ -1,36 +1,37 @@
 import subprocess
+from argparse import ArgumentParser
+from typing import Tuple
 
-from cpp_linter_hooks import args
-from cpp_linter_hooks import expect_version
+from .util import ensure_installed, DEFAULT_CLANG_VERSION
 
 
-def run_clang_format(args) -> int:
-    if expect_version:
-        command = [f'clang-format-{expect_version}', '-i']
-    else:
-        command = ["clang-format", '-i']
-    for arg in args:
-        if arg == expect_version or arg.startswith("--version"):
-            continue
-        command.append(arg)
+parser = ArgumentParser()
+parser.add_argument("--version", default=DEFAULT_CLANG_VERSION)
+
+
+def run_clang_format(args=None) -> Tuple[int, str]:
+    hook_args, other_args = parser.parse_known_args(args)
+    path = ensure_installed("clang-format", hook_args.version)
+    command = [str(path), '-i']
+    command.extend(other_args)
 
     retval = 0
     output = ""
     try:
         if "--dry-run" in command:
-            sp = subprocess.run(command, stdout=subprocess.PIPE)
+            sp = subprocess.run(command, stdout=subprocess.PIPE, encoding="utf-8")
             retval = -1  # Not a fail just identify it's a dry-run.
-            output = sp.stdout.decode("utf-8")
+            output = sp.stdout
         else:
             retval = subprocess.run(command, stdout=subprocess.PIPE).returncode
         return retval, output
     except FileNotFoundError as stderr:
         retval = 1
-        return retval, stderr
+        return retval, str(stderr)
 
 
 def main() -> int:
-    retval, output = run_clang_format(args)
+    retval, output = run_clang_format()
     if retval != 0:
         print(output)
     return retval
diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py
index a029d7d..7095378 100644
--- a/cpp_linter_hooks/clang_tidy.py
+++ b/cpp_linter_hooks/clang_tidy.py
@@ -1,35 +1,36 @@
 import subprocess
+from argparse import ArgumentParser
+from typing import Tuple
 
-from cpp_linter_hooks import args
-from cpp_linter_hooks import expect_version
+from .util import ensure_installed, DEFAULT_CLANG_VERSION
 
 
-def run_clang_tidy(args) -> int:
-    if expect_version:
-        command = [f'clang-tidy-{expect_version}']
-    else:
-        command = ["clang-tidy"]
-    for arg in args:
-        if arg == expect_version or arg.startswith("--version"):
-            continue
-        command.append(arg)
+parser = ArgumentParser()
+parser.add_argument("--version", default=DEFAULT_CLANG_VERSION)
+
+
+def run_clang_tidy(args=None) -> Tuple[int, str]:
+    hook_args, other_args = parser.parse_known_args(args)
+    path = ensure_installed("clang-tidy", hook_args.version)
+    command = [str(path)]
+    command.extend(other_args)
 
     retval = 0
     output = ""
     try:
-        sp = subprocess.run(command, stdout=subprocess.PIPE)
+        sp = subprocess.run(command, stdout=subprocess.PIPE, encoding='utf-8')
         retval = sp.returncode
-        output = sp.stdout.decode("utf-8")
+        output = sp.stdout
         if "warning:" in output or "error:" in output:
             retval = 1
         return retval, output
     except FileNotFoundError as stderr:
         retval = 1
-        return retval, stderr
+        return retval, str(stderr)
 
 
 def main() -> int:
-    retval, output = run_clang_tidy(args)
+    retval, output = run_clang_tidy()
     if retval != 0:
         print(output)
     return retval
diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py
index 101aa02..a3cadc5 100644
--- a/cpp_linter_hooks/util.py
+++ b/cpp_linter_hooks/util.py
@@ -1,43 +1,48 @@
-import subprocess
-
-
-def check_installed(tool: str, version="") -> int:
-    if version:
-        check_version_cmd = [f'{tool}-{version} ', '--version']
-    else:
-        check_version_cmd = [tool, '--version']
-    try:
-        subprocess.run(check_version_cmd, stdout=subprocess.PIPE)
-        retval = 0
-    except FileNotFoundError:
-        retval = install_clang_tools(version)
-    return retval
-
-
-def install_clang_tools(version: str) -> int:
-    if version:
-        # clang-tools exist because install_requires=['clang-tools'] in setup.py
-        install_tool_cmd = ['clang-tools', '-i', version]
-    else:
-        # install version 13 by default if clang-tools not exist.
-        install_tool_cmd = ['clang-tools', '-i', '13']
-    try:
-        subprocess.run(install_tool_cmd, stdout=subprocess.PIPE)
-        retval = 0
-    except Exception:
-        retval = 1
-    return retval
-
-
-def get_expect_version(args) -> str:
-    for arg in args:
-        if arg.startswith("--version"):  # expect specific clang-tools version.
-            # If --version is passed in as 2 arguments, the second is version
-            if arg == "--version" and args.index(arg) != len(args) - 1:
-                # when --version 14
-                expect_version = args[args.index(arg) + 1]
-            else:
-                # when --version=14
-                expect_version = arg.replace(" ", "").replace("=", "").replace("--version", "")
-            return expect_version
-    return ""
+import sys
+from pathlib import Path
+import logging
+from typing import Optional
+
+from clang_tools.install import is_installed as _is_installed, install_tool
+
+
+LOG = logging.getLogger(__name__)
+
+
+DEFAULT_CLANG_VERSION = "13"
+
+
+def is_installed(tool_name: str, version: str) -> Optional[Path]:
+    """Check if tool is installed.
+
+    Checks the current python prefix and PATH via clang_tools.install.is_installed.
+    """
+    # check in current python prefix (usual situation when we installed into pre-commit venv)
+    directory = Path(sys.executable).parent
+    path = (directory / f"{tool_name}-{version}")
+    if path.is_file():
+        return path
+
+    # also check using clang_tools
+    path = _is_installed(tool_name, version)
+    if path is not None:
+        return Path(path)
+
+    # not found
+    return None
+
+
+def ensure_installed(tool_name: str, version: str = DEFAULT_CLANG_VERSION) -> Path:
+    """
+    Ensure tool is available at given version.
+    """
+    LOG.info("Checking for %s, version %s", tool_name, version)
+    path = is_installed(tool_name, version)
+    if path is not None:
+        LOG.info("%s, version %s is already installed", tool_name, version)
+        return path
+
+    LOG.info("Installing %s, version %s", tool_name, version)
+    directory = Path(sys.executable).parent
+    install_tool(tool_name, version, directory=str(directory), no_progress_bar=True)
+    return directory / f"{tool_name}-{version}"


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index d903f82..37357de 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -13,10 +13,10 @@ jobs:
     steps:
       - name: Checkout
         uses: actions/checkout@v4
-      - name: Set up Python 3.10
+      - name: Set up Python 3.12
         uses: actions/setup-python@v5
         with:
-          python-version: "3.10"
+          python-version: "3.12"
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip
diff --git a/testing/good.c b/testing/good.c
new file mode 100644
index 0000000..d907942
--- /dev/null
+++ b/testing/good.c
@@ -0,0 +1,6 @@
+#include <stdio.h>
+int main() {
+  for (;;) break;
+  printf("Hello world!\n");
+  return 0;
+}
diff --git a/testing/.pre-commit-config.yaml b/testing/pre-commit-config-version.yaml
similarity index 65%
rename from testing/.pre-commit-config.yaml
rename to testing/pre-commit-config-version.yaml
index 14e7205..5b7b9b9 100644
--- a/testing/.pre-commit-config.yaml
+++ b/testing/pre-commit-config-version.yaml
@@ -1,6 +1,6 @@
 repos:
-  - repo: https://github.com/cpp-linter/cpp-linter-hooks
-    rev: 2a92e91720ca4bc79d67c3e4aea57642f598d534
+  - repo: .
+    rev: HEAD
     hooks:
       - id: clang-format
         args: [--style=file, --version=16]  # to load .clang-format
diff --git a/testing/pre-commit-config.yaml b/testing/pre-commit-config.yaml
new file mode 100644
index 0000000..f92efec
--- /dev/null
+++ b/testing/pre-commit-config.yaml
@@ -0,0 +1,8 @@
+repos:
+  - repo: .
+    rev: HEAD
+    hooks:
+      - id: clang-format
+        args: [--style=file]  # to load .clang-format
+      - id: clang-tidy
+        args: [--checks=.clang-tidy] # path/to/.clang-tidy
diff --git a/testing/run.sh b/testing/run.sh
index 5d96fac..7a88fcc 100644
--- a/testing/run.sh
+++ b/testing/run.sh
@@ -1,9 +1,15 @@
-pre-commit install
-pre-commit try-repo . -c testing/.pre-commit-config.yaml --files testing/main.c | tee result.txt || true
+rm -f result.txt
+git restore testing/main.c
+
+for config in testing/pre-commit-config.yaml testing/pre-commit-config-version.yaml; do
+    pre-commit clean
+    pre-commit run -c $config --files testing/main.c | tee -a result.txt || true
+    git restore testing/main.c
+done
 
 failed_cases=`grep -c "Failed" result.txt`
 
-if [ $failed_cases -eq 2 ]; then
+if [ $failed_cases -eq 4 ]; then
     echo "=============================="
     echo "Test cpp-linter-hooks success."
     echo "=============================="
diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py
index 04e40f2..7a1b7b7 100644
--- a/tests/test_clang_format.py
+++ b/tests/test_clang_format.py
@@ -1,34 +1,33 @@
-from unittest.mock import patch
-
 import pytest
+from pathlib import Path
 
 from cpp_linter_hooks.clang_format import run_clang_format
 
 
-@pytest.mark.skip(reason="don't know hwo to pass test.")
 @pytest.mark.parametrize(
     ('args', 'expected_retval'), (
-        (['clang-format', '-i', '--style=Google', 'testing/main.c'], 0),
-        (['clang-format', '-i', '--style=Google', '--version=13', 'testing/main.c'], 0),
+        (['--style=Google'], (0, "")),
+        (['--style=Google', '--version=16'], (0, "")),
     ),
 )
-@patch('cpp_linter_hooks.clang_format.subprocess.run')
-def test_run_clang_format_valid(mock_subprocess_run, args, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    ret = run_clang_format(args)
+def test_run_clang_format_valid(args, expected_retval, tmp_path):
+    # copy test file to tmp_path to prevent modifying repo data
+    test_file = tmp_path / "main.c"
+    test_file.write_bytes(Path("testing/main.c").read_bytes())
+    ret = run_clang_format(args + [str(test_file)])
     assert ret == expected_retval
+    assert test_file.read_text() == Path("testing/good.c").read_text()
 
 
 @pytest.mark.parametrize(
     ('args', 'expected_retval'), (
-        (['clang-format', '-i', '--style=Google', 'abc/def.c'], 1),
-        (['clang-format', '-i', '--style=Google', '--version=13', 'abc/def.c'], 1),
+        (['--style=Google',], 1),
+        (['--style=Google', '--version=16'], 1),
     ),
 )
-@patch('cpp_linter_hooks.clang_format.subprocess.run', side_effect=FileNotFoundError)
-def test_run_clang_format_invalid(mock_subprocess_run, args, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    try:
-        ret = run_clang_format(args)
-    except FileNotFoundError:
-        assert ret == expected_retval
+def test_run_clang_format_invalid(args, expected_retval, tmp_path):
+    # non existent file
+    test_file = tmp_path / "main.c"
+
+    ret, _ = run_clang_format(args + [str(test_file)])
+    assert ret == expected_retval
diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py
index 8e7c9f7..acf5f7a 100644
--- a/tests/test_clang_tidy.py
+++ b/tests/test_clang_tidy.py
@@ -1,34 +1,34 @@
-from unittest.mock import patch
-
 import pytest
+from pathlib import Path
 
 from cpp_linter_hooks.clang_tidy import run_clang_tidy
 
 
-@pytest.mark.skip(reason="don't know hwo to pass test.")
+@pytest.mark.skip(reason="see https://github.com/cpp-linter/cpp-linter-hooks/pull/29")
 @pytest.mark.parametrize(
     ('args', 'expected_retval'), (
-        (['clang-tidy', '--checks="boost-*"', 'testing/main.c'], "stdout"),
-        (['clang-tidy', '-checks="boost-*"', '--version=13', 'testing/main.c'], "stdout"),
+        (['--checks="boost-*"'], 1),
+        (['--checks="boost-*"', '--version=16'], 1),
     ),
 )
-@patch('cpp_linter_hooks.clang_tidy.subprocess.run')
-def test_run_clang_tidy_valid(mock_subprocess_run, args, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    ret = run_clang_tidy(args)
+def test_run_clang_tidy_valid(args, expected_retval, tmp_path):
+    # copy test file to tmp_path to prevent modifying repo data
+    test_file = tmp_path / "main.c"
+    test_file.write_bytes(Path("testing/main.c").read_bytes())
+    ret, output = run_clang_tidy(args + [str(test_file)])
     assert ret == expected_retval
+    print(output)
 
 
 @pytest.mark.parametrize(
     ('args', 'expected_retval'), (
-        (['clang-tidy', '-i', '--checks="boost-*"', 'abc/def.c'], ""),
-        (['clang-tidy', '-i', '--checks="boost-*"', '--version=13', 'abc/def.c'], ""),
+        (['--checks="boost-*"'], 1),
+        (['--checks="boost-*"', '--version=16'], 1),
     ),
 )
-@patch('cpp_linter_hooks.clang_tidy.subprocess.run', side_effect=FileNotFoundError)
-def test_run_clang_tidy_invalid(mock_subprocess_run, args, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    try:
-        ret = run_clang_tidy(args)
-    except FileNotFoundError:
-        assert ret == expected_retval
+def test_run_clang_tidy_invalid(args, expected_retval, tmp_path):
+    # non existent file
+    test_file = tmp_path / "main.c"
+
+    ret, _ = run_clang_tidy(args + [str(test_file)])
+    assert ret == expected_retval
diff --git a/tests/test_util.py b/tests/test_util.py
index 05b17b6..ba442b8 100644
--- a/tests/test_util.py
+++ b/tests/test_util.py
@@ -1,52 +1,40 @@
-from unittest.mock import patch
-
+import logging
+import sys
 import pytest
+from itertools import product
 
-from cpp_linter_hooks.util import check_installed
-from cpp_linter_hooks.util import get_expect_version
-from cpp_linter_hooks.util import install_clang_tools
-
-
-@pytest.mark.parametrize(('tool', 'expected_retval'), (('clang-format', 0), ('clang-tidy', 0),),)
-@patch('cpp_linter_hooks.util.subprocess.run')
-def test_check_installed(mock_subprocess_run, tool, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    ret = check_installed(tool)
-    assert ret == expected_retval
+from cpp_linter_hooks.util import ensure_installed, DEFAULT_CLANG_VERSION
 
 
-@pytest.mark.parametrize(('tool', 'version', 'expected_retval'), (('clang-format', '14', 0), ('clang-tidy', '14', 0),),)
-@patch('cpp_linter_hooks.util.subprocess.run')
-def test_check_installed_with_version(mock_subprocess_run, tool, version, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    ret = check_installed(tool, version=version)
-    assert ret == expected_retval
+VERSIONS = [None, "16"]
+TOOLS = ["clang-format", "clang-tidy"]
 
 
-@pytest.mark.parametrize(('tool', 'version', 'expected_retval'), (('non-exist-cmd', '14', 0),),)
-@patch('cpp_linter_hooks.util.subprocess.run')
-def test_check_installed_with_except(mock_subprocess_run, tool, version, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    ret = check_installed(tool, version=version)
-    assert ret == expected_retval
+@pytest.mark.skip(reason="see https://github.com/cpp-linter/cpp-linter-hooks/pull/29")
+@pytest.mark.parametrize(("tool", "version"), list(product(TOOLS, VERSIONS)))
+def test_ensure_installed(tool, version, tmp_path, monkeypatch, caplog):
 
+    bin_path = tmp_path / "bin"
+    with monkeypatch.context() as m:
+        m.setattr(sys, "executable", str(bin_path / "python"))
 
-@pytest.mark.parametrize(('version', 'expected_retval'), (('100', 1),),)
-@patch('cpp_linter_hooks.util.subprocess.run')
-def test_install_clang_tools(mock_subprocess_run, version, expected_retval):
-    mock_subprocess_run.return_value = expected_retval
-    try:
-        ret = install_clang_tools(version)
-        assert ret == expected_retval
-    except Exception:
-        pass
+        for run in range(2):
+            # clear any existing log messages
+            caplog.clear()
+            caplog.set_level(logging.INFO, logger="cpp_linter_hooks.util")
 
+            if version is not None:
+                ensure_installed(tool, version=version)
+            else:
+                ensure_installed(tool)
 
-def test_get_expect_version():
-    args = ['clang-format', '--version 14']
-    version = get_expect_version(args)
-    assert version == '14'
+            bin_version = version or DEFAULT_CLANG_VERSION
+            assert (bin_path / f"{tool}-{bin_version}").is_file()
 
-    args = ['clang-format', '--version=14']
-    version = get_expect_version(args)
-    assert version == '14'
+            # first run should install
+            assert caplog.record_tuples[0][2] == f"Checking for {tool}, version {bin_version}"
+            if run == 0:
+                assert caplog.record_tuples[1][2] == f"Installing {tool}, version {bin_version}"
+            # second run should just confirm it's already installed
+            else:
+                assert caplog.record_tuples[1][2] == f"{tool}, version {bin_version} is already installed"