Virtualenv: activate may fails due to unbound variables when run with set -eu

[ssbarnea]

It seems that an old bug is still here: PS1: unbound variable when called with bash strict mode.

Clearly virtualenv needs a test using bash strict mode and an almost empty set of environment variables, so we avoid future regressions on this.

Please note that this bug does reproduce with ANY supported Unix shell, not only bash, including ksh and zsh.

Here is a simple way to replicate the bug:

#!/bin/bash
# same applies to any other bourne compatible shells (is not bash specific)
set -euox pipefail
pip install -U virtualenv
virtualenv xxx
unset PS1
source xxx/bin/activate

The workaround, while ugly, is to do prepend PS=${PS:-} to the activate line, which defines PS as empty string when is not already defined, or keeps its value when is defined.

The same kind of bug applies to Python version of venv and there is an already open PR to fix it there too.

Please do not postpone/delay implementing a fix just because the same kind of bug exists in other places. Note that the default expansion variable syntax is POSIX compatible and not something new or fancy, the fact that this was not known to the original author should not be an excuse for not using it.

[johnomics]

I am also finding this, running virtualenv 15.1.0. I'm using an environment within a Nextflow pipeline, and Nextflow runs in strict mode by default (https://github.com/nextflow-io/nextflow/issues/302). While Nextflow can be reconfigured to run without strict mode, I agree with the Nextflow developer that it would be preferable to avoid using unbound variables, if possible.

I'm not sure exactly how the activate script is created, but if it comes from activate.sh, then the fix might be as simple as changing $PS1 on lines 57, 59 and 61 to ${PS1-}. (This syntax will use the value of PS1 if available, and the empty string otherwise. It doesn't change the value of PS1. Documentation). At least, if I modify the generated activate script in my environment like this, the error message goes away.

[johnomics]

https://github.com/pypa/virtualenv/issues/991

[ssbarnea]

I am wondering how many years it would take to learn how to write bash code.... the unbound variable bugs in virtualenv are eons old and so easy to fix and to also AVOID them in the future by just adding a simple line to the virtual tests: set -euox pipefail.

Not to mention how many years it will take for the fix to reach all virtualenv distros around there as that's usually packaged on debian, ubuntu, centos, rhel, fedora, .... :( :( :(

[jakub-bochenski]

will the project maintainers even acknowledge this issue exists?

[ssbarnea]

I don't know but considering that we also got a PR which is almost two weeks old. The most likely answer is that they don't give a... submit.

[ssbarnea]

I am going to try to make some noise on irc, twitter, maybe even mailing list. Maybe we can get the fixes merged.

virtualenv is the only thing that prevent me from making the bash strict mode default on CI jobs.

[jakub-bochenski]

+1

I think just disabling nounset at the beginning of the script and restoring it at the end might be more robust instead of trying to teach python devs how to bash :)

[ssbarnea]

@jakub-bochenski maybe you can also help with some comments on irc. Let's see if we can get enough users to wake a virtualenv core dev.

[jakub-bochenski]

@ssbarnea not sure about that, I haven't logged in to IRC for ages. I can try to help generate buzz on the mailing list though

[axd1967]

sigh...

+1

[ssbarnea]

I emailed pypa-dev 7 days ago and didn't get any reply. Also yesterday someone posted that the installed win32 binary contains a trojan, again no reply. I only hope that the trojan was not really into the distro, not that it would affect me.

See https://groups.google.com/forum/#!forum/pypa-dev

[jpuskar]

Ran into this today, figured it was a bug in my code somewhere.
:+1: to including set -euo pipefail in the unit tests.

[jakub-bochenski]

for reference the direct link to discussion mentioned above is: https://groups.google.com/d/topic/pypa-dev/8iVHDOqsj9M/discussion

@pfmoore wrote

I've already responded in much more detail on the virtualenv-users list,

.. which turned out to be the python-virtualenv list; https://groups.google.com/d/topic/python-virtualenv/5xKG8KoBl6g/discussion

[pjeby]

