Skip to content
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

Fix another unbound variable error #424

Merged
merged 2 commits into from
Jan 30, 2022

Conversation

ackalker
Copy link
Contributor

@ackalker ackalker commented Jan 30, 2022

This fixes the case of bash: PS1: unbound variable when PYENV_VIRTUALENV_DISABLE_PROMPT isn't set.
This should have been part of #423, sorry for that. I also fixed the tests related to virtualenv activation/deactivation which were failing due to my changes.

This should have been part of the fix for pyenv#423, but I missed it because
I had PYENV_VIRTUALENV_DISABLE_PROMPT=1 set when doing my own testing.
@@ -34,15 +34,15 @@ unset CONDA_PREFIX
unset PYENV_VIRTUAL_ENV;
unset VIRTUAL_ENV;
unset CONDA_DEFAULT_ENV;
if [ -n "\${_OLD_VIRTUAL_PATH}" ]; then
if [ -n "\${_OLD_VIRTUAL_PATH:-}" ]; then
Copy link
Member

@native-api native-api Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC Bats runs its own instance of Bash so your main shell's settings shouldn't affect it.

So I doubt that this part and similar ones below and in other files are necessary.

Copy link
Contributor Author

@ackalker ackalker Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for the misunderstanding. I discovered the problem by running my own tests, outside of Bats. You can reproduce the problem easily by running this test script on a tree without my latest commits applied, i.e. current master:

test-pyenv-set-u.bash

#!/usr/bin/bash

set -u

eval "$(pyenv init -)"

# Uncomment either of these blocks for tracing

# export PYENV_DEBUG=1

# -or-

export PS4='+(${BASH_SOURCE:-<none>}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }'
set -x

pyenv virtualenv system testenv
echo created
pyenv activate testenv
echo activated
pyenv deactivate
echo deactivated
pyenv virtualenv-delete testenv
echo deleted

set +x

Output of running this on my system, for both methods of tracing is in this gist.
Note the problem is in these two lines: https://gist.github.com/ackalker/4de61234e8579fe96cd3a4c7da90da3b#file-test-pyenv-set-u-ps4-log-L46-L47 , which are in a snippet which is evaled, which doesn't show up in the trace with PYENV_DEBUG=1. Note that PS1 is unbound while running scripts,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not putting the validity of the problem into question. I'm only questioning if this particular part of the changes is really necessary to fix it.

The other changes change the output of sh-activate and tests that check for that output. But these changes to the tests, I do not see why they are necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that I understand the confusion now. The changes to sh-deactivate and sh-activate should have been in one commit, the changes to the tests address changes to both files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gist of running bats test, failing (tests-failing.log) at commit 8f721ac , and passing (tests-passing.log) after applying bc4c73a .
The remaining failing tests are unrelated to my fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying!

@native-api native-api merged commit 017ea60 into pyenv:master Jan 30, 2022
@ackalker ackalker deleted the fix-virtualenv-activate-set-u branch January 30, 2022 18:56
@scop scop mentioned this pull request Oct 11, 2022
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