diff --git a/requirements.txt b/requirements.txt index d3d72d34..5e1bf365 100644 --- a/requirements.txt +++ b/requirements.txt @@ -43,6 +43,9 @@ websocket-client>=1.8.0,<2.0.0 # JSON Schema validation jsonschema>=4.20.0,<5.0.0 +# Requirement specifier parsing (plugin dependency satisfaction checks) +packaging>=23.0,<27.0 + # Testing dependencies pytest>=9.0.3,<10.0.0 pytest-cov>=4.1.0,<5.0.0 diff --git a/scripts/clear_dependency_markers.sh b/scripts/clear_dependency_markers.sh deleted file mode 100755 index 8c1dfc4b..00000000 --- a/scripts/clear_dependency_markers.sh +++ /dev/null @@ -1,29 +0,0 @@ -#!/bin/bash -# Clear all plugin dependency markers to force fresh dependency check -# Useful after updating plugins or troubleshooting dependency issues - -echo "Clearing plugin dependency markers..." - -# Check both possible cache locations -CACHE_DIRS=( - "/var/cache/ledmatrix" - "$HOME/.cache/ledmatrix" -) - -for CACHE_DIR in "${CACHE_DIRS[@]}"; do - if [ -d "$CACHE_DIR" ]; then - echo "Checking $CACHE_DIR..." - marker_count=$(find "$CACHE_DIR" -name "plugin_*_deps_installed" 2>/dev/null | wc -l) - if [ "$marker_count" -gt 0 ]; then - echo "Found $marker_count dependency marker(s) in $CACHE_DIR" - find "$CACHE_DIR" -name "plugin_*_deps_installed" -delete - echo "Cleared $marker_count marker(s)" - else - echo "No dependency markers found in $CACHE_DIR" - fi - fi -done - -echo "Done! Dependency markers cleared." -echo "Next startup will check and install dependencies as needed." - diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 1001f59b..eb3ba659 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -5,10 +5,10 @@ Handles plugin module imports, dependency installation, and class instantiation. Extracted from PluginManager to improve separation of concerns. """ -import hashlib -import json import importlib +import importlib.metadata import importlib.util +import json import os import sys import subprocess @@ -17,12 +17,80 @@ from pathlib import Path from typing import Dict, Any, Optional, Tuple, Type import logging +from packaging.requirements import InvalidRequirement, Requirement + from src.exceptions import PluginError from src.logging_config import get_logger -from src.common.permission_utils import ( - ensure_file_permissions, - get_plugin_file_mode -) + + +def requirements_has_real_deps(requirements_file: str) -> bool: + """ + Check whether a requirements.txt actually specifies anything to install. + + Plugins that ship all their dependencies with LEDMatrix core often keep a + requirements.txt where every line is commented out, for documentation + purposes only. Running pip against such a file still pays the full + subprocess/resolver cost for zero effect, so callers should skip the + install step entirely when this returns False. + """ + try: + with open(requirements_file, 'r', encoding='utf-8') as fh: + for line in fh: + line = line.strip() + if line and not line.startswith('#'): + return True + except OSError: + # Let the caller's own file handling report the error. + return True + return False + + +def requirements_are_satisfied(requirements_file: str) -> bool: + """ + Check whether every real requirement line in requirements.txt is already + satisfied by packages installed in the current interpreter. + + This replaces marker-file tracking with a direct fact check, so it's + immune to stale/missing/corrupted markers: it looks at what's actually + importable right now rather than trusting a hash comparison from a + previous run. Anything ambiguous (pip options, unparseable lines, + extras, unresolvable versions) conservatively returns False so the + caller falls through to running pip — this check only ever saves work, + never masks a real install. + """ + try: + with open(requirements_file, 'r', encoding='utf-8') as fh: + lines = fh.readlines() + except OSError: + return False + + for raw_line in lines: + line = raw_line.strip() + if not line or line.startswith('#'): + continue + if line.startswith('-'): + return False # pip option (-r, --index-url, ...), can't verify + + try: + req = Requirement(line) + except InvalidRequirement: + return False + + if req.extras: + return False # verifying extras' sub-dependencies isn't worth it here + + if req.marker is not None and not req.marker.evaluate(): + continue # not applicable on this platform/interpreter + + try: + installed_version = importlib.metadata.version(req.name) + except importlib.metadata.PackageNotFoundError: + return False + + if req.specifier and not req.specifier.contains(installed_version, prereleases=True): + return False + + return True class PluginLoader: @@ -186,33 +254,23 @@ class PluginLoader: return False requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") - marker_file = os.path.join(safe_plugin_dir, ".dependencies_installed") if not os.path.isfile(requirements_file): return True # No dependencies needed - try: - with open(requirements_file, 'rb') as fh: - current_hash = hashlib.sha256(fh.read()).hexdigest() - except OSError as e: - self.logger.error("Failed to read requirements.txt for %s: %s", plugin_id, e) - return False + if not requirements_has_real_deps(requirements_file): + self.logger.debug( + "requirements.txt for %s has no real dependencies (comments/blank only), skipping pip", + plugin_id + ) + return True - # Skip if requirements.txt hasn't changed since last install - if os.path.isfile(marker_file): - try: - with open(marker_file, 'r', encoding='utf-8') as fh: - stored_hash = fh.read().strip() - except OSError as e: - self.logger.warning( - "Could not read dependency marker for %s (%s), will reinstall dependencies", - plugin_id, e - ) - else: - if stored_hash == current_hash: - self.logger.debug("Dependencies already installed for %s (requirements unchanged)", plugin_id) - return True - self.logger.info("Requirements changed for %s, reinstalling dependencies", plugin_id) + if requirements_are_satisfied(requirements_file): + self.logger.debug( + "Dependencies for %s already satisfied in current environment, skipping pip", + plugin_id + ) + return True try: self.logger.info("Installing dependencies for plugin %s...", plugin_id) @@ -225,12 +283,6 @@ class PluginLoader: ) if result.returncode == 0: - try: - with open(marker_file, 'w', encoding='utf-8') as fh: - fh.write(current_hash) - ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) - except OSError as marker_err: - self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) self.logger.info("Dependencies installed successfully for %s", plugin_id) return True else: @@ -242,8 +294,7 @@ class PluginLoader: # the system copy instead of trying to replace it — matching the # retry already used by install_dependencies_apt.py / safe_pip_install.sh. # Without this retry, the plugin would silently keep running against - # whatever version the system happened to ship, even though the - # marker below claims the requirement is satisfied. + # whatever version the system happened to ship. if "uninstall-no-record-file" in stderr: self.logger.warning( "Dependencies for %s conflict with a system-managed package " @@ -280,12 +331,6 @@ class PluginLoader: "system-managed version satisfies the requirement", plugin_id ) - try: - with open(marker_file, 'w', encoding='utf-8') as fh: - fh.write(current_hash) - ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) - except OSError as marker_err: - self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) return True self.logger.warning( "Dependency installation returned non-zero exit code for %s: %s", diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 22f8b470..6b4a5733 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -9,7 +9,6 @@ API Version: 1.0.0 import json import sys -import subprocess import time import threading from pathlib import Path @@ -177,90 +176,6 @@ class PluginManager: return plugin_ids - def _get_dependency_marker_path(self, plugin_id: str) -> Path: - """Get path to dependency installation marker file.""" - plugin_dir = self.plugins_dir / plugin_id - if not plugin_dir.exists(): - # Try with ledmatrix- prefix - plugin_dir = self.plugins_dir / f"ledmatrix-{plugin_id}" - return plugin_dir / ".dependencies_installed" - - def _check_dependencies_installed(self, plugin_id: str) -> bool: - """Check if dependencies are already installed for a plugin.""" - marker_path = self._get_dependency_marker_path(plugin_id) - return marker_path.exists() - - def _mark_dependencies_installed(self, plugin_id: str) -> None: - """Mark dependencies as installed for a plugin.""" - marker_path = self._get_dependency_marker_path(plugin_id) - try: - marker_path.touch() - # Set proper file permissions after creating marker - from src.common.permission_utils import ( - ensure_file_permissions, - get_plugin_file_mode - ) - ensure_file_permissions(marker_path, get_plugin_file_mode()) - except (OSError, PermissionError) as e: - self.logger.warning("Could not create dependency marker for %s: %s", plugin_id, e) - - def _remove_dependency_marker(self, plugin_id: str) -> None: - """Remove dependency installation marker.""" - marker_path = self._get_dependency_marker_path(plugin_id) - try: - if marker_path.exists(): - marker_path.unlink() - except (OSError, PermissionError) as e: - self.logger.warning("Could not remove dependency marker for %s: %s", plugin_id, e) - - def _install_plugin_dependencies(self, requirements_file: Path) -> bool: - """ - Install plugin dependencies from requirements.txt. - - Args: - requirements_file: Path to requirements.txt - - Returns: - True if installation succeeded or not needed, False on error - """ - try: - self.logger.info("Installing dependencies from %s", requirements_file) - result = subprocess.run( - [sys.executable, "-m", "pip", "install", "--break-system-packages", "--no-cache-dir", "-r", str(requirements_file)], - capture_output=True, - text=True, - timeout=300, - check=False - ) - - if result.returncode == 0: - self.logger.info("Dependencies installed successfully") - return True - else: - self.logger.warning("Dependency installation returned non-zero exit code: %s", result.stderr) - return False - except subprocess.TimeoutExpired: - self.logger.error("Dependency installation timed out") - return False - except FileNotFoundError as e: - self.logger.warning("Command not found: %s. Skipping dependency installation", e) - return True - except (BrokenPipeError, OSError) as e: - # Handle broken pipe errors (errno 32) which can occur during pip downloads - # Often caused by network interruptions or output buffer issues - if isinstance(e, OSError) and e.errno == 32: - self.logger.error( - "Broken pipe error during dependency installation. " - "This usually indicates a network interruption or pip output buffer issue. " - "Try installing again or check your network connection." - ) - else: - self.logger.error("OS error during dependency installation: %s", e) - return False - except Exception as e: - self.logger.error("Unexpected error installing dependencies: %s", e, exc_info=True) - return True - def load_plugin(self, plugin_id: str) -> bool: """ Load a plugin by ID. diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 0683373d..2f76151d 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -5,7 +5,6 @@ Handles plugin discovery, installation, updates, and uninstallation from both the official registry and custom GitHub repositories. """ -import hashlib import os import re import json @@ -26,6 +25,7 @@ import logging from urllib.parse import urlparse from src.common.permission_utils import sudo_remove_directory, install_requirements_file +from src.plugin_system.plugin_loader import requirements_has_real_deps, requirements_are_satisfied try: from jsonschema import Draft7Validator, ValidationError @@ -1912,7 +1912,15 @@ class PluginStoreManager: if not requirements_file.exists(): self.logger.debug(f"No requirements.txt found in {plugin_path.name}") return True - + + if not requirements_has_real_deps(str(requirements_file)): + self.logger.debug(f"requirements.txt for {plugin_path.name} has no real dependencies, skipping pip") + return True + + if requirements_are_satisfied(str(requirements_file)): + self.logger.debug(f"Dependencies for {plugin_path.name} already satisfied, skipping pip") + return True + try: self.logger.info(f"Installing dependencies for {plugin_path.name}") # Routed through the shared root-visible installer (same one the @@ -1929,12 +1937,6 @@ class PluginStoreManager: ) return False self.logger.info(f"Dependencies installed successfully for {plugin_path.name}") - # Write hash marker so plugin_loader skips redundant pip run on next startup - try: - current_hash = hashlib.sha256(requirements_file.read_bytes()).hexdigest() - (plugin_path / ".dependencies_installed").write_text(current_hash, encoding='utf-8') - except OSError as marker_err: - self.logger.debug("Could not write dependency marker for %s: %s", plugin_path.name, marker_err) return True except subprocess.TimeoutExpired: @@ -2432,19 +2434,6 @@ class PluginStoreManager: file_path = line[3:].strip() untracked_files.append(file_path) - # Remove marker files that are safe to delete (they'll be regenerated) - safe_to_remove = ['.dependencies_installed'] - removed_files = [] - for file_name in safe_to_remove: - file_path = plugin_path / file_name - if file_path.exists() and file_name in untracked_files: - try: - file_path.unlink() - removed_files.append(file_name) - self.logger.info(f"Removed marker file {file_name} from {plugin_id} before update") - except Exception as e: - self.logger.warning(f"Could not remove {file_name} from {plugin_id}: {e}") - # Check for tracked file changes status_result = subprocess.run( ['git', '-C', str(plugin_path), 'status', '--porcelain', '--untracked-files=no'], @@ -2455,10 +2444,9 @@ class PluginStoreManager: ) has_changes = bool(status_result.stdout.strip()) - # If there are remaining untracked files (not safe to remove), stash them - remaining_untracked = [f for f in untracked_files if f not in removed_files] - if remaining_untracked: - self.logger.info(f"Found {len(remaining_untracked)} untracked files in {plugin_id}, will stash them") + # If there are untracked files, stash them + if untracked_files: + self.logger.info(f"Found {len(untracked_files)} untracked files in {plugin_id}, will stash them") has_changes = True except subprocess.TimeoutExpired: # If status check times out, assume there might be changes and proceed diff --git a/test/test_plugin_loader.py b/test/test_plugin_loader.py index c1da6f5e..6e1edad6 100644 --- a/test/test_plugin_loader.py +++ b/test/test_plugin_loader.py @@ -216,20 +216,23 @@ class TestPluginLoader: plugin_dir.mkdir() requirements_file = plugin_dir / "requirements.txt" requirements_file.write_text("package1==1.0.0\n") - + mock_subprocess.return_value = MagicMock(returncode=1) result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") assert result is False + @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False) @patch('subprocess.run') def test_install_dependencies_retries_with_ignore_installed_on_apt_conflict( - self, mock_subprocess, plugin_loader, tmp_plugins_dir + self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir ): """An apt-managed package with no pip RECORD file triggers a retry with --ignore-installed rather than silently assuming the old version satisfies - the requirement.""" + the requirement. requirements_are_satisfied() is mocked False here because + this scenario is exactly the case where the installed (apt) version does + NOT satisfy the pin — that's why pip attempts a reinstall in the first place.""" plugin_dir = tmp_plugins_dir / "test_plugin" plugin_dir.mkdir() requirements_file = plugin_dir / "requirements.txt" @@ -249,9 +252,10 @@ class TestPluginLoader: retry_cmd = mock_subprocess.call_args_list[1][0][0] assert "--ignore-installed" in retry_cmd + @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False) @patch('subprocess.run') def test_install_dependencies_apt_conflict_retry_also_fails( - self, mock_subprocess, plugin_loader, tmp_plugins_dir + self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir ): """Still tolerates the failure (returns True) if the --ignore-installed retry itself fails, matching the prior soft-fallback behavior.""" @@ -272,9 +276,10 @@ class TestPluginLoader: assert result is True assert mock_subprocess.call_count == 2 + @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False) @patch('subprocess.run') def test_install_dependencies_apt_conflict_retry_times_out( - self, mock_subprocess, plugin_loader, tmp_plugins_dir + self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir ): """A retry timeout must be tolerated the same way as a retry failure (return True), not propagate to the outer TimeoutExpired handler and @@ -297,3 +302,16 @@ class TestPluginLoader: assert result is True assert mock_subprocess.call_count == 2 + + @patch('subprocess.run') + def test_install_dependencies_already_satisfied_skips_pip(self, mock_subprocess, plugin_loader, tmp_plugins_dir): + """A requirement already satisfied in the current environment shouldn't invoke pip.""" + plugin_dir = tmp_plugins_dir / "test_plugin" + plugin_dir.mkdir() + requirements_file = plugin_dir / "requirements.txt" + requirements_file.write_text("pytest>=1.0\n") + + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + + assert result is True + mock_subprocess.assert_not_called() diff --git a/test/test_plugin_system.py b/test/test_plugin_system.py index b78a3156..cf493cb8 100644 --- a/test/test_plugin_system.py +++ b/test/test_plugin_system.py @@ -84,8 +84,9 @@ class TestPluginLoader: def test_dependency_check(self): """Test dependency checking logic.""" - # This would test _check_dependencies_installed and _install_plugin_dependencies - # which requires mocking subprocess calls and file operations + # Covered by test_plugin_loader.py's install_dependencies tests, + # which exercise requirements_has_real_deps/requirements_are_satisfied + # and the pip subprocess fallback. class TestPluginExecutor: