Skip to content

fix(cli): prevent host python path being used when running uv commands - #178

Merged
ryanking13 merged 2 commits into
mainfrom
gyeongjae/fix-venv-override
Aug 4, 2026
Merged

ryanking13 merged 2 commits into
mainfrom
gyeongjae/fix-venv-override

Conversation

@ryanking13

Copy link
Copy Markdown
Contributor

pywrangler sync previously selected the Pyodide environment through VIRTUAL_ENV. However, the VIRTUAL_ENV env variable can be ignored or overridden by uv's internal interpreter discovery logic. This PR fixes it by explicitly passing --python flag to the uv commands, which is the most dominant flag.

This fixes a problem that some test were failing in CI in old Python versions + hopefully fixes the CI failure in #177

@ask-bonk

ask-bonk Bot commented Aug 3, 2026

Copy link
Copy Markdown

There are remaining VIRTUAL_ENV usages in test_cli.py (lines 240 and 254), but those are in test code that directly calls subprocess.run rather than going through the run_command helper. These are separate from the production code being fixed and could be subject to the same issue, but they're in integration tests where the context is different (they construct the environment explicitly). This is a minor observation but not a blocking concern for this PR.

Let me now verify the uv documentation claim about --python being the most dominant flag:

The changes are clean and correct. Let me summarize my review:

  1. All four uv pip call sites in sync.py (_install_requirements_to_vendor, _install_requirements_to_venv, _log_installed_packages, _get_vendor_package_versions) are consistently updated from env=os.environ | {"VIRTUAL_ENV": ...} to --python <path>.

  2. The os import is still needed for os.name checks on lines 79 and 182.

  3. The test fix in test_version_sync.py correctly adds return_value=Path("pyodide-venv") to the get_pyodide_venv_path mock (since the path is now embedded in the command rather than an env var) and updates the assertion to match the new command structure.

  4. The xfail removal in test_in_workerd.py is appropriate since the root cause (host Python being used due to VIRTUAL_ENV being overridden) is fixed by this PR.

  5. No leftover VIRTUAL_ENV env overrides remain in the production code.

The only minor observation is that test_cli.py still uses the VIRTUAL_ENV env var pattern in its subprocess.run calls (lines 240 and 254), which could theoretically suffer from the same issue. However, those are integration test helpers calling uv pip freeze directly, not going through run_command, and the test environment is more controlled -- so this is not blocking.

LGTM

github run

@ryanking13
ryanking13 requested review from abstractedfox, dom96, hoodmane and joesepi and removed request for dom96 August 3, 2026 10:47
@ryanking13
ryanking13 merged commit 789c29e into main Aug 4, 2026
17 checks passed
@ryanking13
ryanking13 deleted the gyeongjae/fix-venv-override branch August 4, 2026 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants