Skip to content
Merged
85 changes: 84 additions & 1 deletion astrbot/core/utils/pip_installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import importlib.util
import io
import logging
import ntpath
import os
import re
import shlex
Expand All @@ -30,6 +31,7 @@

_DISTLIB_FINDER_PATCH_ATTEMPTED = False
_SITE_PACKAGES_IMPORT_LOCK = threading.RLock()
_PIP_IN_PROCESS_ENV_LOCK = threading.RLock()
_PIP_FAILURE_PATTERNS = {
"error_prefix": re.compile(r"^\s*error:", re.IGNORECASE),
"user_requested": re.compile(r"\bthe user requested\b", re.IGNORECASE),
Expand Down Expand Up @@ -235,6 +237,83 @@ def _run_pip_main_streaming(pip_main, args: list[str]) -> tuple[int, list[str]]:
return result_code, stream.lines


def _run_pip_main_with_temporary_environ(
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
pip_main,
args: list[str],
env_updates: dict[str, str],
) -> tuple[int, list[str]]:
# os.environ is process-wide; serialize temporary mutations around the
# in-process pip invocation and keep the mutation window inside the worker.
with _PIP_IN_PROCESS_ENV_LOCK:
with _temporary_environ(env_updates):
return _run_pip_main_streaming(pip_main, args)


def _normalize_windows_native_build_path(path: str) -> str:
normalized = path.replace("/", "\\")

for prefix in ("\\\\?\\UNC\\", "\\??\\UNC\\"):
if normalized.startswith(prefix):
return ntpath.normpath(f"\\\\{normalized[len(prefix) :]}")

for prefix in ("\\\\?\\", "\\??\\"):
if normalized.startswith(prefix):
normalized = normalized[len(prefix) :]
break

return ntpath.normpath(normalized)


def _prepend_windows_env_path(name: str, path: str) -> str:
existing = os.environ.get(name)
if existing:
return f"{path};{existing}"
return path


def _build_packaged_windows_runtime_build_env() -> dict[str, str]:
if sys.platform != "win32" or not is_packaged_desktop_runtime():
return {}

runtime_executable = _normalize_windows_native_build_path(sys.executable)
runtime_dir = ntpath.dirname(runtime_executable)
if not runtime_dir:
return {}

env_updates: dict[str, str] = {}
include_dir = _normalize_windows_native_build_path(
ntpath.join(runtime_dir, "include")
)
libs_dir = _normalize_windows_native_build_path(ntpath.join(runtime_dir, "libs"))

if os.path.isdir(include_dir):
env_updates["INCLUDE"] = _prepend_windows_env_path("INCLUDE", include_dir)
if os.path.isdir(libs_dir):
env_updates["LIB"] = _prepend_windows_env_path("LIB", libs_dir)

return env_updates


@contextlib.contextmanager
def _temporary_environ(updates: dict[str, str]):
if not updates:
yield
return

missing = object()
previous_values = {key: os.environ.get(key, missing) for key in updates}

try:
os.environ.update(updates)
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Outdated
yield
finally:
for key, previous_value in previous_values.items():
if previous_value is missing:
os.environ.pop(key, None)
else:
os.environ[key] = previous_value


def _matches_pip_failure_pattern(line: str, *pattern_names: str) -> bool:
names = pattern_names or tuple(_PIP_FAILURE_PATTERNS)
return any(_PIP_FAILURE_PATTERNS[name].search(line) for name in names)
Expand Down Expand Up @@ -927,11 +1006,15 @@ def prefer_installed_dependencies(self, requirements_path: str) -> None:
async def _run_pip_in_process(self, args: list[str]) -> int:
pip_main = _get_pip_main()
_patch_distlib_finder_for_frozen_runtime()
build_env = _build_packaged_windows_runtime_build_env()

original_handlers = list(logging.getLogger().handlers)
try:
result_code, output_lines = await asyncio.to_thread(
_run_pip_main_streaming, pip_main, args
_run_pip_main_with_temporary_environ,
pip_main,
args,
build_env,
)
finally:
_cleanup_added_root_handlers(original_handlers)
Expand Down
199 changes: 199 additions & 0 deletions tests/test_pip_installer.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import asyncio
import ntpath
import threading
from unittest.mock import AsyncMock

Expand All @@ -9,6 +10,15 @@
from astrbot.core.utils import requirements_utils
from astrbot.core.utils.pip_installer import PipInstaller

WINDOWS_RUNTIME_ROOT = ntpath.join(r"C:\astrbot-test", "backend", "python")
WINDOWS_RUNTIME_EXECUTABLE = ntpath.join(WINDOWS_RUNTIME_ROOT, "python.exe")
WINDOWS_PACKAGED_RUNTIME_EXECUTABLE = f"\\\\?\\{WINDOWS_RUNTIME_EXECUTABLE}"
WINDOWS_RUNTIME_INCLUDE_DIR = ntpath.join(WINDOWS_RUNTIME_ROOT, "include")
WINDOWS_RUNTIME_LIBS_DIR = ntpath.join(WINDOWS_RUNTIME_ROOT, "libs")
EXISTING_WINDOWS_INCLUDE_DIR = ntpath.join(r"C:\toolchain", "include")
EXISTING_WINDOWS_LIB_DIR = ntpath.join(r"C:\toolchain", "lib")
_ENV_MISSING = object()


def _make_run_pip_mock(
code: int = 0,
Expand All @@ -24,6 +34,56 @@ async def run_pip(*args, **kwargs):
return AsyncMock(side_effect=run_pip)


def _configure_run_pip_in_process_capture(
monkeypatch,
*,
platform: str,
packaged_runtime: bool,
runtime_executable: str = WINDOWS_PACKAGED_RUNTIME_EXECUTABLE,
include_value: str | object = _ENV_MISSING,
lib_value: str | object = _ENV_MISSING,
existing_runtime_dirs: set[str] | None = None,
) -> dict[str, str | None]:
observed_env: dict[str, str | None] = {}

def fake_pip_main(args):
del args
observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
return 0

if packaged_runtime:
monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
else:
monkeypatch.delenv("ASTRBOT_DESKTOP_CLIENT", raising=False)

if include_value is _ENV_MISSING:
monkeypatch.delenv("INCLUDE", raising=False)
else:
monkeypatch.setenv("INCLUDE", include_value)

if lib_value is _ENV_MISSING:
monkeypatch.delenv("LIB", raising=False)
else:
monkeypatch.setenv("LIB", lib_value)

monkeypatch.setattr(pip_installer_module.sys, "platform", platform)
monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)

if existing_runtime_dirs is not None:
monkeypatch.setattr(
pip_installer_module.os.path,
"isdir",
lambda path: path in existing_runtime_dirs,
)

monkeypatch.setattr(
"astrbot.core.utils.pip_installer._get_pip_main",
lambda: fake_pip_main,
)
return observed_env


@pytest.mark.asyncio
async def test_install_targets_site_packages_for_desktop_client(monkeypatch, tmp_path):
monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -226,6 +286,145 @@ def fake_pip_main(args):
]


