-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: make desktop plugin dependency loading safer on Windows #7446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
34954f4
bdf5723
b5a6040
4c53b5e
afa1e95
46a477b
3a49d34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ | |
| import sys | ||
| import tempfile | ||
| import traceback | ||
| from dataclasses import dataclass | ||
| from enum import Enum, auto | ||
| from types import ModuleType | ||
|
|
||
| import yaml | ||
|
|
@@ -37,6 +39,7 @@ | |
| from astrbot.core.utils.io import remove_dir | ||
| from astrbot.core.utils.metrics import Metric | ||
| from astrbot.core.utils.requirements_utils import ( | ||
| MissingRequirementsPlan, | ||
| plan_missing_requirements_install, | ||
| ) | ||
|
|
||
|
|
@@ -77,6 +80,19 @@ def __init__( | |
| self.error = error | ||
|
|
||
|
|
||
| class ImportDependencyRecoveryMode(Enum): | ||
| DISABLED = auto() | ||
| PRELOAD_AND_RECOVER = auto() | ||
| RECOVER_ON_FAILURE = auto() | ||
| REINSTALL_ON_FAILURE = auto() | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ImportDependencyRecoveryState: | ||
| mode: ImportDependencyRecoveryMode | ||
| install_plan: MissingRequirementsPlan | None = None | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def _temporary_filtered_requirements_file( | ||
| *, | ||
|
|
@@ -137,7 +153,10 @@ async def _install_requirements_with_precheck( | |
| requirements_path, | ||
| fallback_reason, | ||
| ) | ||
| await pip_installer.install(requirements_path=requirements_path) | ||
| await pip_installer.install( | ||
| requirements_path=requirements_path, | ||
| allow_target_upgrade=bool(install_plan.version_mismatch_names), | ||
| ) | ||
| return | ||
|
|
||
| logger.info( | ||
|
|
@@ -148,7 +167,10 @@ async def _install_requirements_with_precheck( | |
| with _temporary_filtered_requirements_file( | ||
| install_lines=install_plan.install_lines, | ||
| ) as filtered_requirements_path: | ||
| await pip_installer.install(requirements_path=filtered_requirements_path) | ||
| await pip_installer.install( | ||
| requirements_path=filtered_requirements_path, | ||
| allow_target_upgrade=bool(install_plan.version_mismatch_names), | ||
| ) | ||
|
|
||
|
|
||
| class PluginManager: | ||
|
|
@@ -332,33 +354,106 @@ async def _ensure_plugin_requirements( | |
| logger.exception(str(dependency_error)) | ||
| raise dependency_error from e | ||
|
|
||
| @staticmethod | ||
| def _resolve_import_dependency_recovery_state( | ||
| requirements_path: str, | ||
| *, | ||
| reserved: bool, | ||
| ) -> ImportDependencyRecoveryState: | ||
| if reserved or not os.path.exists(requirements_path): | ||
| return ImportDependencyRecoveryState(ImportDependencyRecoveryMode.DISABLED) | ||
|
|
||
| install_plan = plan_missing_requirements_install(requirements_path) | ||
| if install_plan is None: | ||
| return ImportDependencyRecoveryState( | ||
| ImportDependencyRecoveryMode.RECOVER_ON_FAILURE | ||
| ) | ||
| if install_plan.version_mismatch_names: | ||
| return ImportDependencyRecoveryState( | ||
| ImportDependencyRecoveryMode.REINSTALL_ON_FAILURE, | ||
| install_plan=install_plan, | ||
| ) | ||
|
|
||
| return ImportDependencyRecoveryState( | ||
| ImportDependencyRecoveryMode.PRELOAD_AND_RECOVER, | ||
| install_plan=install_plan, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _try_import_from_installed_dependencies( | ||
| path: str, | ||
| module_str: str, | ||
| root_dir_name: str, | ||
| requirements_path: str, | ||
| import_exc: Exception, | ||
| ) -> ModuleType | None: | ||
| try: | ||
| logger.info( | ||
| f"插件 {root_dir_name} 导入失败,尝试从已安装依赖恢复: {import_exc!s}" | ||
| ) | ||
| pip_installer.prefer_installed_dependencies( | ||
| requirements_path=requirements_path | ||
| ) | ||
| module = __import__(path, fromlist=[module_str]) | ||
| logger.info( | ||
| f"插件 {root_dir_name} 已从 site-packages 恢复依赖,跳过重新安装。" | ||
| ) | ||
| return module | ||
| except Exception as recover_exc: | ||
| logger.info( | ||
| f"插件 {root_dir_name} 已安装依赖恢复失败,将重新安装依赖: {recover_exc!s}" | ||
| ) | ||
| return None | ||
|
|
||
| async def _import_plugin_with_dependency_recovery( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): Consider simplifying You can get the same behavior with much less abstraction by inlining the decision logic into 1. Remove the mini state machine
You can rewrite from astrbot.core.utils.requirements_utils import plan_missing_requirements_install
async def _import_plugin_with_dependency_recovery(
self,
path: str,
module_str: str,
root_dir_name: str,
requirements_path: str,
*,
reserved: bool = False,
) -> ModuleType:
# Pre‑compute plan and flags
has_requirements = (not reserved) and os.path.exists(requirements_path)
install_plan = plan_missing_requirements_install(requirements_path) if has_requirements else None
has_version_mismatch = bool(install_plan and install_plan.version_mismatch_names)
can_use_installed_recovery = has_requirements and not has_version_mismatch
# Optional preload before first import
if can_use_installed_recovery and install_plan is not None:
try:
pip_installer.prefer_installed_dependencies(requirements_path=requirements_path)
except Exception as preload_exc:
logger.info(
"插件 %s 预加载已安装依赖失败,将继续常规导入: %s",
root_dir_name,
preload_exc,
)
try:
return __import__(path, fromlist=[module_str])
except (ModuleNotFoundError, ImportError) as import_exc:
# One recovery attempt using already-installed deps
if can_use_installed_recovery:
try:
logger.info(
"插件 %s 导入失败,尝试从已安装依赖恢复: %s",
root_dir_name,
import_exc,
)
pip_installer.prefer_installed_dependencies(requirements_path=requirements_path)
module = __import__(path, fromlist=[module_str])
logger.info("插件 %s 已从 site-packages 恢复依赖,跳过重新安装。", root_dir_name)
return module
except Exception as recover_exc:
logger.info(
"插件 %s 已安装依赖恢复失败,将重新安装依赖: %s",
root_dir_name,
recover_exc,
)
elif has_version_mismatch:
logger.info(
"插件 %s 预检查检测到版本不匹配,跳过已安装依赖恢复: %s",
root_dir_name,
sorted(install_plan.version_mismatch_names), # type: ignore[arg-type]
)
await self._check_plugin_dept_update(target_plugin=root_dir_name)
return __import__(path, fromlist=[module_str])This preserves:
After this, you can safely delete: class ImportDependencyRecoveryMode(Enum): ...
@dataclass(frozen=True)
class ImportDependencyRecoveryState: ...
@staticmethod
def _resolve_import_dependency_recovery_state(...): ...
@staticmethod
def _try_import_from_installed_dependencies(...): ...The behavior remains identical, but the control flow is linear and localized in one method, reducing the cognitive load and the number of moving parts you need to update when policies change. |
||
| self, | ||
| path: str, | ||
| module_str: str, | ||
| root_dir_name: str, | ||
| requirements_path: str, | ||
| *, | ||
| reserved: bool = False, | ||
| ) -> ModuleType: | ||
| recovery_state = self._resolve_import_dependency_recovery_state( | ||
| requirements_path, | ||
| reserved=reserved, | ||
|
Comment on lines
+417
to
+419
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question (bug_risk): Reserved plugins (or missing requirements files) still trigger dependency reinstall on import failure, which might not be desired.
|
||
| ) | ||
|
|
||
| if recovery_state.mode is ImportDependencyRecoveryMode.PRELOAD_AND_RECOVER: | ||
| try: | ||
| pip_installer.prefer_installed_dependencies( | ||
| requirements_path=requirements_path | ||
| ) | ||
|
Comment on lines
+424
to
+426
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling Consider refactoring |
||
| except Exception as preload_exc: | ||
| logger.info( | ||
| f"插件 {root_dir_name} 预加载已安装依赖失败,将继续常规导入: {preload_exc!s}" | ||
| ) | ||
|
|
||
| try: | ||
| return __import__(path, fromlist=[module_str]) | ||
| except (ModuleNotFoundError, ImportError) as import_exc: | ||
| if os.path.exists(requirements_path): | ||
| try: | ||
| logger.info( | ||
| f"插件 {root_dir_name} 导入失败,尝试从已安装依赖恢复: {import_exc!s}" | ||
| ) | ||
| pip_installer.prefer_installed_dependencies( | ||
| requirements_path=requirements_path | ||
| ) | ||
| module = __import__(path, fromlist=[module_str]) | ||
| logger.info( | ||
| f"插件 {root_dir_name} 已从 site-packages 恢复依赖,跳过重新安装。" | ||
| ) | ||
| return module | ||
| except Exception as recover_exc: | ||
| logger.info( | ||
| f"插件 {root_dir_name} 已安装依赖恢复失败,将重新安装依赖: {recover_exc!s}" | ||
| ) | ||
| if recovery_state.mode in { | ||
| ImportDependencyRecoveryMode.PRELOAD_AND_RECOVER, | ||
| ImportDependencyRecoveryMode.RECOVER_ON_FAILURE, | ||
| }: | ||
| recovered_module = self._try_import_from_installed_dependencies( | ||
| path, | ||
| module_str, | ||
| root_dir_name, | ||
| requirements_path, | ||
| import_exc, | ||
| ) | ||
| if recovered_module is not None: | ||
| return recovered_module | ||
| elif ( | ||
| recovery_state.mode is ImportDependencyRecoveryMode.REINSTALL_ON_FAILURE | ||
| ): | ||
| assert recovery_state.install_plan is not None | ||
| logger.info( | ||
| "插件 %s 预检查检测到版本不匹配,跳过已安装依赖恢复: %s", | ||
| root_dir_name, | ||
| sorted(recovery_state.install_plan.version_mismatch_names), | ||
| ) | ||
|
|
||
| await self._check_plugin_dept_update(target_plugin=root_dir_name) | ||
| return __import__(path, fromlist=[module_str]) | ||
|
|
@@ -788,6 +883,7 @@ async def load( | |
| module_str=module_str, | ||
| root_dir_name=root_dir_name, | ||
| requirements_path=requirements_path, | ||
| reserved=reserved, | ||
| ) | ||
| except Exception as e: | ||
| error_trace = traceback.format_exc() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,10 +29,17 @@ class ParsedPackageInput: | |
| requirement_names: frozenset[str] | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): Consider simplifying the design by removing the separate MissingRequirementsAnalysis type and using a single, defaulted MissingRequirementsPlan for both analysis and planning stages. You can collapse one layer without losing any functionality by reusing 1. Remove
|
||
| class MissingRequirementsAnalysis: | ||
| missing_names: frozenset[str] | ||
| version_mismatch_names: frozenset[str] = frozenset() | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class MissingRequirementsPlan: | ||
| missing_names: frozenset[str] | ||
| install_lines: tuple[str, ...] | ||
| version_mismatch_names: frozenset[str] = frozenset() | ||
| fallback_reason: str | None = None | ||
|
|
||
|
|
||
|
|
@@ -394,24 +401,39 @@ def find_missing_requirements(requirements_path: str) -> set[str] | None: | |
| def find_missing_requirements_from_lines( | ||
| requirement_lines: Sequence[str], | ||
| ) -> set[str] | None: | ||
| analysis = classify_missing_requirements_from_lines(requirement_lines) | ||
| if analysis is None: | ||
| return None | ||
|
|
||
| return set(analysis.missing_names) | ||
|
|
||
|
|
||
| def classify_missing_requirements_from_lines( | ||
| requirement_lines: Sequence[str], | ||
| ) -> MissingRequirementsAnalysis | None: | ||
| required = list(iter_requirements(lines=requirement_lines)) | ||
| if not required: | ||
| return set() | ||
| return MissingRequirementsAnalysis(missing_names=frozenset()) | ||
|
|
||
| installed = collect_installed_distribution_versions(get_requirement_check_paths()) | ||
| if installed is None: | ||
| return None | ||
|
|
||
| missing: set[str] = set() | ||
| version_mismatch_names: set[str] = set() | ||
| for name, specifier in required: | ||
| installed_version = installed.get(name) | ||
| if not installed_version: | ||
| missing.add(name) | ||
| continue | ||
| if specifier and not _specifier_contains_version(specifier, installed_version): | ||
| missing.add(name) | ||
| version_mismatch_names.add(name) | ||
|
|
||
| return missing | ||
| return MissingRequirementsAnalysis( | ||
| missing_names=frozenset(missing), | ||
| version_mismatch_names=frozenset(version_mismatch_names), | ||
| ) | ||
|
|
||
|
|
||
| def build_missing_requirements_install_lines( | ||
|
|
@@ -449,9 +471,11 @@ def plan_missing_requirements_install( | |
| if not can_precheck or requirement_lines is None: | ||
| return None | ||
|
|
||
| missing = find_missing_requirements_from_lines(requirement_lines) | ||
| if missing is None: | ||
| analysis = classify_missing_requirements_from_lines(requirement_lines) | ||
| if analysis is None: | ||
| return None | ||
| missing = analysis.missing_names | ||
| version_mismatch_names = analysis.version_mismatch_names | ||
|
|
||
| install_lines = build_missing_requirements_install_lines( | ||
| requirements_path, | ||
|
|
@@ -468,12 +492,14 @@ def plan_missing_requirements_install( | |
| ) | ||
| return MissingRequirementsPlan( | ||
| missing_names=frozenset(missing), | ||
| version_mismatch_names=frozenset(version_mismatch_names), | ||
| install_lines=(), | ||
| fallback_reason="unmapped missing requirement names", | ||
| ) | ||
|
|
||
| return MissingRequirementsPlan( | ||
| missing_names=frozenset(missing), | ||
| version_mismatch_names=frozenset(version_mismatch_names), | ||
| install_lines=install_lines, | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -60,6 +60,48 @@ def test_check_env(monkeypatch): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| check_env() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_check_env_appends_user_site_packages_after_runtime_paths(monkeypatch): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
60
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider also testing that Since the implementation checks
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| astrbot_root = "/tmp/astrbot-root" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| site_packages_path = "/tmp/astrbot-site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| original_sys_path = list(sys.path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr(sys, "version_info", _version_info(3, 12)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_root", lambda: astrbot_root) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_site_packages_path", lambda: site_packages_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_config_path", lambda: "/tmp/config") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_plugin_path", lambda: "/tmp/plugins") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_temp_path", lambda: "/tmp/temp") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_knowledge_base_path", lambda: "/tmp/kb") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr(sys, "path", ["/runtime/lib", *original_sys_path]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with mock.patch("os.makedirs"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| check_env() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert sys.path[0] == astrbot_root | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert sys.path[-1] == site_packages_path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert sys.path.index(site_packages_path) > sys.path.index("/runtime/lib") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_check_env_does_not_append_duplicate_user_site_packages(monkeypatch): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| astrbot_root = "/tmp/astrbot-root" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| site_packages_path = "/tmp/astrbot-site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| original_sys_path = list(sys.path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr(sys, "version_info", _version_info(3, 12)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_root", lambda: astrbot_root) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_site_packages_path", lambda: site_packages_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_config_path", lambda: "/tmp/config") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_plugin_path", lambda: "/tmp/plugins") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_temp_path", lambda: "/tmp/temp") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr("main.get_astrbot_knowledge_base_path", lambda: "/tmp/kb") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monkeypatch.setattr(sys, "path", [astrbot_root, *original_sys_path, site_packages_path]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with mock.patch("os.makedirs"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| check_env() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert sys.path.count(site_packages_path) == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_version_info_comparisons(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Test _version_info comparison operators with tuples and other instances.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| v3_10 = _version_info(3, 10) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider simplifying the new import-dependency recovery logic by replacing the enum/dataclass and helper with straightforward local flags and a single, linear control flow inside
_import_plugin_with_dependency_recovery.The new
ImportDependencyRecoveryMode/ImportDependencyRecoveryStateplus_try_import_from_installed_dependenciesdo add a mini state machine and extra indirection without buying much reuse. You can keep all the new behaviors (preload, version-mismatch skip, reserved handling) while simplifying control flow by:_try_import_from_installed_dependenciesinto_import_plugin_with_dependency_recovery.A possible refactor (abridged for focus) could look like:
This preserves:
install_plan.reserved/missing requirements disabling recovery entirely.And it removes:
ImportDependencyRecoveryModeImportDependencyRecoveryState_resolve_import_dependency_recovery_state_try_import_from_installed_dependenciesmaking the import-recovery flow read as a single, straightforward sequence.