Fix force overwriting symlink issue (#20) (#22)

* Fix force overwriting issue (#20)

* Fix failed test

* Updated after review

* Use `-f` to update symlink if the tool already exists

also make an assertion that the
symlinked target exists before creating one

* Remove dead code and complete a docstr.

* Improve coverage and remove unused import

* Simplify logic

Co-authored-by: Brendan <2bndy5@gmail.com>

Author: shenxianpeng

.gitignore

@@ -11,3 +11,4 @@ tests/pytestdebug.log
 tests/__pycache__/
 .eggs
 .mypy_cache/
+*env

clang_tools/install.py

@@ -6,7 +6,6 @@
 """
 import os
 import shutil
-import subprocess
 import sys
 from pathlib import Path
 from pathlib import PurePath
@@ -17,26 +16,6 @@
 from . import YELLOW
 from .util import download_file
 
-# pylint: disable=fixme
-
-
-def is_installed(tool_name: str, version: str) -> bool:
-    """An abstract functions to check if a specified clang tool is installed.
-
-    :param tool_name: The name of the tool.
-    :param version: The version of the tool to look for. If provided a blank string,
-        then only the ``tool_name`` is executed during the check.
-    :returns: A `bool` describing if the tool was installed and executed properly.
-    """
-    command = [tool_name, "--version"]
-    if version:
-        command[0] += f"-{version}"
-    try:
-        result = subprocess.run(command, capture_output=True, check=True)
-        return result.returncode == 0
-    except FileNotFoundError:
-        return False
-
 
 def clang_tools_binary_url(
     tool: str, version: str, release_tag: str = "master-208096c1"
@@ -58,9 +37,16 @@ def clang_tools_binary_url(
 
 
 def install_tool(tool_name: str, version: str, directory: str) -> bool:
-    """An abstract function that can install either clang-tidy or clang-format."""
-    if is_installed(tool_name, version):
-        # TODO should probably skip this if `directory` is not in the PATH env var.
+    """An abstract function that can install either clang-tidy or clang-format.
+
+    :param tool_name: The name of the clang-tool to install.
+    :param version: The version of the tools to install.
+    :param directory: The installation directory.
+
+    :returns: `True` if the binary had to be downloaded and installed.
+        `False` if the binary was not downloaded but is installed in ``directory``.
+    """
+    if Path(directory, f"{tool_name}-{version}{suffix}").exists():
         print(f"{tool_name}-{version}", "already installed")
         return False
     bin_url = clang_tools_binary_url(tool_name, version)
@@ -142,6 +128,7 @@ def create_sym_link(
             return False
         os.remove(str(link))
         print("overwriting symbolic link", str(link))
+    assert target.exists()
     link.symlink_to(target)
     print("symbolic link created", str(link))
     return True
@@ -162,5 +149,6 @@ def install_clang_tools(version: str, directory: str, overwrite: bool) -> None:
             f"directory is not in your environment variable PATH.{RESET_COLOR}",
         )
     for tool_name in ("clang-format", "clang-tidy"):
-        if install_tool(tool_name, version, install_dir):
-            create_sym_link(tool_name, version, install_dir, overwrite)
+        install_tool(tool_name, version, install_dir)
+        # `install_tool()` guarantees that the binary exists now
+        create_sym_link(tool_name, version, install_dir, overwrite)

tests/test_install.py

@@ -4,21 +4,13 @@
 import pytest
 from clang_tools import install_os, suffix
 from clang_tools.install import (
-    is_installed,
     clang_tools_binary_url,
     install_dir_name,
     create_sym_link,
     install_tool,
 )
 
 
-@pytest.mark.parametrize("version", ["", pytest.param("1", marks=pytest.mark.xfail)])
-@pytest.mark.parametrize("tool_name", ["clang-format", "clang-tidy"])
-def test_clang_tools_exist(tool_name: str, version: str):
-    """Test `is_installed()`"""
-    assert is_installed(tool_name, version)
-
-
 @pytest.mark.parametrize("version", [str(v) for v in range(7, 14)] + ["12.0.1"])
 @pytest.mark.parametrize("tool_name", ["clang-format", "clang-tidy"])
 def test_clang_tools_binary_url(tool_name: str, version: str):
@@ -61,4 +53,6 @@ def test_install_tools(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, version:
     """Test install tools to a temp directory."""
     monkeypatch.chdir(tmp_path)
     for tool_name in ("clang-format", "clang-tidy"):
-        install_tool(tool_name, version, str(tmp_path))
+        assert install_tool(tool_name, version, str(tmp_path))
+        # invoking again should return False
+        assert not install_tool(tool_name, version, str(tmp_path))