@pytest.mark.parametrize(
("path", "expected"),
[
(
WINDOWS_PACKAGED_RUNTIME_EXECUTABLE,
WINDOWS_RUNTIME_EXECUTABLE,
),
(
r"\\?\UNC\server\share\include",
r"\\server\share\include",
Comment on lines +297 to +314
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding normalization tests for already-normalized and \\?\C:\... style non-UNC paths.

These tests already cover UNC and forward‑slash paths. To better exercise _normalize_windows_native_build_path, please also add:

  • An input that is already normalized (e.g. WINDOWS_RUNTIME_EXECUTABLE) to verify it’s returned unchanged.
  • A non‑UNC extended path like \\?\C:\... or \??\C:\... to cover the branch that strips extended‑length prefixes without going through the UNC logic.

This will ensure both prefix‑handling branches and the no-op path are properly tested and guard against regressions in Windows extended path handling.

),
(
r"\??\UNC\server\share\libs",
r"\\server\share\libs",
),
(
"C:/astrbot-test/backend/python/libs",
WINDOWS_RUNTIME_LIBS_DIR,
),
Comment on lines +316 to +327
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding a case for already-normal UNC paths in _normalize_windows_native_build_path

Since this test already covers extended prefixes and forward-slash normalization, consider adding a case with an already-normal UNC path (e.g. r"\\server\share\include") to verify _normalize_windows_native_build_path is a no-op in that scenario and to guard against future double-normalization or incorrect prefixing.

Suggested change
(
r"\??\UNC\server\share\libs",
r"\\server\share\libs",
),
(
"C:/astrbot-test/backend/python/libs",
WINDOWS_RUNTIME_LIBS_DIR,
),
(
r"\??\UNC\server\share\libs",
r"\\server\share\libs",
),
# Already-normal UNC path should be a no-op
(
r"\\server\share\include",
r"\\server\share\include",
),
(
"C:/astrbot-test/backend/python/libs",
WINDOWS_RUNTIME_LIBS_DIR,
),

],
)
def test_normalize_windows_native_build_path_variants(path, expected):
assert pip_installer_module._normalize_windows_native_build_path(path) == expected


