fix: resolve Windows venv interpreters in setup-image-video.sh (#4200) - #4287
Merged
Conversation
The 6 pre-creation guards each re-encoded the same POSIX/Windows layout probe that venv_python() already owns. Factor it into venv_exists() so future layout changes are a one-place edit instead of seven, and extend the test to cover the new helper plus a guard against future call sites bypassing it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bin/python3), so on Windows — where a venv created via Git Bash puts the interpreter atScripts/python.exe— the pip install steps failed against a nonexistent path and aborted the whole installer underset -euo pipefail.venv_python()helper (resolves the interpreter for either layout) and avenv_exists()companion (checks either layout is present), and applies both consistently across all 6 venv call sites in the script (MiniMax H3 CUDA, AudioLDM2, ACE-Step, MiniMax Music 3, MuScriptor, FLUX.2) — replacing three different ad-hoc probing idioms that had grown up independently, two of which never checked for the Windows layout at all.Test plan
bash -n scripts/setup-image-video.sh— syntax check passes.cd server && NODE_ENV=test npx vitest run ../scripts/setup-image-video.test.js— 13/13 tests pass, covering both helpers directly (POSIX/Windows/missing-venv resolution) plus a structural per-site assertion and a negative regression guard that fails if a future call site bypasses the shared helpers.servertest suite run (unrelated pre-existing timeout flakes under load, none touching this change).