-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: set packaged Windows runtime build env for pip native builds #6575
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 1 commit
cacb1e7
ad50649
ef674a0
e98a054
1927924
d6301ca
9e30112
18df33d
9f1eb53
9e44ba1
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 |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import importlib.util | ||
| import io | ||
| import logging | ||
| import ntpath | ||
| import os | ||
| import re | ||
| import shlex | ||
|
|
@@ -235,6 +236,71 @@ def _run_pip_main_streaming(pip_main, args: list[str]) -> tuple[int, list[str]]: | |
| return result_code, stream.lines | ||
|
|
||
|
|
||
| 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) | ||
| 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) | ||
|
|
@@ -927,12 +993,14 @@ 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 | ||
| ) | ||
| with _temporary_environ(build_env): | ||
| result_code, output_lines = await asyncio.to_thread( | ||
| _run_pip_main_streaming, pip_main, args | ||
| ) | ||
|
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. Modifying the global To make this operation safe, you should serialize the execution of this critical section. I recommend adding an For example: # In PipInstaller.__init__:
self._in_process_lock = asyncio.Lock()
# In _run_pip_in_process:
async def _run_pip_in_process(self, args: list[str]) -> int:
async with self._in_process_lock:
# ... the rest of the method's logicThis will ensure that concurrent calls do not interfere with each other's environment. |
||
| finally: | ||
| _cleanup_added_root_handlers(original_handlers) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,50 @@ def fake_pip_main(args): | |
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_run_pip_in_process_injects_windows_runtime_build_env(monkeypatch): | ||
|
sourcery-ai[bot] marked this conversation as resolved.
Outdated
|
||
| runtime_executable = ( | ||
| r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe" | ||
| ) | ||
| include_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include" | ||
| libs_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs" | ||
|
sourcery-ai[bot] marked this conversation as resolved.
Outdated
|
||
| existing_include = r"C:\VS\include" | ||
| existing_lib = r"C:\VS\lib" | ||
| observed_env = {} | ||
|
|
||
| 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 | ||
|
|
||
| monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1") | ||
| monkeypatch.setenv("INCLUDE", existing_include) | ||
| monkeypatch.setenv("LIB", existing_lib) | ||
| monkeypatch.setattr(pip_installer_module.sys, "platform", "win32") | ||
| monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable) | ||
|
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): Add tests for non-packaged or non-Windows runtimes to ensure no env injection occurs Specifically, consider tests where:
In those cases, assert that the env passed to Suggested implementation: existing_include = r"C:\VS\include"
existing_lib = r"C:\VS\lib"
observed_env = {}
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
monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
monkeypatch.setenv("INCLUDE", existing_include)
monkeypatch.setenv("LIB", existing_lib)
monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
monkeypatch.setattr(
pip_installer_module.os.path,
"isdir",
lambda path: path in {include_dir, libs_dir},
)
monkeypatch.setattr(
"astrbot.core.utils.pip_installer._get_pip_main",
lambda: fake_pip_main,
)
installer = PipInstaller("")
result = await installer._run_pip_in_process(["install", "demo-package"])
# New tests: ensure no env injection on non-Windows and non-packaged runtimes.
async def test_run_pip_in_process_does_not_modify_env_on_non_windows(
monkeypatch, pip_installer_module, runtime_executable
):
existing_include = "/usr/local/include"
existing_lib = "/usr/local/lib"
observed_env = {}
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
# Even if the desktop flag is set, a non-Windows platform should prevent injection
monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
monkeypatch.setenv("INCLUDE", existing_include)
monkeypatch.setenv("LIB", existing_lib)
monkeypatch.setattr(pip_installer_module.sys, "platform", "linux")
monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
monkeypatch.setattr(
"astrbot.core.utils.pip_installer._get_pip_main",
lambda: fake_pip_main,
)
installer = PipInstaller("")
await installer._run_pip_in_process(["install", "demo-package"])
# INCLUDE/LIB should be untouched for non-Windows platforms
assert observed_env["INCLUDE"] == existing_include
assert observed_env["LIB"] == existing_lib
async def test_run_pip_in_process_does_not_inject_env_when_not_packaged(
monkeypatch, pip_installer_module, runtime_executable
):
observed_env = {}
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
# Simulate non-packaged runtime: no ASTRBOT_DESKTOP_CLIENT flag
monkeypatch.delenv("ASTRBOT_DESKTOP_CLIENT", raising=False)
monkeypatch.delenv("INCLUDE", raising=False)
monkeypatch.delenv("LIB", raising=False)
monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
monkeypatch.setattr(
"astrbot.core.utils.pip_installer._get_pip_main",
lambda: fake_pip_main,
)
installer = PipInstaller("")
await installer._run_pip_in_process(["install", "demo-package"])
# For non-packaged runtimes, INCLUDE/LIB should not be injected at all
assert observed_env["INCLUDE"] is None
assert observed_env["LIB"] is NoneThe new tests I added assume:
If fixture or import names differ in your codebase, adjust the parameter names of the new tests and the imports accordingly. Also ensure the new async test functions are at module scope (no extra indentation) alongside the existing |
||
| monkeypatch.setattr( | ||
| pip_installer_module.os.path, | ||
| "isdir", | ||
| lambda path: path in {include_dir, libs_dir}, | ||
| ) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.utils.pip_installer._get_pip_main", | ||
| lambda: fake_pip_main, | ||
| ) | ||
|
|
||
| installer = PipInstaller("") | ||
| result = await installer._run_pip_in_process(["install", "demo-package"]) | ||
|
|
||
| assert result == 0 | ||
| assert observed_env == { | ||
| "INCLUDE": f"{include_dir};{existing_include}", | ||
| "LIB": f"{libs_dir};{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_classifies_nonstandard_conflict_output(monkeypatch): | ||
| def fake_pip_main(args): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.