- Notifications
You must be signed in to change notification settings - Fork 275
[Python] Change .venv script to be more compatible with IDEs #2259
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
Conversation
| Question: Do we care about preserving a user's pre-existing |
Preserve it. |
| @@ -1,8 +1,92 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why get rid of the shebang?
plugins/pip/venvShellHook.sh Outdated
| #!/bin/sh | ||
| set -eu | ||
| STATE_FILE="$DEVBOX_PROJECT_ROOT/.devbox/venv_check_completed" | ||
| echo $STATE_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this left in from debugging?
plugins/pip/venvShellHook.sh Outdated
| fi | ||
| | ||
| # Check Python version | ||
| if ! check_python_version; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we just checking the version to see if venv is available? If so, it might be easier to just try importing it.
| if ! check_python_version; then | |
| if ! python -c "import venv" &>/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a much nicer approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to tweak this because for some reason it was failing the automated tests (but not when run locally)
plugins/pip/venvShellHook.sh Outdated
| echo | ||
| if [[ $REPLY =~ ^[Yy]$ ]]; then | ||
| echo "Overwriting existing virtual environment..." | ||
| rm -rf "$VENV_DIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use python3 -m venv --clear to overwrite.
plugins/pip/venvShellHook.sh Outdated
| } | ||
| | ||
| # Function to check if Python is a symlink to a Devbox Python | ||
| is_devbox_python() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the test -ef flag to check if two files are the same. So you could do:
if [ "$1/bin/python" -ef "$DEVBOX_PACKAGES_DIR/bin/python" ]; then| | ||
| create_venv() { | ||
| python -m venv "$VENV_DIR" --clear | ||
| echo "*\n.*" >> "$VENV_DIR/.gitignore" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
| if ! python -c 'import venv' 1> /dev/null 2> /dev/null; then | ||
| echo "\033[1;33mWARNING: Python version must be > 3.3 to create a virtual environment.\033[0m" | ||
| touch "$STATE_FILE" | ||
| exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work after the user has updated their python version?
i.e. by creating the state-file, will it skip running again to create the venv after upgrading python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should, because the python in the venv is a symlink to the python in our Devbox profile, which is a symlink to the nix store. I'll test a few times to make sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, once the .venv is created, updating python automatically updates the python linked in the .venv folder
Summary
Moves
.venvto the Devbox Project Root, and adds some checks to ensure we warn the user if we are going to squash a user created venv. Moving.venvto the project root makes it more likely that Python Extensions and IDEs will pick up the devbox managed python, instead of a system pythonFixes DEV-105
How was it tested?