Repository navigation
Improve Tool Manager reliability and add tests - #653
Draft
venkateshghule24 wants to merge 11 commits into
Draft
venkateshghule24 wants to merge 11 commits into
venkateshghule24 wants to merge 11 commits into
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <MSYS2>/../usr/bin on PATH instead of <MSYS2>/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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves Tool Manager reliability on Windows and Linux.
Testing: All 203 tests passed on Windows.
Note: This PR depends on PR #652 and should be reviewed after it is merged.