fix(venv): bulletproof virtual env handling#3437
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens developer/CI workflows by standardizing how scripts and Make targets detect and enforce the expected Python virtual environment (.venv), aiming to prevent running commands in the wrong (or missing) venv and to provide consistent error messages.
Changes:
- Introduces shared venv helpers (
is_venv_active,assert_correct_venv) and adopts them across multiple scripts. - Adds Makefile guard targets to check for venv existence/activation and wires them into common targets; improves
clean-allsafety around active venv removal. - Adds venv “must be inactive” checks to runner scripts and makes minor shell-robustness tweaks (
${VAR:-}patterns, awk regex).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/setup_test_env.sh |
Small parameter expansion tweaks for set -u friendliness. |
scripts/setup_dev_venv.sh |
Tightens repo-root check and uses shared venv helpers for consistent behavior. |
scripts/reinstall_editable.sh |
Replaces inline venv checks with shared helper and asserts correct venv. |
scripts/deploy_doc.sh |
Uses shared venv assertion and introduces TOP_DIR resolution. |
scripts/common.sh |
Adds shared venv detection/assertion helpers with canonical-path comparison. |
runner/regression.sh |
Refuses to run when any venv is active (uses shared helper). |
runner/node_upgrade.sh |
Refuses to run when any venv is active (uses shared helper). |
Makefile |
Adds venv guard targets, updates target prerequisites, safer clean-all, and awk regex tweak. |
.source.dev |
Validates activated venv is the repo’s .venv using canonicalized path comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mkoura
force-pushed
the
bulletproof_venv_handling
branch
2 times, most recently
from
b675bab to
e884753
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mkoura
force-pushed
the
bulletproof_venv_handling
branch
2 times, most recently
from
8dc59e4 to
e11c104
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mkoura
force-pushed
the
bulletproof_venv_handling
branch
from
e11c104 to
8e0ef2b
Compare
Detect activated/missing/wrong virtual envs across the Make targets
and shell scripts, with shared helpers and consistent error messages.
- Makefile: add .check-venv-exists and .check-venv-activated guard
targets, wire them into cluster-scripts, start/stop-cluster,
init-lint, lint, fmt, build-doc. cluster-scripts now relies on the
activated venv's PATH instead of hardcoded $(VENV)/bin/. clean-all
no longer takes 'clean' as prereq so the venv check runs first; it
refuses to wipe the currently-activated venv (canonical-path
compare). Help-target awk regex switched from .*? to .* for POSIX
awk compatibility. .check-venv-activated overrides SHELL via
'command -v bash' so it can source the bash-only
scripts/common.sh on systems without /bin/bash (NixOS).
- scripts/common.sh: add is_venv_active and assert_correct_venv
helpers (canonical-path compare, descriptive error messages).
- scripts/deploy_doc.sh, scripts/reinstall_editable.sh,
scripts/setup_dev_venv.sh: replace duplicated inline checks with
assert_correct_venv / is_venv_active.
- scripts/setup_dev_venv.sh: also tighten the repo-root sanity check
(require cardano_node_tests/tests, not just cardano_node_tests) and
reorganize the prompt/sync flow.
- runner/regression.sh, runner/node_upgrade.sh: refuse to run inside
any active virtual env via is_venv_active.
- scripts/setup_test_env.sh: cosmetic ${VAR:-""} -> ${VAR:-}.
mkoura
force-pushed
the
bulletproof_venv_handling
branch
from
8e0ef2b to
e89afa2
Compare
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
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.
Detect activated/missing/wrong virtual envs across the Make targets and shell scripts, with shared helpers and consistent error messages.