@pytest.mark.asyncio
@pytest.mark.parametrize(
("include_exists", "libs_exists"),
[
(True, True),
(True, False),
(False, True),
],
)
async def test_run_pip_in_process_injects_windows_runtime_build_env(
monkeypatch, include_exists, libs_exists
):
existing_runtime_dirs = set()
if include_exists:
existing_runtime_dirs.add(WINDOWS_RUNTIME_INCLUDE_DIR)
if libs_exists:
existing_runtime_dirs.add(WINDOWS_RUNTIME_LIBS_DIR)

observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="win32",
packaged_runtime=True,
include_value=EXISTING_WINDOWS_INCLUDE_DIR,
lib_value=EXISTING_WINDOWS_LIB_DIR,
existing_runtime_dirs=existing_runtime_dirs,
)

installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])

assert result == 0
expected_include = EXISTING_WINDOWS_INCLUDE_DIR
expected_lib = EXISTING_WINDOWS_LIB_DIR
if include_exists:
expected_include = (
f"{WINDOWS_RUNTIME_INCLUDE_DIR};{EXISTING_WINDOWS_INCLUDE_DIR}"
)
if libs_exists:
expected_lib = f"{WINDOWS_RUNTIME_LIBS_DIR};{EXISTING_WINDOWS_LIB_DIR}"
assert observed_env == {"INCLUDE": expected_include, "LIB": expected_lib}
assert pip_installer_module.os.environ["INCLUDE"] == EXISTING_WINDOWS_INCLUDE_DIR
assert pip_installer_module.os.environ["LIB"] == EXISTING_WINDOWS_LIB_DIR
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a variant where INCLUDE/LIB are initially unset and runtime dirs are missing

This already covers the case where runtime dirs are missing but existing INCLUDE/LIB values are present. Please also add a variant where INCLUDE and LIB are absent from os.environ and existing_runtime_dirs is an empty set, and assert that the in-process pip run does not create these keys. That will exercise the "unset env + missing runtime dirs" scenario.

Suggested implementation:

WINDOWS_RUNTIME_LIBS_DIR = ntpath.join(WINDOWS_RUNTIME_ROOT, "libs")
EXISTING_WINDOWS_INCLUDE_DIR = ntpath.join(r"C:\toolchain", "include")
EXISTING_WINDOWS_LIB_DIR = ntpath.join(r"C:\toolchain", "lib")


@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_create_env_when_unset_and_runtime_dirs_missing(
    configure_run_pip_in_process_capture,
):
    observed_env = configure_run_pip_in_process_capture(
        platform="win32",
        packaged_runtime=True,
        include_value=None,
        lib_value=None,
        existing_runtime_dirs=set(),
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0
    assert "INCLUDE" not in observed_env
    assert "LIB" not in observed_env
    assert "INCLUDE" not in pip_installer_module.os.environ
    assert "LIB" not in pip_installer_module.os.environ

This assumes configure_run_pip_in_process_capture treats include_value=None and lib_value=None as “do not set these env vars initially”. If in your fixture the absence is represented differently (e.g. by omitting the arguments or using a sentinel), adjust the call accordingly. Also, this test relies on pip_installer_module already being imported/available as used in the existing tests.



@pytest.mark.asyncio
async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
monkeypatch,
):
observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="win32",
packaged_runtime=True,
existing_runtime_dirs={
WINDOWS_RUNTIME_INCLUDE_DIR,
WINDOWS_RUNTIME_LIBS_DIR,
},
)

installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])