FWIW, the workaround I'm using in .devkit is to set VIRTUAL_ENV_DISABLE_PROMPT=true on the source line. It works better for my use case than setting PS1, because it disables the prompt-setting behavior altogether.

[ssbarnea]

@pjeby @jakub-bochenski @jpuskar @axd1967 Please note that we already have a bugfix for this but in order to merge it we need to review and merge two other PRs, that's only because we do want and need to improve the activate test-suite.

1. https://github.com/pypa/virtualenv/pull/1089 -- enable CI testing on py36 and dropping py33
2. https://github.com/pypa/virtualenv/pull/1087 -- enable use of test_activate.sh script on CI
3. https://github.com/pypa/virtualenv/pull/1078 -- unbound PS1 fix into activate

Probably you will see that the last two are not passing CI tests, that's why we need the others merged first.

Please review/comment on them, it matters more than on other project because virtualenv lacks some reviewing power, this being one of the reasons why I asked to become a maintainer at https://groups.google.com/d/msg/pypa-dev/SgK9vlu93BY/F2_8OoKAAgAJ -- even so, it seems that we will need more than one as I would not be able to review my own changes.

[jakub-bochenski]

even so, it seems that we will need more than one as I would not be able to review my own changes.

@ssbarnea are you asking us to also get reviewer permissions, or just do a review and leave a +1/comments?

EDIT: never mind apparently anyone can review a PR

[jakub-bochenski]

1 done 2 more to go :)

[duaneking]

Can we please get an ETA on when this will be merged and publicly available?

Edit: Still getting this issue, and it just brought down a build this morning.

[robinbowes]

I've also been bitten by this, when working on the build script for an AWS lambda-based image processing solution: https://github.com/awslabs/serverless-image-handler/blob/master/deployment/build-s3-dist.sh

I've used the VIRTUAL_ENV_DISABLE_PROMPT=true source env/bin/activate workaround.

[ssbarnea]

@duaneking @robinbowes There is a lack of maintenance power around virtualenv and if you want to help addressing this issue please read and comment on https://groups.google.com/forum/#!topic/pypa-dev/SgK9vlu93BY

My impression is that PYPA team will react on this only if it gets enough community feedback.

[jakub-bochenski]

FTR we are still waiting for a merge on #1087

[jakub-bochenski]

Guess what is the first example for Use the Unofficial Bash Strict Mode | Sourcing A Nonconforming Document?

Yes, it's python 2 virtualenv.

[axd1967]

Ubuntu 16.04

• works not: VIRTUAL_ENV_DISABLE_PROMPT=true
• works: https://github.com/bogdando/tripleo-ci/commit/318d17a9e2d94723aa1a0d8e2a9ee345e67032ae

[jakub-bochenski]

Note that using bogdando/tripleo-ci@318d17a will override the mode to -u even if it was not active before. Not exactly a best practice construct.

This will preserve the previous state:

old_setting=${-//[^u]/}
...
if [[ -n "$old_setting" ]]; then set -u; fi

[adudek]

right now I suggest using patch (use bash or change accordingly for your needs)
it will start failing (breaking run) once authors finally manage to post this fix, giving you clear indication of change being made

set +H -euo pipefail
pushd "${envdir}"
patch -p0 <<< '
--- bin/activate 2018-10-12 09:08:16.991113929 +0200
+++ bin/activate 2018-10-12 09:27:51.505054528 +0200
@@ -54,11 +54,11 @@
 fi

 if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT-}" ] ; then
-    _OLD_VIRTUAL_PS1="$PS1"
+    _OLD_VIRTUAL_PS1="${PS1:-}"
     if [ "x" != x ] ; then
         PS1="$PS1"
     else
-        PS1="(`basename \"$VIRTUAL_ENV\"`) $PS1"
+        PS1="(`basename \"$VIRTUAL_ENV\"`) ${PS1:-}"
     fi
     export PS1
 fi
'
popd
. "${envdir}/bin/activate"

[allan-simon]

fixed now https://github.com/pypa/virtualenv/pull/922