From bc771830e104595640c490414a08ec507dc77b67 Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Thu, 15 Oct 2026 23:37:58 +0530 Subject: [PATCH 01/11] Fix Tool Manager platform utility regressions Co-Authored-By: Claude Sonnet 5.5 --- src/toolManager/platform_utils.py | 6 +- src/toolManager/tests/test_platform_utils.py | 204 +++++++++++++++++++ 2 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 src/toolManager/tests/test_platform_utils.py diff --git a/src/toolManager/platform_utils.py b/src/toolManager/platform_utils.py index 96a1ef4c9..c983970f3 100644 --- a/src/toolManager/platform_utils.py +++ b/src/toolManager/platform_utils.py @@ -13,7 +13,7 @@ IS_WINDOWS bool IS_LINUX bool IS_MAC bool -get_msys32_path() Path (Windows only: default C:\\msys64) +get_mysys2_path() Path (Windows only: default C:\\msys64) subprocess_flags() dict (suppress console window on Windows) detect_package_manager() str | None distro_label() str @@ -33,7 +33,7 @@ __all__ = [ "IS_WINDOWS", "IS_LINUX", "IS_MAC", - "get_mysys32_path", + "get_mysys2_path", "subprocess_flags", "detect_package_manager", "distro_label", @@ -51,7 +51,7 @@ # Centralised here as the single place to change if the path changes. def get_mysys2_path() -> Path: if not IS_WINDOWS: - return RuntimeError("get_MSYS2_path() is only valid for Windows") + raise RuntimeError("get_MSYS2_path() is only valid for Windows") import os return Path(os.environ.get("MSYS2_PATH", r"C:\msys64")) diff --git a/src/toolManager/tests/test_platform_utils.py b/src/toolManager/tests/test_platform_utils.py new file mode 100644 index 000000000..0f548a336 --- /dev/null +++ b/src/toolManager/tests/test_platform_utils.py @@ -0,0 +1,204 @@ +"""Unit tests for src/toolManager/platform_utils.py. + +platform_utils decides everything at import time from ``sys.platform``, so each +test loads a FRESH copy of the module with ``sys.platform`` faked. That makes +the suite independent of the host OS: it behaves the same on a Windows dev box +and on Linux CI. External commands (``shutil.which``) and platform info +(``platform.release`` / ``freedesktop_os_release``) are mocked, so no real +package manager or tool is ever executed or installed. +""" +import importlib.util +import subprocess +import sys +from pathlib import Path + +import pytest + +_MODULE_PATH = Path(__file__).resolve().parents[1] / "platform_utils.py" + +_CREATE_NO_WINDOW = 0x08000000 + + +class _FakeStartupInfo: + """Stand-in for subprocess.STARTUPINFO (only exists on Windows).""" + + def __init__(self): + self.dwFlags = 0 + self.wShowWindow = None + + +@pytest.fixture +def load(monkeypatch): + """Return load(platform_string) -> a fresh platform_utils module as it + would import on that platform.""" + + def _load(plat): + monkeypatch.setattr(sys, "platform", plat) + if plat == "win32": + # These attributes are Windows-only; provide them (raising=False) + # so the win32 branch of subprocess_flags() can run anywhere. + monkeypatch.setattr(subprocess, "STARTUPINFO", _FakeStartupInfo, + raising=False) + monkeypatch.setattr(subprocess, "STARTF_USESHOWWINDOW", 1, + raising=False) + monkeypatch.setattr(subprocess, "SW_HIDE", 0, raising=False) + monkeypatch.setattr(subprocess, "CREATE_NO_WINDOW", + _CREATE_NO_WINDOW, raising=False) + spec = importlib.util.spec_from_file_location( + "platform_utils_under_test", _MODULE_PATH) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + return _load + + +def _fake_which(available): + """shutil.which replacement that 'finds' only the named commands.""" + names = set(available) + return lambda cmd, *a, **kw: ("/fake/bin/" + cmd) if cmd in names else None + + +# --------------------------------------------------------------------------- # +# OS flags / get_os_id +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize("plat,expected,flags", [ + ("win32", "win32", (True, False, False)), + ("linux", "linux", (False, True, False)), + ("darwin", "darwin", (False, False, True)), + ("freebsd14", "unknown", (False, False, False)), +]) +def test_get_os_id_and_flags(load, plat, expected, flags): + pu = load(plat) + assert pu.get_os_id() == expected + assert (pu.IS_WINDOWS, pu.IS_LINUX, pu.IS_MAC) == flags + + +# --------------------------------------------------------------------------- # +# detect_package_manager +# --------------------------------------------------------------------------- # +class TestDetectPackageManager: + def test_windows_returns_none_even_if_linux_tools_on_path( + self, load, monkeypatch): + pu = load("win32") + monkeypatch.setattr(pu.shutil, "which", + _fake_which({"apt-get", "brew", "choco"})) + assert pu.detect_package_manager() is None + + def test_linux_apt(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr(pu.shutil, "which", _fake_which({"apt-get"})) + assert pu.detect_package_manager() == "apt" + + def test_linux_dnf(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr(pu.shutil, "which", _fake_which({"dnf"})) + assert pu.detect_package_manager() == "dnf" + + def test_linux_priority_apt_before_dnf(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr(pu.shutil, "which", + _fake_which({"dnf", "apt-get"})) + assert pu.detect_package_manager() == "apt" + + def test_linux_no_supported_manager(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr(pu.shutil, "which", _fake_which(set())) + assert pu.detect_package_manager() is None + + def test_macos_brew(self, load, monkeypatch): + pu = load("darwin") + monkeypatch.setattr(pu.shutil, "which", _fake_which({"brew"})) + assert pu.detect_package_manager() == "brew" + + def test_macos_brew_preferred_over_port(self, load, monkeypatch): + pu = load("darwin") + monkeypatch.setattr(pu.shutil, "which", _fake_which({"port", "brew"})) + assert pu.detect_package_manager() == "brew" + + def test_macos_linux_tools_are_ignored(self, load, monkeypatch): + pu = load("darwin") + monkeypatch.setattr(pu.shutil, "which", _fake_which({"apt-get"})) + assert pu.detect_package_manager() is None + + +# --------------------------------------------------------------------------- # +# distro_label +# --------------------------------------------------------------------------- # +class TestDistroLabel: + def test_windows(self, load, monkeypatch): + pu = load("win32") + monkeypatch.setattr(pu.platform, "release", lambda: "11") + assert pu.distro_label() == "Windows 11" + + def test_linux_from_freedesktop_os_release(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr( + pu.platform, "freedesktop_os_release", + lambda: {"NAME": "Ubuntu", "VERSION_ID": "24.04"}, + raising=False) + assert pu.distro_label() == "Ubuntu 24.04" + + def test_linux_rolling_release_has_no_version(self, load, monkeypatch): + pu = load("linux") + monkeypatch.setattr(pu.platform, "freedesktop_os_release", + lambda: {"NAME": "Arch Linux"}, raising=False) + assert pu.distro_label() == "Arch Linux" + + def test_linux_os_release_unreadable_falls_back(self, load, monkeypatch): + pu = load("linux") + + def boom(): + raise OSError("no /etc/os-release") + + monkeypatch.setattr(pu.platform, "freedesktop_os_release", boom, + raising=False) + assert pu.distro_label() == "Linux" + + +# --------------------------------------------------------------------------- # +# subprocess_flags +# --------------------------------------------------------------------------- # +class TestSubprocessFlags: + def test_windows_hides_console(self, load): + pu = load("win32") + flags = pu.subprocess_flags() + assert flags["creationflags"] == _CREATE_NO_WINDOW + si = flags["startupinfo"] + assert si.dwFlags & subprocess.STARTF_USESHOWWINDOW + assert si.wShowWindow == subprocess.SW_HIDE + + @pytest.mark.parametrize("plat", ["linux", "darwin"]) + def test_non_windows_is_empty(self, load, plat): + assert load(plat).subprocess_flags() == {} + + +# --------------------------------------------------------------------------- # +# Regression tests: verify the get_mysys2_path / __all__ fixes +# --------------------------------------------------------------------------- # +class TestRegressions: + def test_all_lists_only_names_that_exist(self, load): + """``__all__`` once listed get_mysys32_path while the function is + get_mysys2_path, so ``from platform_utils import *`` raised + AttributeError. Every name in ``__all__`` must now exist.""" + pu = load("linux") + missing = [n for n in pu.__all__ if not hasattr(pu, n)] + assert missing == [] + + def test_get_mysys2_path_raises_on_non_windows(self, load): + """The function once did ``return RuntimeError(...)`` instead of + raising, so callers' ``except RuntimeError`` never fired and they + received an exception object as the path. It must now raise.""" + pu = load("linux") + with pytest.raises(RuntimeError): + pu.get_mysys2_path() + + def test_get_mysys2_path_default_on_windows(self, load, monkeypatch): + pu = load("win32") + monkeypatch.delenv("MSYS2_PATH", raising=False) + assert str(pu.get_mysys2_path()) == r"C:\msys64" + + def test_get_mysys2_path_env_override_on_windows(self, load, monkeypatch): + pu = load("win32") + monkeypatch.setenv("MSYS2_PATH", r"D:\tools\msys2") + assert str(pu.get_mysys2_path()) == r"D:\tools\msys2" From 42861e0bb4255d52c80cda7f432d6c081da4e7db Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 19:34:48 +0530 Subject: [PATCH 02/11] Run Tool Manager unit tests in CI Add a pytest job to the platform_utils workflow and trigger it on changes under src/toolManager/tests/. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test_platform_utils.yml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.github/workflows/test_platform_utils.yml b/.github/workflows/test_platform_utils.yml index 0159c9e6c..b68ef75ad 100644 --- a/.github/workflows/test_platform_utils.yml +++ b/.github/workflows/test_platform_utils.yml @@ -5,14 +5,34 @@ on: push: paths: - 'src/toolManager/platform_utils.py' + - 'src/toolManager/tests/**' - '.github/workflows/test_platform_utils.yml' pull_request: paths: - 'src/toolManager/platform_utils.py' + - 'src/toolManager/tests/**' - '.github/workflows/test_platform_utils.yml' jobs: + # ── Unit tests (platform-independent, sys.platform is mocked) ──────────────── + unit-tests: + name: Unit tests (pytest) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Install pytest + run: python -m pip install pytest + + - name: Run unit tests + run: python -m pytest src/toolManager/tests -v + # ── Ubuntu (apt) ───────────────────────────────────────────────────────────── test-ubuntu: name: Ubuntu (apt) From 44871d3532a188be1dd97be76224b136301d601d Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:19:23 +0530 Subject: [PATCH 03/11] Report unsupported and failed Linux installs honestly tool_manager_linux.py exited 0 in several cases where nothing was installed or removed, so the GUI reported success: - a detected package manager with no package map (e.g. apk) - a tool with an empty package list (e.g. ghdl/verilator on yum) - eSim on non-apt distributions - a package command that succeeded but left the tool missing - uninstalling a tool that is present but was not installed by the package manager, or eSim on apt with install-eSim.sh missing install/update/uninstall now exit 0 (success), 1 (failed) or 3 (not supported) and always print a matching status line. main.py logs a warning for unsupported tools. Adds unit tests for package selection, command construction, check/install/uninstall, elevation and the apt lock, with all system commands mocked. Co-Authored-By: Claude Opus 5.5 --- src/toolManager/main.py | 5 + src/toolManager/tests/conftest.py | 56 +++ .../tests/test_tool_manager_linux.py | 398 ++++++++++++++++++ src/toolManager/tool_manager_linux.py | 146 +++++-- 4 files changed, 563 insertions(+), 42 deletions(-) create mode 100644 src/toolManager/tests/conftest.py create mode 100644 src/toolManager/tests/test_tool_manager_linux.py diff --git a/src/toolManager/main.py b/src/toolManager/main.py index 9562f96e0..b8e037dbd 100644 --- a/src/toolManager/main.py +++ b/src/toolManager/main.py @@ -41,6 +41,9 @@ PYTHON = sys.executable +# Backend exit code for "not supported on this system" (see tool_manager_linux.py) +_EXIT_UNSUPPORTED = 3 + ANALOG_TOOLS = ["esim", "kicad", "ngspice"] DIGITAL_TOOLS = ["esim", "kicad", "ngspice", "ghdl", "verilator", "llvm"] @@ -154,6 +157,8 @@ def run(self): if "[ERROR]" in line or "_failed|" in line: success = False proc.wait() + if proc.returncode == _EXIT_UNSUPPORTED: + self.progress.emit(f" [WARN] {label} is not supported on this system - skipped.") if proc.returncode != 0: success = False except Exception as e: diff --git a/src/toolManager/tests/conftest.py b/src/toolManager/tests/conftest.py new file mode 100644 index 000000000..099d2426f --- /dev/null +++ b/src/toolManager/tests/conftest.py @@ -0,0 +1,56 @@ +"""Shared fixtures for the Tool Manager unit tests. + +The backend modules import each other by bare name (``from platform_utils +import ...``) and decide platform-specific behaviour at import time, so the +``fresh_import`` fixture imports a FRESH copy of a module with ``sys.platform`` +faked and restores ``sys.modules`` afterwards. Nothing here runs a real +package manager, Chocolatey, MSYS2 or an EDA tool. +""" +import importlib +import sys +from pathlib import Path + +# Import the stdlib modules the backends use *before* sys.platform is faked, +# so they initialise for the real host. +import argparse # noqa: F401 +import contextlib # noqa: F401 +import json # noqa: F401 +import threading # noqa: F401 +import urllib.request # noqa: F401 +import zipfile # noqa: F401 + +import pytest + +TOOLMANAGER_DIR = Path(__file__).resolve().parents[1] + +# Modules that must be re-imported together so they see the faked platform. +_BACKEND_MODULES = ("platform_utils", "utils", "tool_manager_linux", + "tool_manager_windows") + + +@pytest.fixture +def fresh_import(monkeypatch): + """Return fresh_import(module_name, platform) -> freshly imported module.""" + saved = {name: sys.modules.get(name) for name in _BACKEND_MODULES} + monkeypatch.syspath_prepend(str(TOOLMANAGER_DIR)) + + def _import(name, plat): + for mod in _BACKEND_MODULES: + sys.modules.pop(mod, None) + # Fake the platform only while importing: the modules record it in + # IS_WINDOWS & co. at import time, and pytest itself (tmp_path, ...) + # must keep seeing the real host afterwards. + with monkeypatch.context() as m: + m.setattr(sys, "platform", plat) + # tool_manager_windows creates its Download/ folder at import + # time; keep the source tree untouched while testing. + m.setattr(Path, "mkdir", lambda self, *a, **kw: None) + return importlib.import_module(name) + + yield _import + + for name, mod in saved.items(): + if mod is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = mod diff --git a/src/toolManager/tests/test_tool_manager_linux.py b/src/toolManager/tests/test_tool_manager_linux.py new file mode 100644 index 000000000..f9e891ae3 --- /dev/null +++ b/src/toolManager/tests/test_tool_manager_linux.py @@ -0,0 +1,398 @@ +"""Unit tests for src/toolManager/tool_manager_linux.py. + +Package-manager detection, elevation (pkexec/sudo) and the "is it installed?" +lookup are all mocked: tests record the command that WOULD run instead of +running it, so they need no root, no package manager and no EDA tools, and +behave the same on Windows and on Linux CI. +""" +import contextlib +import subprocess +import sys +import types + +import pytest + + +@pytest.fixture +def tml(fresh_import, monkeypatch): + monkeypatch.delenv("WSL_DISTRO_NAME", raising=False) + mod = fresh_import("tool_manager_linux", "linux") + monkeypatch.setattr(mod, "_IS_WSL", False) + return mod + + +class FakeSystem: + """Stands in for the package manager, elevation and installed-tool lookup.""" + + def __init__(self, mod, monkeypatch, pm="apt", rc=0): + self.pm = pm + self.rc = rc # exit code of the elevated command + self.installed = {} # tool -> location + self.commands = [] # every elevated command, in order + self.streamed = [] # every non-elevated command (_stream) + self.locks = [] # pm passed to _apt_lock + + monkeypatch.setattr(mod, "detect_package_manager", lambda: self.pm) + monkeypatch.setattr(mod, "_run_elevated", self._run_elevated) + monkeypatch.setattr(mod, "_stream", self._stream) + monkeypatch.setattr(mod, "_find_installed", self.installed.get) + monkeypatch.setattr(mod, "_apt_lock", self._apt_lock) + + def _run_elevated(self, args): + self.commands.append(list(args)) + return self.rc + + def _stream(self, args): + self.streamed.append(list(args)) + return self.rc + + @contextlib.contextmanager + def _apt_lock(self, pm="apt"): + self.locks.append(pm) + yield + + +def status_lines(out): + """The machine-readable state|tool|detail lines the GUI parses.""" + return [l for l in out.splitlines() if l.count("|") == 2] + + +# --------------------------------------------------------------------------- # +# Package selection and command construction +# --------------------------------------------------------------------------- # +SUPPORTED = [(pm, tool) for pm in ("apt", "dnf", "pacman", "zypper") + for tool in ("kicad", "ngspice", "ghdl", "verilator", "llvm")] + + +@pytest.mark.parametrize("pm,tool", SUPPORTED) +def test_install_builds_command_from_package_map(tml, monkeypatch, capsys, + pm, tool): + fs = FakeSystem(tml, monkeypatch, pm=pm) + fs.installed[tool] = f"/usr/bin/{tool}" + + rc = tml.install_tool(tool, "latest") + + assert rc == tml.EXIT_OK + assert fs.commands == [tml.INSTALL_CMD[pm] + tml.PACKAGE_MAP[pm][tool]] + assert status_lines(capsys.readouterr().out) == [ + f"installed|{tool}|/usr/bin/{tool}"] + + +@pytest.mark.parametrize("pm,tool", SUPPORTED) +def test_uninstall_builds_command_from_package_map(tml, monkeypatch, capsys, + pm, tool): + fs = FakeSystem(tml, monkeypatch, pm=pm) # tool absent after removal + + rc = tml.uninstall_tool(tool, "none") + + assert rc == tml.EXIT_OK + assert fs.commands == [tml.UNINSTALL_CMD[pm] + tml.PACKAGE_MAP[pm][tool]] + assert status_lines(capsys.readouterr().out) == [f"not_installed|{tool}|none"] + + +def test_package_map_and_commands_cover_the_same_managers(tml): + assert set(tml.PACKAGE_MAP) == set(tml.INSTALL_CMD) == set(tml.UNINSTALL_CMD) + + +def test_update_uses_the_install_command(tml, monkeypatch): + fs = FakeSystem(tml, monkeypatch, pm="dnf") + fs.installed["ngspice"] = "/usr/bin/ngspice" + assert tml.install_tool("ngspice", "latest", upgrade=True) == tml.EXIT_OK + assert fs.commands == [["dnf", "install", "-y", "ngspice"]] + + +def test_package_manager_command_runs_inside_the_apt_lock(tml, monkeypatch): + fs = FakeSystem(tml, monkeypatch, pm="pacman") + fs.installed["ghdl"] = "/usr/bin/ghdl" + tml.install_tool("ghdl", "latest") + fs.installed.clear() + tml.uninstall_tool("ghdl", "none") + # _apt_lock itself decides to no-op for non-apt managers (tested below). + assert fs.locks == ["pacman", "pacman"] + + +# --------------------------------------------------------------------------- # +# install: never report success when nothing was installed (regressions) +# --------------------------------------------------------------------------- # +class TestInstallReportsHonestly: + def test_no_package_manager_is_unsupported(self, tml, monkeypatch, capsys): + fs = FakeSystem(tml, monkeypatch, pm=None) + assert tml.install_tool("kicad", "latest") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] + assert "not_supported|kicad|none" in status_lines(capsys.readouterr().out) + + def test_detected_but_unmapped_manager_is_unsupported(self, tml, + monkeypatch, capsys): + """platform_utils can detect 'apk' but there is no package map for it. + This used to print not_supported and still exit 0 (= success).""" + fs = FakeSystem(tml, monkeypatch, pm="apk") + assert tml.install_tool("kicad", "latest") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == ["not_supported|kicad|apk"] + + @pytest.mark.parametrize("tool", ["ghdl", "verilator"]) + def test_empty_package_mapping_is_unsupported(self, tml, monkeypatch, + capsys, tool): + """yum has no ghdl/verilator packages; this used to exit 0 after an + [INFO] line, so the GUI said 'installed successfully'.""" + fs = FakeSystem(tml, monkeypatch, pm="yum") + assert tml.install_tool(tool, "latest") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == [f"not_supported|{tool}|yum"] + + def test_unknown_tool_is_unsupported(self, tml, monkeypatch, capsys): + fs = FakeSystem(tml, monkeypatch, pm="apt") + assert tml.install_tool("nosuchtool", "latest") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == [ + "not_supported|nosuchtool|apt"] + + @pytest.mark.parametrize("pm", ["dnf", "pacman", "zypper", "yum"]) + def test_esim_on_non_apt_distro_is_unsupported(self, tml, monkeypatch, + capsys, pm): + """eSim only installs via the apt script. Other distros used to get + exit 0 and a link, which the GUI showed as a successful install.""" + fs = FakeSystem(tml, monkeypatch, pm=pm) + assert tml.install_tool("esim", "2.4") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] and fs.streamed == [] + out = capsys.readouterr().out + assert status_lines(out) == [f"not_supported|esim|{pm}"] + assert "esim.fossee.in" in out + + def test_failed_package_command_is_a_failure(self, tml, monkeypatch, capsys): + fs = FakeSystem(tml, monkeypatch, pm="apt", rc=100) + fs.installed["kicad"] = "/usr/bin/kicad" # e.g. an older copy + assert tml.install_tool("kicad", "latest") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == ["install_failed|kicad|exit_100"] + + def test_command_succeeded_but_tool_missing_is_a_failure(self, tml, + monkeypatch, capsys): + """Used to print install_failed but still return the package + manager's exit code 0.""" + FakeSystem(tml, monkeypatch, pm="apt", rc=0) + assert tml.install_tool("verilator", "latest") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == [ + "install_failed|verilator|not_found"] + + def test_failure_exit_code_never_collides_with_unsupported(self, tml, + monkeypatch): + FakeSystem(tml, monkeypatch, pm="apt", rc=tml.EXIT_UNSUPPORTED) + assert tml.install_tool("kicad", "latest") == tml.EXIT_FAILED + + +class TestEsimOnApt: + @pytest.fixture + def script(self, tml, monkeypatch, tmp_path): + path = tmp_path / "install-eSim.sh" + path.write_text("#!/bin/bash\n") + monkeypatch.setattr(tml, "INSTALL_SH", path) + return path + + @pytest.mark.parametrize("upgrade,flag", [(False, "--install"), + (True, "--upgrade")]) + def test_install_runs_the_script(self, tml, monkeypatch, script, + upgrade, flag): + fs = FakeSystem(tml, monkeypatch, pm="apt") + assert tml.install_tool("esim", "2.4", upgrade=upgrade) == tml.EXIT_OK + assert fs.streamed == [["bash", str(script), flag]] + assert fs.commands == [] + + def test_script_failure_is_reported(self, tml, monkeypatch, capsys, script): + FakeSystem(tml, monkeypatch, pm="apt", rc=2) + assert tml.install_tool("esim", "2.4") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == ["install_failed|esim|exit_2"] + + def test_missing_script_is_a_failure(self, tml, monkeypatch, capsys, tmp_path): + monkeypatch.setattr(tml, "INSTALL_SH", tmp_path / "missing.sh") + fs = FakeSystem(tml, monkeypatch, pm="apt") + assert tml.install_tool("esim", "2.4") == tml.EXIT_FAILED + assert fs.streamed == [] + assert "install_failed|esim|script_missing" in capsys.readouterr().out + + def test_uninstall_runs_the_script(self, tml, monkeypatch, script): + fs = FakeSystem(tml, monkeypatch, pm="apt") + assert tml.uninstall_tool("esim", "none") == tml.EXIT_OK + assert fs.streamed == [["bash", str(script), "--uninstall"]] + + def test_uninstall_script_failure_is_reported(self, tml, monkeypatch, + capsys, script): + FakeSystem(tml, monkeypatch, pm="apt", rc=1) + assert tml.uninstall_tool("esim", "none") == tml.EXIT_FAILED + assert "uninstall_failed|esim|exit_1" in capsys.readouterr().out + + def test_uninstall_with_missing_script_is_a_failure(self, tml, monkeypatch, + capsys, tmp_path): + """Used to fall through to 'Nothing to uninstall' and exit 0.""" + monkeypatch.setattr(tml, "INSTALL_SH", tmp_path / "missing.sh") + FakeSystem(tml, monkeypatch, pm="apt") + assert tml.uninstall_tool("esim", "none") == tml.EXIT_FAILED + assert "uninstall_failed|esim|script_missing" in capsys.readouterr().out + + +# --------------------------------------------------------------------------- # +# uninstall +# --------------------------------------------------------------------------- # +class TestUninstall: + def test_failed_package_command_is_a_failure(self, tml, monkeypatch, capsys): + fs = FakeSystem(tml, monkeypatch, pm="dnf", rc=1) + fs.installed["ngspice"] = "/usr/bin/ngspice" + assert tml.uninstall_tool("ngspice", "none") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == [ + "uninstall_failed|ngspice|exit_1"] + + def test_tool_still_present_is_a_failure(self, tml, monkeypatch, capsys): + fs = FakeSystem(tml, monkeypatch, pm="apt", rc=0) + fs.installed["kicad"] = "/usr/local/bin/kicad" + assert tml.uninstall_tool("kicad", "none") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == [ + "uninstall_failed|kicad|still_found"] + + def test_no_package_manager_is_unsupported(self, tml, monkeypatch): + FakeSystem(tml, monkeypatch, pm=None) + assert tml.uninstall_tool("kicad", "none") == tml.EXIT_UNSUPPORTED + + @pytest.mark.parametrize("pm,tool", [("apk", "kicad"), ("yum", "ghdl"), + ("dnf", "esim")]) + def test_nothing_to_remove_when_absent_is_success(self, tml, monkeypatch, + capsys, pm, tool): + fs = FakeSystem(tml, monkeypatch, pm=pm) + assert tml.uninstall_tool(tool, "none") == tml.EXIT_OK + assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == [f"not_installed|{tool}|none"] + + @pytest.mark.parametrize("pm,tool", [("apk", "kicad"), ("yum", "ghdl"), + ("dnf", "esim")]) + def test_present_but_unmanaged_is_unsupported(self, tml, monkeypatch, + capsys, pm, tool): + """Used to claim not_installed (exit 0) even though the tool was + still on the system.""" + fs = FakeSystem(tml, monkeypatch, pm=pm) + fs.installed[tool] = f"/opt/{tool}/bin/{tool}" + assert tml.uninstall_tool(tool, "none") == tml.EXIT_UNSUPPORTED + assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == [f"not_supported|{tool}|{pm}"] + + +# --------------------------------------------------------------------------- # +# check / _find_installed +# --------------------------------------------------------------------------- # +class TestCheck: + def test_binary_on_path(self, tml, monkeypatch, capsys): + monkeypatch.setattr(tml, "_which_native", + lambda b: "/usr/bin/ngspice" if b == "ngspice" else None) + assert tml.check_tool("ngspice", "none") == 0 + assert capsys.readouterr().out.strip() == "installed|ngspice|/usr/bin/ngspice" + + def test_any_listed_binary_counts(self, tml, monkeypatch): + monkeypatch.setattr(tml, "_which_native", + lambda b: "/usr/bin/llvm-config" if b == "llvm-config" else None) + assert tml._find_installed("llvm") == "/usr/bin/llvm-config" + + def test_not_installed_on_non_apt(self, tml, monkeypatch, capsys): + monkeypatch.setattr(tml, "_which_native", lambda b: None) + monkeypatch.setattr(tml, "detect_package_manager", lambda: "dnf") + monkeypatch.setattr(tml.subprocess, "run", pytest.fail) # no dpkg here + assert tml.check_tool("ghdl", "none") == 1 + assert capsys.readouterr().out.strip() == "not_installed|ghdl|none" + + @pytest.mark.parametrize("stdout,rc,expected", [ + ("Status: install ok installed\n", 0, "dpkg"), + ("Status: deinstall ok config-files\n", 0, None), + ("", 1, None), + ]) + def test_dpkg_fallback_on_apt(self, tml, monkeypatch, stdout, rc, expected): + monkeypatch.setattr(tml, "_which_native", lambda b: None) + monkeypatch.setattr(tml, "detect_package_manager", lambda: "apt") + calls = [] + + def fake_run(cmd, **kw): + calls.append(cmd) + return subprocess.CompletedProcess(cmd, rc, stdout=stdout, stderr="") + + monkeypatch.setattr(tml.subprocess, "run", fake_run) + assert tml._find_installed("kicad") == expected + assert calls == [["dpkg", "-s", "kicad"]] + + def test_missing_dpkg_does_not_crash(self, tml, monkeypatch): + monkeypatch.setattr(tml, "_which_native", lambda b: None) + monkeypatch.setattr(tml, "detect_package_manager", lambda: "apt") + + def no_dpkg(cmd, **kw): + raise FileNotFoundError(cmd[0]) + + monkeypatch.setattr(tml.subprocess, "run", no_dpkg) + assert tml._find_installed("kicad") is None + + +# --------------------------------------------------------------------------- # +# Elevation, WSL handling and the apt lock +# --------------------------------------------------------------------------- # +class TestElevation: + @pytest.fixture + def streamed(self, tml, monkeypatch): + calls = [] + monkeypatch.setattr(tml, "_stream", lambda args: calls.append(args) or 0) + return calls + + def _which(self, available): + return lambda name: f"/usr/bin/{name}" if name in available else None + + def test_prefers_pkexec_when_usable(self, tml, monkeypatch, streamed): + monkeypatch.setattr(tml.shutil, "which", self._which({"pkexec", "sudo"})) + monkeypatch.setattr(tml, "_pkexec_usable", lambda: True) + tml._run_elevated(["apt-get", "install", "-y", "x"]) + assert streamed == [["/usr/bin/pkexec", "apt-get", "install", "-y", "x"]] + + def test_falls_back_to_sudo(self, tml, monkeypatch, streamed): + monkeypatch.setattr(tml.shutil, "which", self._which({"pkexec", "sudo"})) + monkeypatch.setattr(tml, "_pkexec_usable", lambda: False) + tml._run_elevated(["dnf", "install", "-y", "x"]) + assert streamed == [["/usr/bin/sudo", "-S", "dnf", "install", "-y", "x"]] + + def test_runs_unelevated_when_nothing_available(self, tml, monkeypatch, + streamed): + monkeypatch.setattr(tml.shutil, "which", self._which(set())) + tml._run_elevated(["pacman", "-S", "x"]) + assert streamed == [["pacman", "-S", "x"]] + + def test_pkexec_not_usable_on_wsl(self, tml, monkeypatch): + monkeypatch.setattr(tml, "_IS_WSL", True) + monkeypatch.setenv("DISPLAY", ":0") + assert tml._pkexec_usable() is False + + def test_pkexec_needs_a_display(self, tml, monkeypatch): + monkeypatch.delenv("DISPLAY", raising=False) + monkeypatch.delenv("WAYLAND_DISPLAY", raising=False) + assert tml._pkexec_usable() is False + monkeypatch.setenv("WAYLAND_DISPLAY", "wayland-0") + assert tml._pkexec_usable() is True + + def test_wsl_detected_from_environment(self, tml, monkeypatch): + monkeypatch.setenv("WSL_DISTRO_NAME", "Ubuntu") + assert tml._detect_wsl() is True + + def test_which_native_off_wsl_uses_shutil_which(self, tml, monkeypatch): + monkeypatch.setattr(tml.shutil, "which", lambda b: "/usr/bin/" + b) + assert tml._which_native("ghdl") == "/usr/bin/ghdl" + + +class TestAptLock: + def test_non_apt_managers_do_not_lock(self, tml, monkeypatch): + # Would fail if it tried to import fcntl or open the lock file. + monkeypatch.setattr(tml, "_APT_LOCK_FILE", "/nonexistent/dir/lock") + with tml._apt_lock("dnf"): + pass + + def test_apt_takes_and_releases_an_exclusive_lock(self, tml, monkeypatch, + tmp_path): + # A fake fcntl keeps this test runnable on Windows too. + calls = [] + fake_fcntl = types.SimpleNamespace( + LOCK_EX="EX", LOCK_UN="UN", + flock=lambda f, op: calls.append(op)) + monkeypatch.setitem(sys.modules, "fcntl", fake_fcntl) + monkeypatch.setattr(tml, "_APT_LOCK_FILE", str(tmp_path / "apt.lock")) + + with tml._apt_lock("apt"): + assert calls == ["EX"] + assert calls == ["EX", "UN"] diff --git a/src/toolManager/tool_manager_linux.py b/src/toolManager/tool_manager_linux.py index 0159018e5..c4d7cb840 100644 --- a/src/toolManager/tool_manager_linux.py +++ b/src/toolManager/tool_manager_linux.py @@ -1,6 +1,15 @@ #!/usr/bin/env python3 """ tool_manager_linux.py — Linux backend for eSim Tool Manager. + +Exit codes for install / update / uninstall +------------------------------------------- +0 success (status line: installed|... or not_installed|...) +1 attempted, but failed (install_failed|... or uninstall_failed|...) +3 not supported on this system (not_supported|...) + +``check`` keeps its own convention: 0 = installed, 1 = not installed. +(2 is left to argparse, which uses it for command-line errors.) """ import sys @@ -20,6 +29,10 @@ BASE_DIR = Path(__file__).resolve().parent INSTALL_SH = BASE_DIR / "install-eSim.sh" +EXIT_OK = 0 +EXIT_FAILED = 1 +EXIT_UNSUPPORTED = 3 + # ── Package names per package manager ──────────────────────────────────────── PACKAGE_MAP = { "apt": { @@ -196,72 +209,96 @@ def _apt_lock(pm: str = "apt"): # ── check ───────────────────────────────────────────────────────────────────── -def check_tool(tool: str, version: str) -> int: - """Check if a tool binary exists in PATH. Returns 0 if found, 1 if not.""" +def _find_installed(tool: str): + """Return where *tool* is installed (a binary path or "dpkg"), or None.""" binaries = _BINARY_CHECK.get(tool, [tool]) for binary in binaries: path = _which_native(binary) if path: - print(f"installed|{tool}|{path}", flush=True) - return 0 + return path # Fallback: dpkg status check on apt distros - pm = detect_package_manager() - if pm == "apt": - r = subprocess.run( - ["dpkg", "-s", tool], - capture_output=True, text=True - ) + if detect_package_manager() == "apt": + try: + r = subprocess.run( + ["dpkg", "-s", tool], + capture_output=True, text=True + ) + except OSError: + return None if r.returncode == 0 and "Status: install ok installed" in r.stdout: - print(f"installed|{tool}|dpkg", flush=True) - return 0 + return "dpkg" + return None + +def check_tool(tool: str, version: str) -> int: + """Check if a tool binary exists in PATH. Returns 0 if found, 1 if not.""" + where = _find_installed(tool) + if where: + print(f"installed|{tool}|{where}", flush=True) + return 0 print(f"not_installed|{tool}|none", flush=True) return 1 +def _unsupported(tool: str, pm, reason: str) -> int: + print(f"[INFO] {reason}", flush=True) + print(f"not_supported|{tool}|{pm or 'none'}", flush=True) + return EXIT_UNSUPPORTED + + # ── install ─────────────────────────────────────────────────────────────────── def install_tool(tool: str, version: str, upgrade: bool = False) -> int: pm = detect_package_manager() if not pm: print("[ERROR] No supported package manager found.", flush=True) - return 1 + print(f"not_supported|{tool}|none", flush=True) + return EXIT_UNSUPPORTED # Ubuntu/Debian + esim → delegate to the maintained shell script if pm == "apt" and tool == "esim": - if INSTALL_SH.exists(): - flag = "--upgrade" if upgrade else "--install" - return _stream(["bash", str(INSTALL_SH), flag]) - else: + if not INSTALL_SH.exists(): print(f"[ERROR] install-eSim.sh not found at {INSTALL_SH}", flush=True) - return 1 + print("install_failed|esim|script_missing", flush=True) + return EXIT_FAILED + flag = "--upgrade" if upgrade else "--install" + rc = _stream(["bash", str(INSTALL_SH), flag]) + if rc != 0: + print(f"install_failed|esim|exit_{rc}", flush=True) + return EXIT_FAILED + return EXIT_OK if tool == "esim": - print("[INFO] eSim has no native package for this distro.", flush=True) - print("[INFO] Please visit https://esim.fossee.in for installation options.", flush=True) - return 0 + print(f"[INFO] eSim has no native package for {pm}.", flush=True) + return _unsupported(tool, pm, "Please visit https://esim.fossee.in " + "for installation options.") packages = PACKAGE_MAP.get(pm, {}).get(tool) if packages is None: - print(f"not_supported|{tool}|{pm}", flush=True) - return 0 + return _unsupported(tool, pm, f"'{tool}' is not supported with {pm}.") if not packages: - print(f"[INFO] '{tool}' has no individual package on {pm}.", flush=True) - return 0 + return _unsupported(tool, pm, f"'{tool}' has no individual package on {pm}.") cmd_base = INSTALL_CMD.get(pm) if not cmd_base: print(f"[ERROR] No install command defined for '{pm}'.", flush=True) - return 1 + print(f"install_failed|{tool}|no_command", flush=True) + return EXIT_FAILED action = "Upgrading" if upgrade else "Installing" print(f"[Linux/{pm}] {action} {tool}: {' '.join(packages)} ...", flush=True) with _apt_lock(pm): rc = _run_elevated(cmd_base + packages) - if check_tool(tool, version) != 0: - print(f"install_failed|{tool}|none", flush=True) - return rc + if rc != 0: + print(f"install_failed|{tool}|exit_{rc}", flush=True) + return EXIT_FAILED + where = _find_installed(tool) + if not where: + print(f"install_failed|{tool}|not_found", flush=True) + return EXIT_FAILED + print(f"installed|{tool}|{where}", flush=True) + return EXIT_OK # ── uninstall ───────────────────────────────────────────────────────────────── @@ -269,26 +306,51 @@ def uninstall_tool(tool: str, version: str) -> int: pm = detect_package_manager() if not pm: print("[ERROR] No supported package manager found.", flush=True) - return 1 - + print(f"not_supported|{tool}|none", flush=True) + return EXIT_UNSUPPORTED - if pm == "apt" and tool == "esim" and INSTALL_SH.exists(): - return _stream(["bash", str(INSTALL_SH), "--uninstall"]) + if pm == "apt" and tool == "esim": + if not INSTALL_SH.exists(): + print(f"[ERROR] install-eSim.sh not found at {INSTALL_SH}", flush=True) + print("uninstall_failed|esim|script_missing", flush=True) + return EXIT_FAILED + rc = _stream(["bash", str(INSTALL_SH), "--uninstall"]) + if rc != 0: + print(f"uninstall_failed|esim|exit_{rc}", flush=True) + return EXIT_FAILED + return EXIT_OK - packages = PACKAGE_MAP.get(pm, {}).get(tool, []) + packages = PACKAGE_MAP.get(pm, {}).get(tool) if not packages: - print(f"[INFO] Nothing to uninstall for '{tool}' on {pm}.", flush=True) - print(f"not_installed|{tool}|none", flush=True) - return 0 + # Nothing this backend could have installed. That is only a success + # if the tool really is absent; otherwise we cannot remove it. + where = _find_installed(tool) + if not where: + print(f"[INFO] '{tool}' is not installed.", flush=True) + print(f"not_installed|{tool}|none", flush=True) + return EXIT_OK + return _unsupported(tool, pm, + f"'{tool}' ({where}) was not installed by {pm}; " + "remove it manually.") + + cmd_base = UNINSTALL_CMD.get(pm) + if not cmd_base: + print(f"[ERROR] No uninstall command defined for '{pm}'.", flush=True) + print(f"uninstall_failed|{tool}|no_command", flush=True) + return EXIT_FAILED - cmd_base = UNINSTALL_CMD.get(pm, []) print(f"[Linux/{pm}] Uninstalling {tool}: {' '.join(packages)} ...", flush=True) with _apt_lock(pm): rc = _run_elevated(cmd_base + packages) - - if check_tool(tool, version) == 0: - print(f"uninstall_failed|{tool}|none", flush=True) - return rc + + if rc != 0: + print(f"uninstall_failed|{tool}|exit_{rc}", flush=True) + return EXIT_FAILED + if _find_installed(tool): + print(f"uninstall_failed|{tool}|still_found", flush=True) + return EXIT_FAILED + print(f"not_installed|{tool}|none", flush=True) + return EXIT_OK if __name__ == "__main__": parser = argparse.ArgumentParser(description="eSim Tool Manager — Linux Backend") From df269bd4b2acf372f96fdeac164ae59f9ae9612a Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:20:00 +0530 Subject: [PATCH 04/11] Make utils and the Windows backend safe to import off Windows utils.py and tool_manager_windows.py called get_mysys2_path() at import time and only printed the RuntimeError it raises off Windows. That printed an "[ERROR]" line on import and left MSYS2_PATH undefined, so get_msys2_bash()/get_msys2_mingw_bin() raised NameError. MSYS2_PATH is now None off Windows and the helpers return None. Windows behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/toolManager/tests/test_utils.py | 124 ++++++++++++++++++++++++ src/toolManager/tool_manager_windows.py | 6 +- src/toolManager/utils.py | 13 ++- 3 files changed, 134 insertions(+), 9 deletions(-) create mode 100644 src/toolManager/tests/test_utils.py diff --git a/src/toolManager/tests/test_utils.py b/src/toolManager/tests/test_utils.py new file mode 100644 index 000000000..ba86303f1 --- /dev/null +++ b/src/toolManager/tests/test_utils.py @@ -0,0 +1,124 @@ +"""Unit tests for src/toolManager/utils.py and for importing the Windows +backend on other platforms. + +utils.py is imported by tool_manager_windows.py; both used to call +get_mysys2_path() unguarded at import time. Off Windows that printed an +"[ERROR]" line (which the GUI treats as a failed operation) and left +MSYS2_PATH undefined, so the MSYS2 helpers raised NameError. +""" +import subprocess +import sys + +import pytest + + +@pytest.fixture +def utils_linux(fresh_import): + return fresh_import("utils", "linux") + + +# --------------------------------------------------------------------------- # +# Import safety (regressions) +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize("module", ["utils", "tool_manager_windows"]) +@pytest.mark.parametrize("plat", ["linux", "darwin"]) +def test_import_off_windows_is_silent(fresh_import, capsys, module, plat): + mod = fresh_import(module, plat) + out = capsys.readouterr() + assert out.out == "" and out.err == "" + assert mod.MSYS2_PATH is None + + +def test_msys2_helpers_return_none_off_windows(utils_linux): + assert utils_linux.get_msys2_bash() is None + assert utils_linux.get_msys2_mingw_bin() is None + + +def test_windows_backend_dispatch_table_loads_off_windows(fresh_import): + tmw = fresh_import("tool_manager_windows", "linux") + assert set(tmw.TOOLS) >= {"kicad", "ngspice", "llvm", "ghdl", "verilator", + "esim", "chocolatey"} + + +# --------------------------------------------------------------------------- # +# MSYS2 helpers on Windows (preserved behaviour) +# --------------------------------------------------------------------------- # +class TestMsys2OnWindows: + @pytest.fixture + def utils_win(self, fresh_import, monkeypatch, tmp_path): + monkeypatch.setenv("MSYS2_PATH", str(tmp_path)) + return fresh_import("utils", "win32") + + def test_msys2_path_comes_from_platform_utils(self, utils_win, tmp_path): + assert str(utils_win.MSYS2_PATH) == str(tmp_path) + + def test_bash_found(self, utils_win, tmp_path): + bash = tmp_path / "usr" / "bin" / "bash.exe" + bash.parent.mkdir(parents=True) + bash.write_text("") + assert utils_win.get_msys2_bash() == bash + + def test_bash_missing(self, utils_win): + assert utils_win.get_msys2_bash() is None + + def test_mingw64_preferred_over_ucrt64(self, utils_win, tmp_path): + for sub in ("ucrt64", "mingw64"): + (tmp_path / sub / "bin").mkdir(parents=True) + assert utils_win.get_msys2_mingw_bin() == tmp_path / "mingw64" / "bin" + + def test_falls_back_to_ucrt64(self, utils_win, tmp_path): + (tmp_path / "ucrt64" / "bin").mkdir(parents=True) + assert utils_win.get_msys2_mingw_bin() == tmp_path / "ucrt64" / "bin" + + def test_no_mingw_dir(self, utils_win): + assert utils_win.get_msys2_mingw_bin() is None + + +# --------------------------------------------------------------------------- # +# Command helpers and status output +# --------------------------------------------------------------------------- # +def test_print_status_strips_newlines(utils_linux, capsys): + utils_linux.print_status("installed", "42\r\n", "latest") + assert capsys.readouterr().out == "installed|42|latest\n" + + +def test_print_status_keeps_falsy_installed(utils_linux, capsys): + utils_linux.print_status("not_installed", None, "latest") + assert capsys.readouterr().out == "not_installed|None|latest\n" + + +def test_run_cmd_safe_returns_result(utils_linux, monkeypatch): + seen = {} + + def fake_run(cmd, **kw): + seen.update(kw) + return subprocess.CompletedProcess(cmd, 0, stdout="ok", stderr="") + + monkeypatch.setattr(utils_linux.subprocess, "run", fake_run) + result = utils_linux.run_cmd_safe(["tool", "--version"], timeout=5) + assert result.stdout == "ok" + assert seen["timeout"] == 5 and seen["shell"] is False + + +@pytest.mark.parametrize("exc", [FileNotFoundError("tool"), + subprocess.TimeoutExpired("tool", 5)]) +def test_run_cmd_safe_returns_none_on_error(utils_linux, monkeypatch, exc): + def fake_run(cmd, **kw): + raise exc + + monkeypatch.setattr(utils_linux.subprocess, "run", fake_run) + assert utils_linux.run_cmd_safe(["tool"]) is None + + +def test_run_cmd_stream_streams_and_collects_output(utils_linux, capsys): + rc, out = utils_linux.run_cmd_stream( + [sys.executable, "-c", "print('one'); print(); print('two')"], + timeout=60) + assert rc == 0 + assert out == "one\ntwo" + assert capsys.readouterr().out.splitlines() == ["one", "two"] + + +def test_run_cmd_stream_missing_command(utils_linux): + rc, out = utils_linux.run_cmd_stream(["definitely-not-a-real-command-xyz"]) + assert rc == -1 and out diff --git a/src/toolManager/tool_manager_windows.py b/src/toolManager/tool_manager_windows.py index a3de1be9a..1466f3b1c 100644 --- a/src/toolManager/tool_manager_windows.py +++ b/src/toolManager/tool_manager_windows.py @@ -20,10 +20,8 @@ if _local_path not in sys.path: sys.path.insert(0, _local_path) -try: - MSYS2_PATH = get_mysys2_path(); -except RuntimeError as e: - print(e) +# MSYS2 only exists on Windows; this backend is only run there. +MSYS2_PATH = get_mysys2_path() if IS_WINDOWS else None from utils import ( run_cmd_safe, run_cmd_stream, which, print_status, diff --git a/src/toolManager/utils.py b/src/toolManager/utils.py index c07858c8c..bc75c5e90 100644 --- a/src/toolManager/utils.py +++ b/src/toolManager/utils.py @@ -12,17 +12,15 @@ import shutil import threading from pathlib import Path -from platform_utils import get_mysys2_path, subprocess_flags +from platform_utils import IS_WINDOWS, get_mysys2_path, subprocess_flags # ==================== CONSTANTS & DEFAULTS ==================== # Default Windows installation paths DEFAULT_ESIM_DIR = Path(r"C:\FOSSEE\eSim") -try: - MSYS2_PATH = get_mysys2_path(); -except RuntimeError as e: - print(f"[ERROR]: {e}", flush=True) +# MSYS2 only exists on Windows; elsewhere the MSYS2 helpers below return None. +MSYS2_PATH = get_mysys2_path() if IS_WINDOWS else None WIN_KICAD_PATHS = [ (r"C:\Program Files\KiCad\9.0\bin\kicad.exe", "9"), @@ -139,6 +137,8 @@ def get_msys2_bash(): """ Returns the path to MSYS2 bash.exe if found, else None. """ + if MSYS2_PATH is None: + return None bash_path = MSYS2_PATH / "usr" / "bin" / "bash.exe" return bash_path if bash_path.exists() else None @@ -147,7 +147,10 @@ def get_msys2_mingw_bin(): Returns the path to the MSYS2 MinGW bin directory if found, else None. Checks mingw64/bin first then ucrt64/bin and mingw32/bin. """ + if MSYS2_PATH is None: + return None for subdir in ["mingw64", "ucrt64", "mingw32"]: bin_dir = MSYS2_PATH / subdir / "bin" if bin_dir.exists(): return bin_dir + return None From cbedd24e2e1e660ea1cf6f1449c39476395158c4 Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:29:05 +0530 Subject: [PATCH 05/11] Do not remove working ngspice/KiCad installs before replacing them install_ngspice() ran "choco uninstall ngspice" before installing, so a failed install left no ngspice at all. It now skips a plain install when the requested version is already present, runs the same version-specific "choco install --version X --force --allow-downgrade" command without uninstalling first, and uses "choco upgrade" for updates to latest. Not verified against real Chocolatey: that the install command switches an existing ngspice to another version in place, and what state a failed install leaves the existing package in. install_kicad() ran cleanup_kicad() (uninstall plus folder removal) even when the requested version was already installed; a plain install of that version is now a no-op. Version switches and updates keep the existing flow. Co-Authored-By: Claude Opus 5.5 --- .../test_tool_manager_windows_install.py | 238 ++++++++++++++++++ src/toolManager/tool_manager_windows.py | 51 +++- 2 files changed, 276 insertions(+), 13 deletions(-) create mode 100644 src/toolManager/tests/test_tool_manager_windows_install.py diff --git a/src/toolManager/tests/test_tool_manager_windows_install.py b/src/toolManager/tests/test_tool_manager_windows_install.py new file mode 100644 index 000000000..4dc8db4a3 --- /dev/null +++ b/src/toolManager/tests/test_tool_manager_windows_install.py @@ -0,0 +1,238 @@ +"""Unit tests for the ngspice and KiCad install logic in tool_manager_windows.py. + +Chocolatey, the installed-tool lookups and the KiCad cleanup are mocked, so +these tests run on any OS without Chocolatey, admin rights or EDA tools. The +module is imported with a non-Windows platform so it does not re-wrap +sys.stdout (which it does on real Windows). +""" +import pytest + + +@pytest.fixture +def tmw(fresh_import, monkeypatch): + mod = fresh_import("tool_manager_windows", "linux") + monkeypatch.setattr(mod.time, "sleep", lambda s: None) + return mod + + +class FakeChoco: + """Records every command the installer would run.""" + + def __init__(self, mod, monkeypatch, rc=0, available=True): + self.rc = rc + self.commands = [] + monkeypatch.setattr(mod, "find_chocolatey", + lambda v=None: ("choco", "2.2.2") if available else (None, None)) + monkeypatch.setattr(mod, "run_cmd_stream", self._stream) + monkeypatch.setattr(mod, "run_cmd_safe", self._safe) + + def _stream(self, cmd, timeout=900, **kw): + self.commands.append(list(cmd)) + return self.rc, "" if self.rc == 0 else "Chocolatey installed 0/1 packages" + + def _safe(self, cmd, timeout=30, **kw): + self.commands.append(list(cmd)) + return None + + def ran(self, word): + """Commands whose choco sub-command is *word* (install/upgrade/uninstall).""" + return [c for c in self.commands if len(c) > 1 and c[1] == word] + + +def last_status(out): + return [l for l in out.splitlines() if l.count("|") == 2][-1] + + +def version_arg(cmd): + return cmd[cmd.index("--version") + 1] + + +# --------------------------------------------------------------------------- # +# _version_satisfies +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize("installed,target,expected", [ + ("42", "42", True), + ("42.1", "42", True), + ("9.0.7", "9", True), + ("9.x", "9", True), + ("41", "42", False), + ("4", "42", False), + ("42", "4", False), # "42" must not satisfy version "4" + ("unknown", "42", False), + (None, "42", False), + ("unknown", "latest", True), +]) +def test_version_satisfies(tmw, installed, target, expected): + assert tmw._version_satisfies(installed, target) is expected + + +# --------------------------------------------------------------------------- # +# install_ngspice +# --------------------------------------------------------------------------- # +class TestInstallNgspice: + def _installed(self, tmw, monkeypatch, *states): + """find_ngspice_safe returns each state in turn (last one repeats).""" + states = list(states) + + def fake_find(version=None): + return states.pop(0) if len(states) > 1 else states[0] + + monkeypatch.setattr(tmw, "find_ngspice_safe", fake_find) + + def test_fresh_install_succeeds(self, tmw, monkeypatch, capsys): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, (None, None), ("C:/ngspice.exe", "42")) + tmw.install_ngspice("42") + assert choco.ran("install") == [[ + "choco", "install", "ngspice", "--version", "42", "-y", + "--no-progress", "--allow-downgrade", "--force"]] + assert last_status(capsys.readouterr().out) == "installed|42|42" + + def test_never_uninstalls_before_installing(self, tmw, monkeypatch): + """Regression: install_ngspice used to run 'choco uninstall ngspice' + first, so a failed install left the user with no ngspice at all.""" + choco = FakeChoco(tmw, monkeypatch, rc=1) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "41")) + tmw.install_ngspice("42") + assert choco.ran("uninstall") == [] + assert len(choco.ran("install")) == 1 + + def test_failed_install_reports_failure(self, tmw, monkeypatch, capsys): + FakeChoco(tmw, monkeypatch, rc=1) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "41")) + tmw.install_ngspice("42") + state, detail, target = last_status(capsys.readouterr().out).split("|") + assert (state, target) == ("install_failed", "42") + assert detail # carries a hint from choco's output + + def test_already_installed_version_is_not_reinstalled(self, tmw, monkeypatch, + capsys): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "42")) + tmw.install_ngspice("42") + assert choco.commands == [] + assert last_status(capsys.readouterr().out) == "installed|42|42" + + def test_latest_already_installed_is_not_reinstalled(self, tmw, monkeypatch): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "unknown")) + tmw.install_ngspice("latest") + assert choco.commands == [] + + def test_unknown_installed_version_still_installs_requested(self, tmw, + monkeypatch): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "unknown")) + tmw.install_ngspice("40") + assert [version_arg(c) for c in choco.ran("install")] == ["40"] + + def test_switching_version_installs_over_existing(self, tmw, monkeypatch, + capsys): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "42"), + ("C:/ngspice.exe", "38")) + tmw.install_ngspice("38") + assert choco.ran("uninstall") == [] + cmd = choco.ran("install")[0] + assert version_arg(cmd) == "38" and "--allow-downgrade" in cmd + assert last_status(capsys.readouterr().out) == "installed|38|38" + + def test_update_to_latest_uses_choco_upgrade(self, tmw, monkeypatch): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "41")) + tmw.install_ngspice("latest", upgrade=True) + assert choco.ran("uninstall") == [] + assert choco.ran("upgrade") == [ + ["choco", "upgrade", "ngspice", "-y", "--no-progress"]] + + def test_update_reinstalls_even_when_version_matches(self, tmw, monkeypatch): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "42")) + tmw.install_ngspice("42", upgrade=True) + assert len(choco.ran("install")) == 1 + + def test_installed_but_not_found_afterwards(self, tmw, monkeypatch, capsys): + FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, (None, None)) + tmw.install_ngspice("42") + assert last_status(capsys.readouterr().out) == "install_failed|not_found|42" + + def test_chocolatey_missing(self, tmw, monkeypatch, capsys): + choco = FakeChoco(tmw, monkeypatch, available=False) + self._installed(tmw, monkeypatch, (None, None)) + tmw.install_ngspice("42") + assert choco.commands == [] + assert last_status(capsys.readouterr().out) == "install_failed|choco_missing|42" + + +# --------------------------------------------------------------------------- # +# install_kicad: cleanup_kicad() removes KiCad, so it must not run needlessly +# --------------------------------------------------------------------------- # +class TestInstallKicad: + @pytest.fixture + def cleanups(self, tmw, monkeypatch): + calls = [] + monkeypatch.setattr(tmw, "cleanup_kicad", lambda: calls.append(True)) + return calls + + def _installed(self, tmw, monkeypatch, *states): + states = list(states) + + def fake_find(version=None): + return states.pop(0) if len(states) > 1 else states[0] + + monkeypatch.setattr(tmw, "find_kicad_fixed", fake_find) + + @pytest.mark.parametrize("current,target", [("9.0.7", "9"), ("9.x", "9"), + ("8.0.9", "latest")]) + def test_already_installed_is_not_removed(self, tmw, monkeypatch, capsys, + cleanups, current, target): + """Regression: installing the version already present ran + cleanup_kicad() (uninstall + delete folders) and reinstalled.""" + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", current)) + tmw.install_kicad(target) + assert cleanups == [] + assert choco.commands == [] + assert last_status(capsys.readouterr().out) == f"installed|{current}|{target}" + + def test_kicad6_already_installed_skips_direct_download(self, tmw, + monkeypatch, cleanups): + FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", "6.0.11")) + monkeypatch.setattr(tmw, "install_kicad_direct", pytest.fail) + tmw.install_kicad("6") + assert cleanups == [] + + def test_different_version_still_installs(self, tmw, monkeypatch, capsys, + cleanups): + """Switching major version keeps the existing cleanup + install flow.""" + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", "8.0.9"), + ("C:/KiCad/bin/kicad.exe", "9.0.7")) + tmw.install_kicad("9") + assert cleanups == [True] + assert choco.ran("install")[0][:5] == ["choco", "install", "kicad", + "--version", "9.0.7"] + assert last_status(capsys.readouterr().out) == "installed|9.0.7|9" + + def test_update_still_runs_full_install(self, tmw, monkeypatch, cleanups): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", "9.0.7")) + tmw.install_kicad("latest", upgrade=True) + assert cleanups == [True] + assert len(choco.ran("install")) == 1 + + def test_not_installed_installs(self, tmw, monkeypatch, cleanups): + choco = FakeChoco(tmw, monkeypatch) + self._installed(tmw, monkeypatch, (None, None), + ("C:/KiCad/bin/kicad.exe", "9.0.7")) + tmw.install_kicad("latest") + assert len(choco.ran("install")) == 1 + + def test_chocolatey_missing(self, tmw, monkeypatch, capsys, cleanups): + FakeChoco(tmw, monkeypatch, available=False) + self._installed(tmw, monkeypatch, (None, None)) + tmw.install_kicad("9") + assert cleanups == [] + assert last_status(capsys.readouterr().out) == "install_failed|choco_missing|9" diff --git a/src/toolManager/tool_manager_windows.py b/src/toolManager/tool_manager_windows.py index 1466f3b1c..07a0bce5b 100644 --- a/src/toolManager/tool_manager_windows.py +++ b/src/toolManager/tool_manager_windows.py @@ -462,6 +462,15 @@ def check_kicad(target_version): print_status("not_installed", "none", target_version) def install_kicad(target_version, upgrade=False): + # The install paths below start with cleanup_kicad(), which removes the + # existing KiCad. Never do that just to reinstall the version already there. + if not upgrade: + exe, current = find_kicad_fixed(None) + if exe and _version_satisfies(current, target_version): + print(f"[OK] KiCad {current} is already installed") + print_status("installed", current, target_version) + return + choco_exe, _ = find_chocolatey(None) if not choco_exe: print_status("install_failed", "choco_missing", target_version) @@ -717,30 +726,46 @@ def check_ngspice(target_version): else: print_status("not_installed", "none", target_version) +def _version_satisfies(installed, target_version): + """True only when an installed version is known to match the request.""" + if target_version == "latest": + return True + if not installed or installed == "unknown": + return False + return installed == target_version or installed.startswith(f"{target_version}.") + + def install_ngspice(target_version, upgrade=False): + if not upgrade: + exe, current = find_ngspice_safe(None) + if exe and _version_satisfies(current, target_version): + print(f"[OK] ngspice {current or 'unknown'} is already installed") + print_status("installed", current or "unknown", target_version) + return + choco_exe, _ = find_chocolatey(None) if not choco_exe: print_status("install_failed", "choco_missing", target_version) return - + exact_version = NGSPICE_VERSIONS.get(target_version, target_version) - - print("Uninstalling existing ngspice...") - uninstall_cmd = ["choco", "uninstall", "ngspice", "-y", "--force-dependencies", "--no-progress"] - run_cmd_safe(uninstall_cmd, timeout=180) - - print(f"[1/3] Removing existing ngspice...") - print(f"[2/3] Installing ngspice {target_version} via Chocolatey...") - + + # Install over the existing copy instead of uninstalling it first, so a + # failed install no longer starts by removing a working ngspice. The + # version-specific command is unchanged (--force / --allow-downgrade). + # Not yet verified against real Chocolatey: that it switches versions in + # place, and what it leaves behind if the install fails part-way. + print(f"[1/2] Installing ngspice {target_version} via Chocolatey...") + if target_version == "latest": - cmd = ["choco", "install", "ngspice", "-y", "--no-progress"] + cmd = ["choco", "upgrade" if upgrade else "install", "ngspice", "-y", "--no-progress"] else: cmd = ["choco", "install", "ngspice", "--version", exact_version, "-y", "--no-progress", "--allow-downgrade", "--force"] - + rc, out = run_cmd_stream(cmd, timeout=300) - + if rc == 0: - print("[3/3] Verifying installation...") + print("[2/2] Verifying installation...") time.sleep(2) exe, installed_version = find_ngspice_safe(None) if exe: From 3b6c2b8fdfc469cb824afb3f32c8a44c2c5655db Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:20:01 +0530 Subject: [PATCH 06/11] Run Tool Manager CI when the tested backend modules change Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test_platform_utils.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/test_platform_utils.yml b/.github/workflows/test_platform_utils.yml index b68ef75ad..28a4204f1 100644 --- a/.github/workflows/test_platform_utils.yml +++ b/.github/workflows/test_platform_utils.yml @@ -5,11 +5,17 @@ on: push: paths: - 'src/toolManager/platform_utils.py' + - 'src/toolManager/utils.py' + - 'src/toolManager/tool_manager_linux.py' + - 'src/toolManager/tool_manager_windows.py' - 'src/toolManager/tests/**' - '.github/workflows/test_platform_utils.yml' pull_request: paths: - 'src/toolManager/platform_utils.py' + - 'src/toolManager/utils.py' + - 'src/toolManager/tool_manager_linux.py' + - 'src/toolManager/tool_manager_windows.py' - 'src/toolManager/tests/**' - '.github/workflows/test_platform_utils.yml' From 701ad95ff7b1bcd4edd81f91b74cf57cea935634 Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:35:28 +0530 Subject: [PATCH 07/11] Only skip ngspice/KiCad installs when the executable exists The "already installed" checks added in the previous commits trusted the lookup helpers too much: - find_ngspice_safe() reports a version whenever "choco list" prints a matching line, even if no ngspice.exe exists. On Chocolatey 1.x that command lists the online feed, so a plain install was skipped on machines without ngspice. - find_kicad_fixed() returns a registry InstallLocation even when the folder is gone, so a stale Uninstall entry skipped the install. install_ngspice() now also requires _find_ngspice_exe(), and install_kicad() requires the returned path to be an existing file. Cleanup, version switching and updates are unchanged. The new regression tests run the real lookup functions and fake only Chocolatey's output, the registry and the install folders. They fail without this fix. Co-Authored-By: Claude Opus 5.5 --- .../test_tool_manager_windows_install.py | 268 +++++++++++++++--- src/toolManager/tool_manager_windows.py | 12 +- 2 files changed, 237 insertions(+), 43 deletions(-) diff --git a/src/toolManager/tests/test_tool_manager_windows_install.py b/src/toolManager/tests/test_tool_manager_windows_install.py index 4dc8db4a3..44973d217 100644 --- a/src/toolManager/tests/test_tool_manager_windows_install.py +++ b/src/toolManager/tests/test_tool_manager_windows_install.py @@ -1,10 +1,17 @@ """Unit tests for the ngspice and KiCad install logic in tool_manager_windows.py. -Chocolatey, the installed-tool lookups and the KiCad cleanup are mocked, so -these tests run on any OS without Chocolatey, admin rights or EDA tools. The -module is imported with a non-Windows platform so it does not re-wrap -sys.stdout (which it does on real Windows). +Chocolatey and the KiCad cleanup are always mocked, so these tests run on any +OS without Chocolatey, admin rights or EDA tools. Most tests also replace the +installed-tool lookups to drive the install flow; the "already installed" +decisions are tested at the end against the REAL find_ngspice_safe() and +find_kicad_fixed(), with only Chocolatey's output, the registry and the file +system faked. The module is imported with a non-Windows platform so it does +not re-wrap sys.stdout (which it does on real Windows). """ +import subprocess +import sys +import types + import pytest @@ -71,13 +78,16 @@ def test_version_satisfies(tmw, installed, target, expected): # --------------------------------------------------------------------------- # class TestInstallNgspice: def _installed(self, tmw, monkeypatch, *states): - """find_ngspice_safe returns each state in turn (last one repeats).""" + """find_ngspice_safe returns each state in turn (last one repeats); + the executable from the first state is what is on disk beforehand.""" states = list(states) + exe_before = states[0][0] def fake_find(version=None): return states.pop(0) if len(states) > 1 else states[0] monkeypatch.setattr(tmw, "find_ngspice_safe", fake_find) + monkeypatch.setattr(tmw, "_find_ngspice_exe", lambda: exe_before) def test_fresh_install_succeeds(self, tmw, monkeypatch, capsys): choco = FakeChoco(tmw, monkeypatch) @@ -105,20 +115,6 @@ def test_failed_install_reports_failure(self, tmw, monkeypatch, capsys): assert (state, target) == ("install_failed", "42") assert detail # carries a hint from choco's output - def test_already_installed_version_is_not_reinstalled(self, tmw, monkeypatch, - capsys): - choco = FakeChoco(tmw, monkeypatch) - self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "42")) - tmw.install_ngspice("42") - assert choco.commands == [] - assert last_status(capsys.readouterr().out) == "installed|42|42" - - def test_latest_already_installed_is_not_reinstalled(self, tmw, monkeypatch): - choco = FakeChoco(tmw, monkeypatch) - self._installed(tmw, monkeypatch, ("C:/ngspice.exe", "unknown")) - tmw.install_ngspice("latest") - assert choco.commands == [] - def test_unknown_installed_version_still_installs_requested(self, tmw, monkeypatch): choco = FakeChoco(tmw, monkeypatch) @@ -183,27 +179,6 @@ def fake_find(version=None): monkeypatch.setattr(tmw, "find_kicad_fixed", fake_find) - @pytest.mark.parametrize("current,target", [("9.0.7", "9"), ("9.x", "9"), - ("8.0.9", "latest")]) - def test_already_installed_is_not_removed(self, tmw, monkeypatch, capsys, - cleanups, current, target): - """Regression: installing the version already present ran - cleanup_kicad() (uninstall + delete folders) and reinstalled.""" - choco = FakeChoco(tmw, monkeypatch) - self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", current)) - tmw.install_kicad(target) - assert cleanups == [] - assert choco.commands == [] - assert last_status(capsys.readouterr().out) == f"installed|{current}|{target}" - - def test_kicad6_already_installed_skips_direct_download(self, tmw, - monkeypatch, cleanups): - FakeChoco(tmw, monkeypatch) - self._installed(tmw, monkeypatch, ("C:/KiCad/bin/kicad.exe", "6.0.11")) - monkeypatch.setattr(tmw, "install_kicad_direct", pytest.fail) - tmw.install_kicad("6") - assert cleanups == [] - def test_different_version_still_installs(self, tmw, monkeypatch, capsys, cleanups): """Switching major version keeps the existing cleanup + install flow.""" @@ -236,3 +211,216 @@ def test_chocolatey_missing(self, tmw, monkeypatch, capsys, cleanups): tmw.install_kicad("9") assert cleanups == [] assert last_status(capsys.readouterr().out) == "install_failed|choco_missing|9" + + +# --------------------------------------------------------------------------- # +# "Already installed?" decisions, using the REAL lookup functions +# --------------------------------------------------------------------------- # +CHOCO_V1_FEED = ("Chocolatey v1.4.0\n" + "ngspice 44.2 [Approved] Downloads cached for licensed users\n" + "1 packages found.\n") + + +def choco_v2_local(version): + return f"Chocolatey v2.2.2\nngspice {version}\n1 packages installed.\n" + + +class _FakeRegKey: + def __init__(self, entry=None): + self.entry = entry + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + +class LookupHost: + """A fake Windows machine for find_ngspice_safe() / find_kicad_fixed(). + + Only the outside world is faked: what is on PATH, the output of + "choco list", the default install folders (redirected into tmp_path) and + the Uninstall registry key. The lookup functions themselves run unchanged. + Installs are recorded, never executed. + """ + + def __init__(self, tmw, monkeypatch, tmp_path): + self.tmp = tmp_path + self.choco_list = None # None: choco is not on PATH + self.registry = [] # Uninstall entries (dicts) + self.installs = [] + self.cleanups = [] + + self.ngspice_exe = tmp_path / "ngspice" / "bin" / "ngspice.exe" + monkeypatch.setattr(tmw, "WIN_NGSPICE_PATHS", [str(self.ngspice_exe)]) + monkeypatch.setattr(tmw, "WIN_KICAD_PATHS", [ + (str(self.kicad_exe(v)), v) for v in ("9", "8", "7", "6")]) + monkeypatch.setattr(tmw, "which", self._which) + monkeypatch.setattr(tmw, "run_cmd_safe", self._run_cmd_safe) + monkeypatch.setattr(tmw, "run_cmd_stream", self._run_cmd_stream) + monkeypatch.setattr(tmw, "find_chocolatey", lambda v=None: ("choco", "2.2.2")) + monkeypatch.setattr(tmw, "cleanup_kicad", lambda: self.cleanups.append(True)) + monkeypatch.setattr(tmw, "install_kicad_direct", pytest.fail) + monkeypatch.setitem(sys.modules, "winreg", self._winreg()) + + # -- what exists on the fake machine -------------------------------- + def kicad_exe(self, major): + return self.tmp / "Program Files" / "KiCad" / f"{major}.0" / "bin" / "kicad.exe" + + @staticmethod + def create(path, text=""): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return path + + # -- faked externals ------------------------------------------------ + def _which(self, name): + if name in ("choco", "choco.exe") and self.choco_list is not None: + return r"C:\ProgramData\chocolatey\bin\choco.exe" + return None # no ngspice / kicad on PATH + + def _run_cmd_safe(self, cmd, timeout=30, **kw): + if cmd[1:2] == ["list"]: + return subprocess.CompletedProcess(cmd, 0, self.choco_list or "", "") + return None + + def _run_cmd_stream(self, cmd, timeout=900, **kw): + self.installs.append(list(cmd)) + return 0, "" + + def _winreg(self): + host = self + + def open_key(parent, sub=None, *args): + if isinstance(parent, _FakeRegKey): # an Uninstall subkey + return _FakeRegKey(host.registry[int(sub)]) + return _FakeRegKey() # the Uninstall key + + def query_value(key, name): + if name not in key.entry: + raise FileNotFoundError(name) + return key.entry[name], 1 + + return types.SimpleNamespace( + HKEY_LOCAL_MACHINE=1, HKEY_CURRENT_USER=2, + KEY_READ=0, KEY_WOW64_64KEY=0, KEY_WOW64_32KEY=0, + OpenKey=open_key, + QueryInfoKey=lambda key: (len(host.registry), 0, 0), + EnumKey=lambda key, i: str(i), + QueryValueEx=query_value) + + +@pytest.fixture +def host(tmw, monkeypatch, tmp_path): + return LookupHost(tmw, monkeypatch, tmp_path) + + +class TestNgspiceDetection: + @pytest.mark.parametrize("target", ["latest", "44"]) + def test_feed_listing_without_executable_still_installs(self, tmw, host, + target): + """Regression (independent review): on Chocolatey 1.x "choco list" + shows the online feed, so find_ngspice_safe() reports ngspice 44.2 + although nothing is installed. The install used to be skipped.""" + host.choco_list = CHOCO_V1_FEED + assert tmw.find_ngspice_safe(None) == ("ngspice", "44.2") # the trigger + + tmw.install_ngspice(target) + + assert len(host.installs) == 1 + assert host.installs[0][:3] == ["choco", "install", "ngspice"] + + def test_local_listing_without_executable_still_installs(self, tmw, host): + """Package registered with Chocolatey but its files are gone.""" + host.choco_list = choco_v2_local("42") + tmw.install_ngspice("42") + assert len(host.installs) == 1 + + def test_installed_executable_with_matching_version_is_skipped( + self, tmw, host, capsys): + host.create(host.ngspice_exe) + host.choco_list = choco_v2_local("42") + tmw.install_ngspice("42") + assert host.installs == [] + assert last_status(capsys.readouterr().out) == "installed|42|42" + + def test_installed_executable_with_other_version_installs(self, tmw, host): + host.create(host.ngspice_exe) + host.choco_list = choco_v2_local("41") + tmw.install_ngspice("42") + assert [version_arg(c) for c in host.installs] == ["42"] + + def test_executable_with_unknown_version(self, tmw, host): + """Without Chocolatey the version is "unknown": good enough for + "latest", never for a specific version.""" + host.create(host.ngspice_exe) + tmw.install_ngspice("latest") + assert host.installs == [] + tmw.install_ngspice("42") + assert [version_arg(c) for c in host.installs] == ["42"] + + def test_nothing_installed_installs(self, tmw, host): + tmw.install_ngspice("latest") + assert host.installs == [["choco", "install", "ngspice", "-y", "--no-progress"]] + + +class TestKicadDetection: + def stale_entry(self, host, version="8.0.9"): + return {"DisplayName": f"KiCad {version}", "DisplayVersion": version, + "InstallLocation": str(host.tmp / "Program Files" / "KiCad" / "removed")} + + @pytest.mark.parametrize("target", ["latest", "8"]) + def test_stale_registry_entry_still_installs(self, tmw, host, target): + """Regression (independent review): an Uninstall registry entry whose + folder was deleted made find_kicad_fixed() return that folder, and the + install was skipped as "already installed".""" + host.registry.append(self.stale_entry(host)) + assert tmw.find_kicad_fixed(None) == ( + host.registry[0]["InstallLocation"], "8.0.9") # the trigger + + tmw.install_kicad(target) + + assert host.cleanups == [True] + assert host.installs and host.installs[0][:3] == ["choco", "install", "kicad"] + + def test_registry_entry_with_executable_is_skipped(self, tmw, host, capsys): + location = host.tmp / "Apps" / "KiCad" + host.create(location / "bin" / "kicad.exe") + host.registry.append({"DisplayName": "KiCad 8.0.9", "DisplayVersion": "8.0.9", + "InstallLocation": str(location)}) + tmw.install_kicad("8") + assert host.cleanups == [] and host.installs == [] + assert last_status(capsys.readouterr().out) == "installed|8.0.9|8" + + def test_default_install_with_version_file_is_skipped(self, tmw, host, capsys): + exe = host.create(host.kicad_exe("9")) + host.create(exe.parent.parent / "version.txt", "KiCad 9.0.7\n") + tmw.install_kicad("9") + assert host.cleanups == [] and host.installs == [] + assert last_status(capsys.readouterr().out) == "installed|9.0.7|9" + + def test_default_install_without_version_file_is_skipped(self, tmw, host, + capsys): + host.create(host.kicad_exe("9")) + tmw.install_kicad("latest") + assert host.cleanups == [] and host.installs == [] + assert last_status(capsys.readouterr().out) == "installed|9.x|latest" + + def test_kicad6_installed_skips_cleanup_and_direct_download(self, tmw, host): + host.create(host.kicad_exe("6")) + tmw.install_kicad("6") # install_kicad_direct would fail the test + assert host.cleanups == [] + + def test_other_version_installed_keeps_version_switch(self, tmw, host): + """Switching versions keeps the existing cleanup + install flow.""" + host.create(host.kicad_exe("9")) + tmw.install_kicad("8") + assert host.cleanups == [True] + assert version_arg(host.installs[0]) == "8.0.9" + + def test_nothing_installed_installs(self, tmw, host): + assert tmw.find_kicad_fixed(None) == (None, None) + tmw.install_kicad("latest") + assert host.cleanups == [True] + assert host.installs[0][:3] == ["choco", "install", "kicad"] diff --git a/src/toolManager/tool_manager_windows.py b/src/toolManager/tool_manager_windows.py index 07a0bce5b..4c5e8bd0e 100644 --- a/src/toolManager/tool_manager_windows.py +++ b/src/toolManager/tool_manager_windows.py @@ -464,9 +464,11 @@ def check_kicad(target_version): def install_kicad(target_version, upgrade=False): # The install paths below start with cleanup_kicad(), which removes the # existing KiCad. Never do that just to reinstall the version already there. + # find_kicad_fixed() can return a registry InstallLocation whose folder no + # longer exists, so only skip when an actual kicad.exe file is present. if not upgrade: exe, current = find_kicad_fixed(None) - if exe and _version_satisfies(current, target_version): + if exe and os.path.isfile(exe) and _version_satisfies(current, target_version): print(f"[OK] KiCad {current} is already installed") print_status("installed", current, target_version) return @@ -737,8 +739,12 @@ def _version_satisfies(installed, target_version): def install_ngspice(target_version, upgrade=False): if not upgrade: - exe, current = find_ngspice_safe(None) - if exe and _version_satisfies(current, target_version): + # find_ngspice_safe() also reports a version when "choco list" prints + # a matching line, even if no ngspice.exe exists (on Chocolatey 1.x + # that command lists the online feed). Only skip when the executable + # itself is present. + _, current = find_ngspice_safe(None) + if _find_ngspice_exe() and _version_satisfies(current, target_version): print(f"[OK] ngspice {current or 'unknown'} is already installed") print_status("installed", current or "unknown", target_version) return From c78ae44ed10fdfa25fdb811cdb277eac9024a242 Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:35:28 +0530 Subject: [PATCH 08/11] Test Linux package commands against explicit expected values The command tests compared against the module's own PACKAGE_MAP and INSTALL_CMD, so a wrong package name or flag would still pass. They now use hand-written expected commands for representative managers and tools, plus a breadth check over every supported combination. The apt-lock test only checked that _apt_lock was called. It now uses the real _apt_lock with a fake fcntl and checks the command runs after the lock is taken and before it is released, including on failure. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_tool_manager_linux.py | 112 ++++++++++++++---- 1 file changed, 86 insertions(+), 26 deletions(-) diff --git a/src/toolManager/tests/test_tool_manager_linux.py b/src/toolManager/tests/test_tool_manager_linux.py index f9e891ae3..0d2ff0ab6 100644 --- a/src/toolManager/tests/test_tool_manager_linux.py +++ b/src/toolManager/tests/test_tool_manager_linux.py @@ -24,22 +24,24 @@ def tml(fresh_import, monkeypatch): class FakeSystem: """Stands in for the package manager, elevation and installed-tool lookup.""" - def __init__(self, mod, monkeypatch, pm="apt", rc=0): + def __init__(self, mod, monkeypatch, pm="apt", rc=0, fake_lock=True): self.pm = pm self.rc = rc # exit code of the elevated command self.installed = {} # tool -> location self.commands = [] # every elevated command, in order self.streamed = [] # every non-elevated command (_stream) - self.locks = [] # pm passed to _apt_lock + self.events = [] # "run" and lock events, in order monkeypatch.setattr(mod, "detect_package_manager", lambda: self.pm) monkeypatch.setattr(mod, "_run_elevated", self._run_elevated) monkeypatch.setattr(mod, "_stream", self._stream) monkeypatch.setattr(mod, "_find_installed", self.installed.get) - monkeypatch.setattr(mod, "_apt_lock", self._apt_lock) + if fake_lock: + monkeypatch.setattr(mod, "_apt_lock", self._apt_lock) def _run_elevated(self, args): self.commands.append(list(args)) + self.events.append("run") return self.rc def _stream(self, args): @@ -48,7 +50,6 @@ def _stream(self, args): @contextlib.contextmanager def _apt_lock(self, pm="apt"): - self.locks.append(pm) yield @@ -60,36 +61,69 @@ def status_lines(out): # --------------------------------------------------------------------------- # # Package selection and command construction # --------------------------------------------------------------------------- # +# Expected commands are written out by hand (not read from PACKAGE_MAP / +# INSTALL_CMD), so a wrong package name or flag in the module is caught. +INSTALL_CASES = [ + ("apt", "kicad", ["apt-get", "install", "-y", "kicad", "kicad-footprints", + "kicad-libraries", "kicad-symbols", "kicad-templates"]), + ("apt", "llvm", ["apt-get", "install", "-y", "clang", "llvm"]), + ("dnf", "llvm", ["dnf", "install", "-y", "clang", "llvm-devel"]), + ("yum", "ngspice", ["yum", "install", "-y", "ngspice"]), + ("pacman", "kicad", ["pacman", "-S", "--noconfirm", "kicad", + "kicad-library", "kicad-library-3d"]), + ("zypper", "verilator", ["zypper", "install", "--non-interactive", + "verilator"]), +] + +UNINSTALL_CASES = [ + ("apt", "kicad", ["apt-get", "remove", "-y", "--autoremove", "kicad", + "kicad-footprints", "kicad-libraries", "kicad-symbols", + "kicad-templates"]), + ("dnf", "llvm", ["dnf", "remove", "-y", "clang", "llvm-devel"]), + ("yum", "kicad", ["yum", "remove", "-y", "kicad"]), + ("pacman", "ghdl", ["pacman", "-Rs", "--noconfirm", "ghdl"]), + ("zypper", "ngspice", ["zypper", "remove", "--non-interactive", + "--clean-deps", "ngspice"]), +] + +PM_BINARY = {"apt": "apt-get", "dnf": "dnf", "yum": "yum", + "pacman": "pacman", "zypper": "zypper"} SUPPORTED = [(pm, tool) for pm in ("apt", "dnf", "pacman", "zypper") for tool in ("kicad", "ngspice", "ghdl", "verilator", "llvm")] -@pytest.mark.parametrize("pm,tool", SUPPORTED) -def test_install_builds_command_from_package_map(tml, monkeypatch, capsys, - pm, tool): +@pytest.mark.parametrize("pm,tool,expected", INSTALL_CASES) +def test_install_command(tml, monkeypatch, capsys, pm, tool, expected): fs = FakeSystem(tml, monkeypatch, pm=pm) fs.installed[tool] = f"/usr/bin/{tool}" - rc = tml.install_tool(tool, "latest") - - assert rc == tml.EXIT_OK - assert fs.commands == [tml.INSTALL_CMD[pm] + tml.PACKAGE_MAP[pm][tool]] + assert tml.install_tool(tool, "latest") == tml.EXIT_OK + assert fs.commands == [expected] assert status_lines(capsys.readouterr().out) == [ f"installed|{tool}|/usr/bin/{tool}"] -@pytest.mark.parametrize("pm,tool", SUPPORTED) -def test_uninstall_builds_command_from_package_map(tml, monkeypatch, capsys, - pm, tool): +@pytest.mark.parametrize("pm,tool,expected", UNINSTALL_CASES) +def test_uninstall_command(tml, monkeypatch, capsys, pm, tool, expected): fs = FakeSystem(tml, monkeypatch, pm=pm) # tool absent after removal - rc = tml.uninstall_tool(tool, "none") - - assert rc == tml.EXIT_OK - assert fs.commands == [tml.UNINSTALL_CMD[pm] + tml.PACKAGE_MAP[pm][tool]] + assert tml.uninstall_tool(tool, "none") == tml.EXIT_OK + assert fs.commands == [expected] assert status_lines(capsys.readouterr().out) == [f"not_installed|{tool}|none"] +@pytest.mark.parametrize("pm,tool", SUPPORTED) +def test_every_supported_tool_installs_and_uninstalls(tml, monkeypatch, pm, tool): + """Breadth check: each tool these managers support runs exactly one + command with that manager and reports success.""" + fs = FakeSystem(tml, monkeypatch, pm=pm) + fs.installed[tool] = f"/usr/bin/{tool}" + assert tml.install_tool(tool, "latest") == tml.EXIT_OK + fs.installed.clear() + assert tml.uninstall_tool(tool, "none") == tml.EXIT_OK + assert [c[0] for c in fs.commands] == [PM_BINARY[pm]] * 2 + + def test_package_map_and_commands_cover_the_same_managers(tml): assert set(tml.PACKAGE_MAP) == set(tml.INSTALL_CMD) == set(tml.UNINSTALL_CMD) @@ -101,14 +135,40 @@ def test_update_uses_the_install_command(tml, monkeypatch): assert fs.commands == [["dnf", "install", "-y", "ngspice"]] -def test_package_manager_command_runs_inside_the_apt_lock(tml, monkeypatch): - fs = FakeSystem(tml, monkeypatch, pm="pacman") - fs.installed["ghdl"] = "/usr/bin/ghdl" - tml.install_tool("ghdl", "latest") - fs.installed.clear() - tml.uninstall_tool("ghdl", "none") - # _apt_lock itself decides to no-op for non-apt managers (tested below). - assert fs.locks == ["pacman", "pacman"] +class TestCommandsRunInsideAptLock: + """Uses the REAL _apt_lock with a fake fcntl, and checks the package + command runs after the lock is taken and before it is released.""" + + @pytest.fixture + def fs(self, tml, monkeypatch, tmp_path): + fs = FakeSystem(tml, monkeypatch, fake_lock=False) + fake_fcntl = types.SimpleNamespace( + LOCK_EX="lock", LOCK_UN="unlock", + flock=lambda f, op: fs.events.append(op)) + monkeypatch.setitem(sys.modules, "fcntl", fake_fcntl) + monkeypatch.setattr(tml, "_APT_LOCK_FILE", str(tmp_path / "apt.lock")) + return fs + + def test_apt_install(self, tml, fs): + fs.installed["ghdl"] = "/usr/bin/ghdl" + tml.install_tool("ghdl", "latest") + assert fs.events == ["lock", "run", "unlock"] + + def test_apt_uninstall(self, tml, fs): + tml.uninstall_tool("ghdl", "none") + assert fs.events == ["lock", "run", "unlock"] + + def test_lock_released_when_command_fails(self, tml, fs): + fs.rc = 100 + tml.install_tool("ghdl", "latest") + assert fs.events == ["lock", "run", "unlock"] + + @pytest.mark.parametrize("pm", ["dnf", "pacman", "zypper", "yum"]) + def test_other_managers_are_not_locked(self, tml, fs, pm): + fs.pm = pm + fs.installed["ngspice"] = "/usr/bin/ngspice" + tml.install_tool("ngspice", "latest") + assert fs.events == ["run"] # --------------------------------------------------------------------------- # From d15e4cbd1adf1fe11f5d3462aec5c7436d08e55f Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:36:50 +0530 Subject: [PATCH 09/11] Close the output pipe of streamed commands utils.run_cmd_stream() and tool_manager_linux._stream() read a child process's stdout but never closed the pipe. It was only released by the garbage collector, with a ResourceWarning, which made the test suite fail intermittently under "pytest -W error". Both now close the pipe once the output has been read. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_tool_manager_linux.py | 27 +++++++++++++++++++ src/toolManager/tests/test_utils.py | 15 +++++++++++ src/toolManager/tool_manager_linux.py | 9 ++++--- src/toolManager/utils.py | 1 + 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/toolManager/tests/test_tool_manager_linux.py b/src/toolManager/tests/test_tool_manager_linux.py index 0d2ff0ab6..f147f0277 100644 --- a/src/toolManager/tests/test_tool_manager_linux.py +++ b/src/toolManager/tests/test_tool_manager_linux.py @@ -436,6 +436,33 @@ def test_which_native_off_wsl_uses_shutil_which(self, tml, monkeypatch): assert tml._which_native("ghdl") == "/usr/bin/ghdl" +class TestStream: + """_stream runs a real (harmless) Python subprocess.""" + + def test_streams_output_and_returns_exit_code(self, tml, capsys): + rc = tml._stream([sys.executable, "-c", + "print('hello'); print(); raise SystemExit(4)"]) + assert rc == 4 + assert capsys.readouterr().out.splitlines() == ["hello"] + + def test_closes_its_output_pipe(self, tml, monkeypatch): + """Regression: the pipe was left open (ResourceWarning on cleanup).""" + procs = [] + real_popen = subprocess.Popen + + def recording_popen(*args, **kwargs): + procs.append(real_popen(*args, **kwargs)) + return procs[-1] + + monkeypatch.setattr(tml.subprocess, "Popen", recording_popen) + tml._stream([sys.executable, "-c", "print('x')"]) + assert procs[0].stdout.closed + + def test_missing_command(self, tml, capsys): + assert tml._stream(["definitely-not-a-real-command-xyz"]) == 1 + assert "[ERROR] Command not found" in capsys.readouterr().out + + class TestAptLock: def test_non_apt_managers_do_not_lock(self, tml, monkeypatch): # Would fail if it tried to import fcntl or open the lock file. diff --git a/src/toolManager/tests/test_utils.py b/src/toolManager/tests/test_utils.py index ba86303f1..6da01ac3d 100644 --- a/src/toolManager/tests/test_utils.py +++ b/src/toolManager/tests/test_utils.py @@ -122,3 +122,18 @@ def test_run_cmd_stream_streams_and_collects_output(utils_linux, capsys): def test_run_cmd_stream_missing_command(utils_linux): rc, out = utils_linux.run_cmd_stream(["definitely-not-a-real-command-xyz"]) assert rc == -1 and out + + +def test_run_cmd_stream_closes_its_output_pipe(utils_linux, monkeypatch): + """Regression: the pipe was left open, which surfaced as a + ResourceWarning (and a failure under -W error) when it was collected.""" + procs = [] + real_popen = subprocess.Popen + + def recording_popen(*args, **kwargs): + procs.append(real_popen(*args, **kwargs)) + return procs[-1] + + monkeypatch.setattr(utils_linux.subprocess, "Popen", recording_popen) + utils_linux.run_cmd_stream([sys.executable, "-c", "print('x')"], timeout=60) + assert procs[0].stdout.closed diff --git a/src/toolManager/tool_manager_linux.py b/src/toolManager/tool_manager_linux.py index c4d7cb840..8ab31187b 100644 --- a/src/toolManager/tool_manager_linux.py +++ b/src/toolManager/tool_manager_linux.py @@ -161,10 +161,11 @@ def _stream(args: list) -> int: encoding="utf-8", errors="ignore", ) - for line in proc.stdout: - line = line.rstrip() - if line: - print(line, flush=True) + with proc.stdout: + for line in proc.stdout: + line = line.rstrip() + if line: + print(line, flush=True) proc.wait() return proc.returncode except FileNotFoundError: diff --git a/src/toolManager/utils.py b/src/toolManager/utils.py index bc75c5e90..da4ba02c7 100644 --- a/src/toolManager/utils.py +++ b/src/toolManager/utils.py @@ -111,6 +111,7 @@ def _kill(): proc.wait() finally: timer.cancel() + proc.stdout.close() return proc.returncode, "\n".join(output_lines) From 8a69bef670dd589c8df0e761161fd5d20812148a Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:52:27 +0530 Subject: [PATCH 10/11] Report only verified Windows install results Detection: - find_ngspice_safe() now requires an ngspice executable and takes the version from Chocolatey's record of the LOCAL install, else "unknown". The new _choco_installed_version() uses "choco list --limit-output", adding --local-only on Chocolatey 1.x, whose plain "list" searches the online feed. An unclear Chocolatey version or failed query gives no version rather than a guess. - find_kicad_fixed() skips Uninstall registry entries whose kicad.exe no longer exists, so a stale entry can neither block a reinstall nor count as a successful install or a failed uninstall. Install results: - ngspice and LLVM: a known version that differs from the request is a failure, and an unreadable version is never replaced by the requested one. - LLVM no longer runs "choco uninstall llvm" before installing, skips a plain install of a version already present, and uses "choco upgrade" for updates to latest. - GHDL: a copied ghdl.exe that fails --version is a failure. - Verilator: files copied without a findable executable is a failure, and the MSYS2 update no longer removes the package before "pacman -S". - eSim: a non-zero installer exit code is a failure, even when an older eSim is still on disk. - get_msys2_env() put /../usr/bin on PATH instead of /usr/bin. Not verified against real Chocolatey: the 1.x/2.x "choco list" behaviour (taken from Chocolatey's documentation) and that the version-specific install commands switch versions in place. The eSim installer exit code check assumes it returns non-zero on failure, as NSIS installers do. Tests run the real lookup functions against a fake machine (Chocolatey 1.x/2.x with separate local packages and feed, registry, folders) and fail without these changes. Co-Authored-By: Claude Opus 5.5 --- .../test_tool_manager_windows_install.py | 417 +++++++++++++++--- src/toolManager/tool_manager_windows.py | 193 ++++---- 2 files changed, 466 insertions(+), 144 deletions(-) diff --git a/src/toolManager/tests/test_tool_manager_windows_install.py b/src/toolManager/tests/test_tool_manager_windows_install.py index 44973d217..16fc66710 100644 --- a/src/toolManager/tests/test_tool_manager_windows_install.py +++ b/src/toolManager/tests/test_tool_manager_windows_install.py @@ -1,16 +1,17 @@ -"""Unit tests for the ngspice and KiCad install logic in tool_manager_windows.py. - -Chocolatey and the KiCad cleanup are always mocked, so these tests run on any -OS without Chocolatey, admin rights or EDA tools. Most tests also replace the -installed-tool lookups to drive the install flow; the "already installed" -decisions are tested at the end against the REAL find_ngspice_safe() and -find_kicad_fixed(), with only Chocolatey's output, the registry and the file -system faked. The module is imported with a non-Windows platform so it does -not re-wrap sys.stdout (which it does on real Windows). +"""Unit tests for the install logic in tool_manager_windows.py. + +Chocolatey, downloads, installers and the KiCad cleanup are always mocked, so +these tests run on any OS without Chocolatey, admin rights or EDA tools. The +first tests replace the installed-tool lookups to drive the install flow; the +later ones (LookupHost) run the REAL find_ngspice_safe(), find_kicad_fixed() +and find_llvm_fixed() with only Chocolatey, the registry and the file system +faked. The module is imported with a non-Windows platform so it does not +re-wrap sys.stdout (which it does on real Windows). """ import subprocess import sys import types +import zipfile import pytest @@ -78,16 +79,13 @@ def test_version_satisfies(tmw, installed, target, expected): # --------------------------------------------------------------------------- # class TestInstallNgspice: def _installed(self, tmw, monkeypatch, *states): - """find_ngspice_safe returns each state in turn (last one repeats); - the executable from the first state is what is on disk beforehand.""" + """find_ngspice_safe returns each state in turn (last one repeats).""" states = list(states) - exe_before = states[0][0] def fake_find(version=None): return states.pop(0) if len(states) > 1 else states[0] monkeypatch.setattr(tmw, "find_ngspice_safe", fake_find) - monkeypatch.setattr(tmw, "_find_ngspice_exe", lambda: exe_before) def test_fresh_install_succeeds(self, tmw, monkeypatch, capsys): choco = FakeChoco(tmw, monkeypatch) @@ -214,15 +212,9 @@ def test_chocolatey_missing(self, tmw, monkeypatch, capsys, cleanups): # --------------------------------------------------------------------------- # -# "Already installed?" decisions, using the REAL lookup functions +# Detection, using the REAL lookup functions # --------------------------------------------------------------------------- # -CHOCO_V1_FEED = ("Chocolatey v1.4.0\n" - "ngspice 44.2 [Approved] Downloads cached for licensed users\n" - "1 packages found.\n") - - -def choco_v2_local(version): - return f"Chocolatey v2.2.2\nngspice {version}\n1 packages installed.\n" +CHOCO = r"C:\ProgramData\chocolatey\bin\choco.exe" class _FakeRegKey: @@ -237,29 +229,38 @@ def __exit__(self, *exc): class LookupHost: - """A fake Windows machine for find_ngspice_safe() / find_kicad_fixed(). - - Only the outside world is faked: what is on PATH, the output of - "choco list", the default install folders (redirected into tmp_path) and - the Uninstall registry key. The lookup functions themselves run unchanged. - Installs are recorded, never executed. + """A fake Windows machine for the real lookup functions. + + Only the outside world is faked: what is on PATH, Chocolatey (its version, + its locally installed packages and the online feed), the default install + folders (redirected into tmp_path) and the Uninstall registry key. + find_ngspice_safe(), find_kicad_fixed() and find_llvm_fixed() run + unchanged. Package installs are recorded, never executed; ``on_install`` + can simulate what an install leaves behind. """ def __init__(self, tmw, monkeypatch, tmp_path): self.tmp = tmp_path - self.choco_list = None # None: choco is not on PATH - self.registry = [] # Uninstall entries (dicts) + self.choco_version = "2.2.2" # None: Chocolatey is not installed + self.local = {} # packages Chocolatey installed here + self.feed = {} # what the online feed offers + self.list_rc = 0 + self.list_commands = [] + self.registry = [] # Uninstall entries (dicts) self.installs = [] self.cleanups = [] + self.on_install = None # callable(cmd) run for each install + self.clang_version = {} # clang.exe path -> "--version" output self.ngspice_exe = tmp_path / "ngspice" / "bin" / "ngspice.exe" + self.clang_exe = tmp_path / "LLVM" / "bin" / "clang.exe" monkeypatch.setattr(tmw, "WIN_NGSPICE_PATHS", [str(self.ngspice_exe)]) monkeypatch.setattr(tmw, "WIN_KICAD_PATHS", [ (str(self.kicad_exe(v)), v) for v in ("9", "8", "7", "6")]) + monkeypatch.setattr(tmw, "WIN_LLVM_PATHS", [str(self.clang_exe)]) monkeypatch.setattr(tmw, "which", self._which) monkeypatch.setattr(tmw, "run_cmd_safe", self._run_cmd_safe) monkeypatch.setattr(tmw, "run_cmd_stream", self._run_cmd_stream) - monkeypatch.setattr(tmw, "find_chocolatey", lambda v=None: ("choco", "2.2.2")) monkeypatch.setattr(tmw, "cleanup_kicad", lambda: self.cleanups.append(True)) monkeypatch.setattr(tmw, "install_kicad_direct", pytest.fail) monkeypatch.setitem(sys.modules, "winreg", self._winreg()) @@ -276,17 +277,38 @@ def create(path, text=""): # -- faked externals ------------------------------------------------ def _which(self, name): - if name in ("choco", "choco.exe") and self.choco_list is not None: - return r"C:\ProgramData\chocolatey\bin\choco.exe" - return None # no ngspice / kicad on PATH + if name in ("choco", "choco.exe") and self.choco_version is not None: + return CHOCO + return None # no ngspice / kicad / clang on PATH def _run_cmd_safe(self, cmd, timeout=30, **kw): - if cmd[1:2] == ["list"]: - return subprocess.CompletedProcess(cmd, 0, self.choco_list or "", "") + if cmd[:2] == [CHOCO, "--version"]: + return subprocess.CompletedProcess(cmd, 0, self.choco_version + "\n", "") + if cmd[:2] == [CHOCO, "list"]: + self.list_commands.append(list(cmd)) + return self._choco_list(cmd) + if cmd[1:] == ["--version"] and cmd[0] in self.clang_version: + return subprocess.CompletedProcess(cmd, 0, self.clang_version[cmd[0]], "") return None + def _choco_list(self, cmd): + """Chocolatey 2.x lists local packages; 1.x lists the online feed + unless --local-only is given (and has no --local-only in 2.x).""" + v2 = int(self.choco_version.split(".")[0]) >= 2 + if v2 and "--local-only" in cmd: + return subprocess.CompletedProcess(cmd, 1, "", "Unknown option --local-only") + source = self.local if (v2 or "--local-only" in cmd) else self.feed + name = cmd[cmd.index("--exact") + 1] + lines = [f"{name}|{source[name]}"] if name in source else [] + if "--limit-output" not in cmd: + lines = [f"Chocolatey v{self.choco_version}"] + \ + [l.replace("|", " ") for l in lines] + ["0 packages found."] + return subprocess.CompletedProcess(cmd, self.list_rc, "\n".join(lines) + "\n", "") + def _run_cmd_stream(self, cmd, timeout=900, **kw): self.installs.append(list(cmd)) + if self.on_install: + self.on_install(cmd) return 0, "" def _winreg(self): @@ -316,45 +338,95 @@ def host(tmw, monkeypatch, tmp_path): return LookupHost(tmw, monkeypatch, tmp_path) +class TestChocoInstalledVersion: + def test_v2_lists_local_packages_without_local_only(self, tmw, host): + host.local["ngspice"] = "42" + assert tmw._choco_installed_version("ngspice") == "42" + assert host.list_commands == [[CHOCO, "list", "--exact", "ngspice", + "--limit-output"]] + + def test_v1_uses_local_only_and_ignores_the_feed(self, tmw, host): + host.choco_version = "1.4.0" + host.local["ngspice"] = "41" + host.feed["ngspice"] = "44.2" + assert tmw._choco_installed_version("ngspice") == "41" + assert host.list_commands == [[CHOCO, "list", "--local-only", "--exact", + "ngspice", "--limit-output"]] + + def test_v1_feed_only_is_not_installed(self, tmw, host): + host.choco_version = "1.4.0" + host.feed["ngspice"] = "44.2" + assert tmw._choco_installed_version("ngspice") is None + + @pytest.mark.parametrize("choco_version", [None, "unknown", ""]) + def test_no_or_unparseable_chocolatey_gives_no_version(self, tmw, host, + choco_version): + host.choco_version = choco_version + host.local["ngspice"] = "42" + assert tmw._choco_installed_version("ngspice") is None + assert host.list_commands == [] + + def test_failed_query_gives_no_version(self, tmw, host): + host.local["ngspice"] = "42" + host.list_rc = 1 + assert tmw._choco_installed_version("ngspice") is None + + def test_other_package_names_are_ignored(self, tmw, host): + host.local["ngspice-dev"] = "1.0" + assert tmw._choco_installed_version("ngspice") is None + + class TestNgspiceDetection: @pytest.mark.parametrize("target", ["latest", "44"]) - def test_feed_listing_without_executable_still_installs(self, tmw, host, - target): + def test_v1_feed_listing_without_executable_installs(self, tmw, host, + target): """Regression (independent review): on Chocolatey 1.x "choco list" - shows the online feed, so find_ngspice_safe() reports ngspice 44.2 - although nothing is installed. The install used to be skipped.""" - host.choco_list = CHOCO_V1_FEED - assert tmw.find_ngspice_safe(None) == ("ngspice", "44.2") # the trigger + shows the online feed, and find_ngspice_safe() used to report + ngspice 44.2 although nothing was installed, so the install was + skipped.""" + host.choco_version = "1.4.0" + host.feed["ngspice"] = "44.2" + assert tmw.find_ngspice_safe(None) == (None, None) tmw.install_ngspice(target) assert len(host.installs) == 1 assert host.installs[0][:3] == ["choco", "install", "ngspice"] - def test_local_listing_without_executable_still_installs(self, tmw, host): - """Package registered with Chocolatey but its files are gone.""" - host.choco_list = choco_v2_local("42") + def test_v1_reports_the_local_version_not_the_feed(self, tmw, host): + host.choco_version = "1.4.0" + host.create(host.ngspice_exe) + host.local["ngspice"] = "41" + host.feed["ngspice"] = "44.2" + assert tmw.find_ngspice_safe(None) == (str(host.ngspice_exe), "41") + tmw.install_ngspice("44.2") + assert [version_arg(c) for c in host.installs] == ["44.2"] + + def test_registered_package_without_executable_installs(self, tmw, host): + host.local["ngspice"] = "42" + assert tmw.find_ngspice_safe(None) == (None, None) tmw.install_ngspice("42") assert len(host.installs) == 1 def test_installed_executable_with_matching_version_is_skipped( self, tmw, host, capsys): host.create(host.ngspice_exe) - host.choco_list = choco_v2_local("42") + host.local["ngspice"] = "42" tmw.install_ngspice("42") assert host.installs == [] assert last_status(capsys.readouterr().out) == "installed|42|42" def test_installed_executable_with_other_version_installs(self, tmw, host): host.create(host.ngspice_exe) - host.choco_list = choco_v2_local("41") + host.local["ngspice"] = "41" tmw.install_ngspice("42") assert [version_arg(c) for c in host.installs] == ["42"] def test_executable_with_unknown_version(self, tmw, host): - """Without Chocolatey the version is "unknown": good enough for - "latest", never for a specific version.""" + """Installed outside Chocolatey: the version is "unknown", which is + good enough for "latest" but never for a specific version.""" host.create(host.ngspice_exe) + assert tmw.find_ngspice_safe(None) == (str(host.ngspice_exe), "unknown") tmw.install_ngspice("latest") assert host.installs == [] tmw.install_ngspice("42") @@ -365,25 +437,81 @@ def test_nothing_installed_installs(self, tmw, host): assert host.installs == [["choco", "install", "ngspice", "-y", "--no-progress"]] +class TestNgspiceInstallResult: + def _installs(self, host, version, create_exe=True): + def on_install(cmd): + host.local["ngspice"] = version + if create_exe: + host.create(host.ngspice_exe) + host.on_install = on_install + + def test_success_reports_the_installed_version(self, tmw, host, capsys): + self._installs(host, "42") + tmw.install_ngspice("42") + assert last_status(capsys.readouterr().out) == "installed|42|42" + + def test_command_succeeded_but_no_executable_is_a_failure(self, tmw, host, + capsys): + """Regression: on Chocolatey 1.x the feed listing alone used to make + a failed install look successful.""" + host.choco_version = "1.4.0" + host.feed["ngspice"] = "44.2" + tmw.install_ngspice("latest") + assert last_status(capsys.readouterr().out) == "install_failed|not_found|latest" + + def test_different_version_after_install_is_a_failure(self, tmw, host, + capsys): + host.create(host.ngspice_exe) + host.local["ngspice"] = "41" + tmw.install_ngspice("42") # command "succeeds", 41 stays + assert last_status(capsys.readouterr().out) == "install_failed|version_41|42" + + def test_unknown_version_after_install_is_not_invented(self, tmw, host, + capsys): + host.on_install = lambda cmd: host.create(host.ngspice_exe) + tmw.install_ngspice("42") # not recorded by Chocolatey + assert last_status(capsys.readouterr().out) == "installed|unknown|42" + + class TestKicadDetection: def stale_entry(self, host, version="8.0.9"): return {"DisplayName": f"KiCad {version}", "DisplayVersion": version, "InstallLocation": str(host.tmp / "Program Files" / "KiCad" / "removed")} @pytest.mark.parametrize("target", ["latest", "8"]) - def test_stale_registry_entry_still_installs(self, tmw, host, target): + def test_stale_registry_entry_does_not_block_install(self, tmw, host, target): """Regression (independent review): an Uninstall registry entry whose folder was deleted made find_kicad_fixed() return that folder, and the install was skipped as "already installed".""" host.registry.append(self.stale_entry(host)) - assert tmw.find_kicad_fixed(None) == ( - host.registry[0]["InstallLocation"], "8.0.9") # the trigger + assert tmw.find_kicad_fixed(None) == (None, None) tmw.install_kicad(target) assert host.cleanups == [True] assert host.installs and host.installs[0][:3] == ["choco", "install", "kicad"] + def test_stale_registry_entry_is_not_reported_as_success(self, tmw, host, + capsys): + """After a "successful" choco run that left no kicad.exe, the stale + entry used to be accepted as proof of installation.""" + host.registry.append(self.stale_entry(host)) + tmw.install_kicad("latest") + assert last_status(capsys.readouterr().out).startswith("install_failed|") + + def test_install_that_creates_kicad_succeeds(self, tmw, host, capsys): + host.on_install = lambda cmd: host.create(host.kicad_exe("9")) + tmw.install_kicad("latest") + assert last_status(capsys.readouterr().out) == "installed|9.x|latest" + + def test_stale_entry_does_not_hide_a_real_install(self, tmw, host): + location = host.tmp / "Apps" / "KiCad" + host.create(location / "bin" / "kicad.exe") + host.registry += [self.stale_entry(host, "7.0.11"), + {"DisplayName": "KiCad 8.0.9", "DisplayVersion": "8.0.9", + "InstallLocation": str(location)}] + assert tmw.find_kicad_fixed(None) == (str(location / "bin" / "kicad.exe"), "8.0.9") + def test_registry_entry_with_executable_is_skipped(self, tmw, host, capsys): location = host.tmp / "Apps" / "KiCad" host.create(location / "bin" / "kicad.exe") @@ -424,3 +552,190 @@ def test_nothing_installed_installs(self, tmw, host): tmw.install_kicad("latest") assert host.cleanups == [True] assert host.installs[0][:3] == ["choco", "install", "kicad"] + + def test_uninstall_with_only_a_stale_entry_left_succeeds(self, tmw, host, + capsys): + """The stale entry made uninstall_kicad() report still_found.""" + host.registry.append(self.stale_entry(host)) + tmw.uninstall_kicad() + assert last_status(capsys.readouterr().out) == "not_installed|none|none" + + +# --------------------------------------------------------------------------- # +# LLVM: same uninstall-first and invented-version problems as ngspice +# --------------------------------------------------------------------------- # +class TestInstallLlvm: + def _clang(self, host, version_output="clang version 17.0.6\nTarget: x86_64"): + host.create(host.clang_exe) + host.clang_version[str(host.clang_exe)] = version_output + + def test_already_installed_version_is_skipped(self, tmw, host, capsys): + self._clang(host) + tmw.install_llvm("17") + assert host.installs == [] + assert last_status(capsys.readouterr().out) == "installed|17|17" + + def test_switching_version_never_uninstalls_first(self, tmw, host): + """Regression: install_llvm() ran "choco uninstall llvm" first.""" + self._clang(host) + tmw.install_llvm("18") + assert [c for c in host.installs if c[1] == "uninstall"] == [] + assert [version_arg(c) for c in host.installs] == ["18.1.8"] + + def test_update_to_latest_uses_choco_upgrade(self, tmw, host): + self._clang(host) + tmw.install_llvm("latest", upgrade=True) + assert host.installs == [["choco", "upgrade", "llvm", "-y", "--no-progress"]] + + def test_success_reports_the_version_clang_reports(self, tmw, host, capsys): + host.on_install = lambda cmd: self._clang(host, "clang version 18.1.8\n") + tmw.install_llvm("18") + assert last_status(capsys.readouterr().out) == "installed|18|18" + + def test_unreadable_version_is_not_invented(self, tmw, host, capsys): + """Regression: when clang --version could not be read, the requested + version was reported as installed.""" + host.on_install = lambda cmd: self._clang(host, "garbage\n") + tmw.install_llvm("18") + assert last_status(capsys.readouterr().out) == "install_failed|verification_failed|18" + + def test_wrong_version_after_install_is_a_failure(self, tmw, host, capsys): + self._clang(host) # 17 stays after the "install" + tmw.install_llvm("18") + assert last_status(capsys.readouterr().out) == "install_failed|version_17|18" + + +# --------------------------------------------------------------------------- # +# GHDL, Verilator, eSim and MSYS2 helpers +# --------------------------------------------------------------------------- # +class TestInstallGhdl: + @pytest.fixture + def ghdl(self, tmw, monkeypatch, tmp_path): + """A downloadable zip containing ghdl.exe; BASE_DIR moved to tmp.""" + monkeypatch.setattr(tmw, "BASE_DIR", tmp_path / "toolManager") + + def fake_download(url, dest, hook=None): + with zipfile.ZipFile(dest, "w") as zf: + zf.writestr("GHDL/bin/ghdl.exe", "") + + monkeypatch.setattr(tmw.urllib.request, "urlretrieve", fake_download) + version = {} + + def fake_run(cmd, timeout=30, env=None, **kw): + return subprocess.CompletedProcess(cmd, version["rc"], version["out"], "") + + monkeypatch.setattr(tmw, "run_cmd_safe", fake_run) + return version + + def test_working_exe_reports_its_version(self, tmw, ghdl, capsys): + ghdl.update(rc=0, out="GHDL 5.1.1 (tarball) [Dunoon edition]\n") + tmw.install_ghdl("latest") + assert last_status(capsys.readouterr().out) == "installed|5.1.1|latest" + + def test_exe_that_does_not_run_is_a_failure(self, tmw, ghdl, capsys): + """Regression: a ghdl.exe failing --version was reported as + installed with the requested version.""" + ghdl.update(rc=1, out="") + tmw.install_ghdl("5.1.1") + assert last_status(capsys.readouterr().out) == "install_failed|version_check_failed|5.1.1" + + +class TestInstallVerilator: + @pytest.fixture + def archive(self, tmw, monkeypatch, tmp_path): + """A local Verilator archive with a fake py7zr and MSYS2 in tmp.""" + monkeypatch.setattr(tmw, "DOWNLOAD_DIR", tmp_path / "Download") + monkeypatch.setattr(tmw, "MSYS2_PATH", tmp_path / "msys64") + archive = tmp_path / "Download" / "verilator-5.032.7z" + archive.parent.mkdir(parents=True) + archive.write_text("") + + class SevenZipFile: + def __init__(self, path, mode="r"): + pass + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def extractall(self, dest): + (dest / "verilator" / "bin").mkdir(parents=True) + + monkeypatch.setitem(sys.modules, "py7zr", + types.SimpleNamespace(SevenZipFile=SevenZipFile)) + return archive + + def test_copied_but_not_found_is_a_failure(self, tmw, archive, monkeypatch, + capsys): + """Regression: files copied but no executable found was reported as + installed with the requested version.""" + monkeypatch.setattr(tmw, "find_verilator", lambda v=None: (None, None)) + tmw.install_verilator("5.032") + out = capsys.readouterr().out + assert last_status(out) == "install_failed|not_found_after_install|5.032" + assert "installed|" not in out.replace("install_failed|", "") + + def test_found_with_unknown_version_is_not_invented(self, tmw, archive, + monkeypatch, capsys): + monkeypatch.setattr(tmw, "find_verilator", + lambda v=None: ("C:/msys64/mingw64/bin/verilator.exe", None)) + tmw.install_verilator("5.032") + assert last_status(capsys.readouterr().out) == "installed|unknown|5.032" + + def test_msys2_update_does_not_remove_verilator_first(self, tmw, monkeypatch, + capsys): + """Regression: an update ran "pacman -R" before installing again.""" + commands = [] + monkeypatch.setattr(tmw, "get_msys2_bash", lambda: "C:/msys64/usr/bin/bash.exe") + monkeypatch.setattr(tmw, "run_msys2_command", + lambda cmd, timeout=300: commands.append(cmd) + or types.SimpleNamespace(returncode=1, stdout="", stderr="")) + tmw.install_verilator("latest", upgrade=True) + assert not [c for c in commands if "pacman -R" in c] + assert any("pacman -S --noconfirm mingw-w64-x86_64-verilator" in c for c in commands) + assert last_status(capsys.readouterr().out).startswith("install_failed|") + + +class TestInstallEsim: + @pytest.fixture + def esim(self, tmw, monkeypatch, tmp_path): + monkeypatch.setattr(tmw, "BASE_DIR", tmp_path / "toolManager") + monkeypatch.setattr(tmw, "ESIM_DIR", tmp_path / "FOSSEE" / "eSim") + monkeypatch.setattr(tmw, "STATE_FILE", tmp_path / "information.json") + installer = tmp_path / "eSim-2.4_installer.exe" + installer.write_text("") + monkeypatch.setattr(tmw, "_get_package", lambda *a, **kw: installer) + result = {} + monkeypatch.setattr(tmw.subprocess, "run", + lambda cmd, timeout=None: subprocess.CompletedProcess( + cmd, result["rc"])) + return result + + def test_failed_installer_over_old_install_is_a_failure(self, tmw, esim, + capsys): + """Regression: the installer's exit code was ignored, so an older + eSim still on disk made a failed install look successful.""" + LookupHost.create(tmw.ESIM_DIR / "eSim.bat") + esim["rc"] = 2 + tmw.install_esim("2.4") + assert last_status(capsys.readouterr().out) == "install_failed|installer_exit_2|2.4" + assert not tmw.STATE_FILE.exists() + + def test_successful_installer_is_verified(self, tmw, esim, capsys): + LookupHost.create(tmw.ESIM_DIR / "eSim.bat") + esim["rc"] = 0 + tmw.install_esim("2.4") + assert last_status(capsys.readouterr().out) == "installed|2.4|2.4" + + +def test_msys2_env_adds_msys2_usr_bin(tmw, monkeypatch, tmp_path): + """Regression: get_msys2_env() added /../usr/bin (C:\\usr\\bin) + instead of /usr/bin to PATH.""" + usr_bin = tmp_path / "msys64" / "usr" / "bin" + usr_bin.mkdir(parents=True) + monkeypatch.setattr(tmw, "MSYS2_PATH", tmp_path / "msys64") + monkeypatch.setattr(tmw, "get_msys2_mingw_bin", lambda: None) + path = tmw.get_msys2_env()["PATH"].split(tmw.os.pathsep) + assert path[0] == str(usr_bin) diff --git a/src/toolManager/tool_manager_windows.py b/src/toolManager/tool_manager_windows.py index 4c5e8bd0e..7288bd8ca 100644 --- a/src/toolManager/tool_manager_windows.py +++ b/src/toolManager/tool_manager_windows.py @@ -155,9 +155,11 @@ def find_kicad_fixed(version=None): dv = winreg.QueryValueEx(sk, "DisplayVersion")[0] loc = winreg.QueryValueEx(sk, "InstallLocation")[0] exe = os.path.join(loc, "bin", "kicad.exe") + if not os.path.isfile(exe): + continue # stale entry: KiCad was removed or moved major = dv.split(".")[0] if dv else "unknown" if version is None or str(version) in ("latest", major): - return (exe if os.path.exists(exe) else loc), dv + return exe, dv except OSError: pass except OSError: @@ -168,33 +170,42 @@ def find_kicad_fixed(version=None): ep = which("kicad") or which("kicad.exe") return (ep, "unknown") if ep else (None, None) +def _choco_installed_version(package): + """Version of *package* that Chocolatey installed on THIS machine, or None. + + Chocolatey 2.x "choco list" only lists local packages. On 1.x the same + command searches the online feed, and local packages need --local-only. + When the Chocolatey version is unclear or the query fails, return None + instead of guessing. + """ + choco_exe, choco_version = find_chocolatey(None) + major = re.match(r"(\d+)\.", choco_version or "") + if not choco_exe or not major: + return None + cmd = [choco_exe, "list", "--exact", package, "--limit-output"] + if int(major.group(1)) < 2: + cmd.insert(2, "--local-only") + result = run_cmd_safe(cmd, timeout=15) + if not result or result.returncode != 0: + return None + for line in result.stdout.splitlines(): + name, sep, found = line.strip().partition("|") + if sep and found and name.lower() == package.lower(): + return found + return None + + def find_ngspice_safe(version=None): - choco = which("choco") or which("choco.exe") - if choco: - try: - for choco_cmd in [ - [choco, "list", "--exact", "ngspice"], - [choco, "list", "--local-only", "--exact", "ngspice"], - ]: - result = run_cmd_safe(choco_cmd, timeout=15) - if result and result.returncode == 0: - for line in result.stdout.splitlines(): - m = re.match(r'ngspice\s+(\S+)', line, re.IGNORECASE) - if m: - found_ver = m.group(1) - exe = _find_ngspice_exe() - if version is None or found_ver.startswith(str(version)): - return exe or "ngspice", found_ver - return exe or "ngspice", found_ver - break - except Exception: - pass + """Return (exe, version) of the ngspice executable on this machine. + A Chocolatey record alone is not proof of an installation: without an + executable this returns (None, None). The version is Chocolatey's local + record, or "unknown" (ngspice.exe itself is not run to ask). + """ exe = _find_ngspice_exe() - if exe: - return exe, "unknown" - - return None, None + if not exe: + return None, None + return exe, _choco_installed_version("ngspice") or "unknown" def _find_ngspice_exe(): @@ -296,7 +307,7 @@ def get_msys2_env(): if msys_bin: paths.append(str(msys_bin)) msys_root = MSYS2_PATH if msys_root: - usr_bin = msys_root.parent / "usr" / "bin" + usr_bin = msys_root / "usr" / "bin" if usr_bin.exists(): paths.append(str(usr_bin)) if paths: env["PATH"] = os.pathsep.join(paths) + os.pathsep + env.get("PATH", "") @@ -464,11 +475,10 @@ def check_kicad(target_version): def install_kicad(target_version, upgrade=False): # The install paths below start with cleanup_kicad(), which removes the # existing KiCad. Never do that just to reinstall the version already there. - # find_kicad_fixed() can return a registry InstallLocation whose folder no - # longer exists, so only skip when an actual kicad.exe file is present. + # (find_kicad_fixed() only reports a kicad.exe that exists.) if not upgrade: exe, current = find_kicad_fixed(None) - if exe and os.path.isfile(exe) and _version_satisfies(current, target_version): + if exe and _version_satisfies(current, target_version): print(f"[OK] KiCad {current} is already installed") print_status("installed", current, target_version) return @@ -554,14 +564,7 @@ def install_kicad(target_version, upgrade=False): installation_success = True break - check_paths = [ - (r"C:\Program Files\KiCad\9.0\bin\kicad.exe", "9"), - (r"C:\Program Files\KiCad\8.0\bin\kicad.exe", "8"), - (r"C:\Program Files\KiCad\7.0\bin\kicad.exe", "7"), - (r"C:\Program Files\KiCad\6.0\bin\kicad.exe", "6"), - ] - - for path, ver in check_paths: + for path, ver in WIN_KICAD_PATHS: if os.path.exists(path): print(f"[SUCCESS] Found KiCad at: {path}") print_status("installed", f"{ver}.x", target_version) @@ -737,14 +740,28 @@ def _version_satisfies(installed, target_version): return installed == target_version or installed.startswith(f"{target_version}.") +def _report_install_result(tool, exe, found, target_version, missing="not_found"): + """Print the final status once the install command itself succeeded. + + Success needs an executable on disk, and a known version that differs + from the one requested counts as a failure. An unknown version is + reported as "unknown", never as the requested one. + """ + if not exe: + print(f"[ERROR] {tool} not found after installation") + print_status("install_failed", missing, target_version) + elif found not in (None, "unknown") and not _version_satisfies(found, target_version): + print(f"[ERROR] {tool} {found} is installed, not {target_version}") + print_status("install_failed", f"version_{found}", target_version) + else: + print(f"[OK] {tool} {found or 'unknown'} installed successfully") + print_status("installed", found or "unknown", target_version) + + def install_ngspice(target_version, upgrade=False): if not upgrade: - # find_ngspice_safe() also reports a version when "choco list" prints - # a matching line, even if no ngspice.exe exists (on Chocolatey 1.x - # that command lists the online feed). Only skip when the executable - # itself is present. - _, current = find_ngspice_safe(None) - if _find_ngspice_exe() and _version_satisfies(current, target_version): + exe, current = find_ngspice_safe(None) + if exe and _version_satisfies(current, target_version): print(f"[OK] ngspice {current or 'unknown'} is already installed") print_status("installed", current or "unknown", target_version) return @@ -774,11 +791,7 @@ def install_ngspice(target_version, upgrade=False): print("[2/2] Verifying installation...") time.sleep(2) exe, installed_version = find_ngspice_safe(None) - if exe: - print(f"[OK] ngspice {installed_version or 'unknown'} installed successfully") - print_status("installed", installed_version or "unknown", target_version) - else: - print_status("install_failed", "not_found", target_version) + _report_install_result("ngspice", exe, installed_version, target_version) else: print_status("install_failed", out[-50:].replace('\n',' ').replace('|','_') or "unknown_error", target_version) @@ -802,49 +815,39 @@ def check_llvm(target_version): print_status("not_installed", "none", target_version) def install_llvm(target_version, upgrade=False): + if not upgrade: + exe, current = find_llvm_fixed(None) + if exe and _version_satisfies(current, target_version): + print(f"[OK] LLVM {current} is already installed") + print_status("installed", current, target_version) + return + choco_exe, _ = find_chocolatey(None) if not choco_exe: print_status("install_failed", "choco_missing", target_version) return - + exact_version = LLVM_VERSIONS.get(target_version, target_version) - - print(f"[1/3] Removing existing LLVM...") - run_cmd_stream(["choco", "uninstall", "llvm", "-y", "--no-progress"], timeout=180) - - print(f"[2/3] Installing LLVM {target_version} ({exact_version}) via Chocolatey...") - + + # As for ngspice: install over the existing LLVM instead of uninstalling + # it first. Not yet verified against real Chocolatey that the + # version-specific command switches versions in place. + print(f"[1/2] Installing LLVM {target_version} ({exact_version}) via Chocolatey...") + if target_version == "latest": - cmd = ["choco", "install", "llvm", "-y", "--no-progress"] + cmd = ["choco", "upgrade" if upgrade else "install", "llvm", "-y", "--no-progress"] else: cmd = ["choco", "install", "llvm", "--version", exact_version, "-y", "--no-progress", "--allow-downgrade", "--force"] - + rc, out = run_cmd_stream(cmd, timeout=300) - + if rc == 0: - print("[3/3] Verifying LLVM installation...") + print("[2/2] Verifying LLVM installation...") time.sleep(3) - - common_paths = [ - r"C:\Program Files\LLVM\bin\clang.exe", - r"C:\Program Files (x86)\LLVM\bin\clang.exe", - ] - for path in common_paths: - if os.path.exists(path): - result = run_cmd_safe([path, "--version"], timeout=10) - if result and result.returncode == 0: - match = re.search(r'clang version (\d+)\.', result.stdout) - if match: - found_ver = match.group(1) - print(f"[SUCCESS] LLVM {found_ver} installed at {path}") - print_status("installed", found_ver, target_version) - return - print(f"[SUCCESS] LLVM installed at {path}") - print_status("installed", exact_version or target_version, target_version) - return - - print("[ERROR] LLVM verification failed - exe not found after install") - print_status("install_failed", "verification_failed", target_version) + # find_llvm_fixed() only reports a clang whose --version it can read. + exe, found_ver = find_llvm_fixed(None) + _report_install_result("LLVM", exe, found_ver, target_version, + missing="verification_failed") else: print_status("install_failed", out[-50:].replace('\n',' ').replace('|','_') or "choco_error", target_version) @@ -996,8 +999,8 @@ def _ghdl_hook(n, bs, total): print(f"[OK] GHDL {version} ready to use") print_status("installed", version, v) else: - print(f"[WARNING] Version check failed") - print_status("installed", v, v) + print(f"[ERROR] {dest_exe} was copied but does not run (--version failed)") + print_status("install_failed", "version_check_failed", v) else: print(f"[ERROR] No ghdl.exe found in archive") print_status("install_failed", "no_exe", v) @@ -1100,11 +1103,11 @@ def _install_verilator_from_7z(archive_path, v): time.sleep(2) exe, installed_ver = find_verilator(None) if exe: - print(f"[OK] Verilator {installed_ver or v} is ready") - return True - else: - print("[WARNING] Files copied but could not verify version") + print(f"[OK] Verilator {installed_ver or 'unknown'} is ready") return True + print("[ERROR] Files copied but no Verilator executable was found") + print_status("install_failed", "not_found_after_install", v) + return False def install_verilator(v, upgrade=False): @@ -1117,7 +1120,7 @@ def install_verilator(v, upgrade=False): print(f"[INFO] Using local archive: {fname}") if _install_verilator_from_7z(archive, v): exe, installed_ver = find_verilator(None) - print_status("installed", installed_ver or v, v) + print_status("installed", installed_ver or "unknown", v) return else: print(f"[INFO] Archive not found: {archive}") @@ -1136,11 +1139,9 @@ def install_verilator(v, upgrade=False): print("[MSYS2] Step 1/3: Updating package database...") run_msys2_command("pacman -Syu --noconfirm 2>&1 || true") - if upgrade: - print("[MSYS2] Step 2/3: Removing existing Verilator...") - run_msys2_command("pacman -R --noconfirm mingw-w64-x86_64-verilator 2>/dev/null || true") - else: - print("[MSYS2] Step 2/3: Preparing...") + # "pacman -S" upgrades or reinstalls an installed package in place, so + # an update no longer removes the working Verilator first. + print("[MSYS2] Step 2/3: Preparing...") print("[MSYS2] Step 3/3: Installing Verilator (may take a few minutes)...") result = run_msys2_command("pacman -S --noconfirm mingw-w64-x86_64-verilator") @@ -1255,7 +1256,13 @@ def install_esim(target_version, upgrade=False): print(f"[INFO] Running installer: {installer.name}") print("[INFO] Silent install in progress (10-20 min). Please wait...") try: - subprocess.run([str(installer), "/S"], timeout=1800) + result = subprocess.run([str(installer), "/S"], timeout=1800) + if result.returncode != 0: + # An older eSim may still be present, so the file checks below + # cannot tell whether this installer actually worked. + print(f"[ERROR] eSim installer exited with code {result.returncode}") + print_status("install_failed", f"installer_exit_{result.returncode}", target_version) + return time.sleep(20) for p in [BASE_DIR / "eSim.bat", BASE_DIR / "src" / "frontEnd" / "Application.py", ESIM_DIR / "eSim.bat", ESIM_DIR / "uninst-eSim.exe"]: if p.exists(): From 6f9f1a92eb14c2d134f8e55ae5637487f83bc1e3 Mon Sep 17 00:00:00 2001 From: venkateshghule24 Date: Wed, 4 Nov 2026 20:52:27 +0530 Subject: [PATCH 11/11] Verify Linux eSim installs and survive an unusable apt lock file - Updating eSim on apt passed --upgrade, which install-eSim.sh rejects (it only accepts --install, --install-analog, --install-digital and --uninstall), so every update failed. It is now reported as not supported (exit 3). - install-eSim-24.04.sh can exit 0 without installing eSim, e.g. when KiCad 8 is already present. Install now checks for the /usr/bin/esim launcher the scripts create, and uninstall checks that it is gone. - If the apt serial lock file cannot be opened (e.g. it belongs to another user), the backend crashed without a status line. It now warns and continues; apt still takes its own dpkg lock. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_tool_manager_linux.py | 59 ++++++++++++++++--- src/toolManager/tool_manager_linux.py | 31 +++++++++- 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/toolManager/tests/test_tool_manager_linux.py b/src/toolManager/tests/test_tool_manager_linux.py index f147f0277..be9a0c2ec 100644 --- a/src/toolManager/tests/test_tool_manager_linux.py +++ b/src/toolManager/tests/test_tool_manager_linux.py @@ -163,6 +163,18 @@ def test_lock_released_when_command_fails(self, tml, fs): tml.install_tool("ghdl", "latest") assert fs.events == ["lock", "run", "unlock"] + def test_unopenable_lock_file_does_not_abort(self, tml, fs, monkeypatch, + tmp_path, capsys): + """Regression: if the lock file could not be opened (e.g. created by + another user) the backend crashed with a traceback and no status.""" + monkeypatch.setattr(tml, "_APT_LOCK_FILE", str(tmp_path)) # a directory + fs.installed["ghdl"] = "/usr/bin/ghdl" + assert tml.install_tool("ghdl", "latest") == tml.EXIT_OK + assert fs.events == ["run"] + out = capsys.readouterr().out + assert "[WARN] Cannot open" in out + assert status_lines(out) == ["installed|ghdl|/usr/bin/ghdl"] + @pytest.mark.parametrize("pm", ["dnf", "pacman", "zypper", "yum"]) def test_other_managers_are_not_locked(self, tml, fs, pm): fs.pm = pm @@ -248,14 +260,39 @@ def script(self, tml, monkeypatch, tmp_path): monkeypatch.setattr(tml, "INSTALL_SH", path) return path - @pytest.mark.parametrize("upgrade,flag", [(False, "--install"), - (True, "--upgrade")]) - def test_install_runs_the_script(self, tml, monkeypatch, script, - upgrade, flag): + def test_install_runs_the_script_and_finds_the_launcher(self, tml, monkeypatch, + capsys, script): fs = FakeSystem(tml, monkeypatch, pm="apt") - assert tml.install_tool("esim", "2.4", upgrade=upgrade) == tml.EXIT_OK - assert fs.streamed == [["bash", str(script), flag]] + fs.installed["esim"] = "/usr/bin/esim" + assert tml.install_tool("esim", "2.4") == tml.EXIT_OK + assert fs.streamed == [["bash", str(script), "--install"]] assert fs.commands == [] + assert status_lines(capsys.readouterr().out) == ["installed|esim|/usr/bin/esim"] + + def test_script_success_without_launcher_is_a_failure(self, tml, monkeypatch, + capsys, script): + """Regression: install-eSim-24.04.sh exits 0 without installing eSim + when KiCad 8 is already installed; that was reported as success.""" + FakeSystem(tml, monkeypatch, pm="apt", rc=0) # no /usr/bin/esim + assert tml.install_tool("esim", "2.4") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == ["install_failed|esim|not_found"] + + def test_update_is_unsupported(self, tml, monkeypatch, capsys, script): + """Regression: update passed --upgrade, which install-eSim.sh rejects + (it only accepts --install*/--uninstall), so every update failed.""" + fs = FakeSystem(tml, monkeypatch, pm="apt") + fs.installed["esim"] = "/usr/bin/esim" + assert tml.install_tool("esim", "2.4", upgrade=True) == tml.EXIT_UNSUPPORTED + assert fs.streamed == [] + assert status_lines(capsys.readouterr().out) == ["not_supported|esim|apt"] + + def test_script_accepts_the_flags_the_backend_uses(self, tml): + """The flags the backend passes must be ones install-eSim.sh accepts.""" + text = tml.INSTALL_SH.read_text(encoding="utf-8") + accepted = text.split('if [[ "$ARGUMENT" != ', 1)[1].split("]]; then", 1)[0] + for flag in ("--install", "--uninstall"): + assert f'"{flag}"' in accepted + assert "--upgrade" not in accepted def test_script_failure_is_reported(self, tml, monkeypatch, capsys, script): FakeSystem(tml, monkeypatch, pm="apt", rc=2) @@ -269,10 +306,18 @@ def test_missing_script_is_a_failure(self, tml, monkeypatch, capsys, tmp_path): assert fs.streamed == [] assert "install_failed|esim|script_missing" in capsys.readouterr().out - def test_uninstall_runs_the_script(self, tml, monkeypatch, script): + def test_uninstall_runs_the_script(self, tml, monkeypatch, capsys, script): fs = FakeSystem(tml, monkeypatch, pm="apt") assert tml.uninstall_tool("esim", "none") == tml.EXIT_OK assert fs.streamed == [["bash", str(script), "--uninstall"]] + assert status_lines(capsys.readouterr().out) == ["not_installed|esim|none"] + + def test_uninstall_leaving_the_launcher_is_a_failure(self, tml, monkeypatch, + capsys, script): + fs = FakeSystem(tml, monkeypatch, pm="apt", rc=0) + fs.installed["esim"] = "/usr/bin/esim" + assert tml.uninstall_tool("esim", "none") == tml.EXIT_FAILED + assert status_lines(capsys.readouterr().out) == ["uninstall_failed|esim|still_found"] def test_uninstall_script_failure_is_reported(self, tml, monkeypatch, capsys, script): diff --git a/src/toolManager/tool_manager_linux.py b/src/toolManager/tool_manager_linux.py index 8ab31187b..16778cb60 100644 --- a/src/toolManager/tool_manager_linux.py +++ b/src/toolManager/tool_manager_linux.py @@ -200,7 +200,17 @@ def _apt_lock(pm: str = "apt"): import fcntl as _fcntl - with open(_APT_LOCK_FILE, "w") as _lf: + try: + _lf = open(_APT_LOCK_FILE, "w") + except OSError as e: + # e.g. the file was created by another user. This lock only serialises + # the Tool Manager's own apt runs; apt still takes its own dpkg lock. + print(f"[WARN] Cannot open {_APT_LOCK_FILE} ({e}); " + "continuing without the serial lock.", flush=True) + yield + return + + with _lf: print("[INFO] Waiting for apt serial lock ...", flush=True) _fcntl.flock(_lf, _fcntl.LOCK_EX) try: @@ -258,15 +268,26 @@ def install_tool(tool: str, version: str, upgrade: bool = False) -> int: # Ubuntu/Debian + esim → delegate to the maintained shell script if pm == "apt" and tool == "esim": + if upgrade: + # install-eSim.sh only accepts --install, --install-analog, + # --install-digital and --uninstall; it has no upgrade mode. + return _unsupported(tool, pm, "install-eSim.sh cannot update eSim; " + "uninstall it and install it again.") if not INSTALL_SH.exists(): print(f"[ERROR] install-eSim.sh not found at {INSTALL_SH}", flush=True) print("install_failed|esim|script_missing", flush=True) return EXIT_FAILED - flag = "--upgrade" if upgrade else "--install" - rc = _stream(["bash", str(INSTALL_SH), flag]) + rc = _stream(["bash", str(INSTALL_SH), "--install"]) if rc != 0: print(f"install_failed|esim|exit_{rc}", flush=True) return EXIT_FAILED + # The script can exit 0 without installing eSim (e.g. when KiCad 8 is + # already installed), so check for the /usr/bin/esim launcher it creates. + where = _find_installed("esim") + if not where: + print("install_failed|esim|not_found", flush=True) + return EXIT_FAILED + print(f"installed|esim|{where}", flush=True) return EXIT_OK if tool == "esim": @@ -319,6 +340,10 @@ def uninstall_tool(tool: str, version: str) -> int: if rc != 0: print(f"uninstall_failed|esim|exit_{rc}", flush=True) return EXIT_FAILED + if _find_installed("esim"): + print("uninstall_failed|esim|still_found", flush=True) + return EXIT_FAILED + print("not_installed|esim|none", flush=True) return EXIT_OK packages = PACKAGE_MAP.get(pm, {}).get(tool)