assert result == 0
assert observed_env == {
"INCLUDE": WINDOWS_RUNTIME_INCLUDE_DIR,
"LIB": WINDOWS_RUNTIME_LIBS_DIR,
}
assert ";" not in observed_env["INCLUDE"]
assert ";" not in observed_env["LIB"]
assert "INCLUDE" not in pip_installer_module.os.environ
assert "LIB" not in pip_installer_module.os.environ
Comment on lines +462 to +499
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider a test where only one of the runtime include/lib directories exists and no existing INCLUDE/LIB env vars are set.

The existing tests cover: (1) both runtime dirs present with no pre-existing INCLUDE/LIB, (2) combinations of include_exists/libs_exists when INCLUDE/LIB are already set, and (3) the case where both runtime dirs are missing. What’s missing is when only one of include/libs exists and both env vars are unset. Extending this test parametrically (or adding a small new one) to assert that only the existing dir is injected and that no trailing ; is added would close this gap and better guard against subtle string-formatting issues in _build_packaged_windows_runtime_build_env.

Suggested change
@pytest.mark.asyncio
async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
monkeypatch,
):
observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="win32",
packaged_runtime=True,
existing_runtime_dirs={
WINDOWS_RUNTIME_INCLUDE_DIR,
WINDOWS_RUNTIME_LIBS_DIR,
},
)
installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])
assert result == 0
assert observed_env == {
"INCLUDE": WINDOWS_RUNTIME_INCLUDE_DIR,
"LIB": WINDOWS_RUNTIME_LIBS_DIR,
}
assert ";" not in observed_env["INCLUDE"]
assert ";" not in observed_env["LIB"]
assert "INCLUDE" not in pip_installer_module.os.environ
assert "LIB" not in pip_installer_module.os.environ
@pytest.mark.asyncio
@pytest.mark.parametrize(
"include_exists, libs_exists",
[
(True, True), # both runtime dirs present
(True, False), # only include dir present
(False, True), # only libs dir present
],
)
async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
monkeypatch,
include_exists,
libs_exists,
):
existing_runtime_dirs = set()
if include_exists:
existing_runtime_dirs.add(WINDOWS_RUNTIME_INCLUDE_DIR)
if libs_exists:
existing_runtime_dirs.add(WINDOWS_RUNTIME_LIBS_DIR)
observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="win32",
packaged_runtime=True,
existing_runtime_dirs=existing_runtime_dirs,
)
installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])
assert result == 0
expected_env = {}
if include_exists:
expected_env["INCLUDE"] = WINDOWS_RUNTIME_INCLUDE_DIR
if libs_exists:
expected_env["LIB"] = WINDOWS_RUNTIME_LIBS_DIR
assert observed_env == expected_env
if include_exists:
assert ";" not in observed_env["INCLUDE"]
if libs_exists:
assert ";" not in observed_env["LIB"]
# Ensure process env is not polluted
assert "INCLUDE" not in pip_installer_module.os.environ
assert "LIB" not in pip_installer_module.os.environ



@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_modify_env_on_non_windows(monkeypatch):
existing_include = "/toolchain/include"
existing_lib = "/toolchain/lib"
observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="linux",
packaged_runtime=True,
include_value=existing_include,
lib_value=existing_lib,
)

installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])

assert result == 0
assert observed_env == {"INCLUDE": existing_include, "LIB": existing_lib}
assert pip_installer_module.os.environ["INCLUDE"] == existing_include
assert pip_installer_module.os.environ["LIB"] == existing_lib


@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_inject_env_when_not_packaged(monkeypatch):
observed_env = _configure_run_pip_in_process_capture(
monkeypatch,
platform="win32",
packaged_runtime=False,
existing_runtime_dirs={
WINDOWS_RUNTIME_INCLUDE_DIR,
WINDOWS_RUNTIME_LIBS_DIR,
},
)

installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])

assert result == 0
assert observed_env == {"INCLUDE": None, "LIB": None}
assert "INCLUDE" not in pip_installer_module.os.environ
assert "LIB" not in pip_installer_module.os.environ


@pytest.mark.asyncio
async def test_run_pip_in_process_classifies_nonstandard_conflict_output(monkeypatch):
def fake_pip_main(args):
Expand Down
Loading