Fix bug where custom virtualenv can not be activated by pipenv shell

[jxltom]

The issue

Closes #3339. This is because in PIPENV_ACTIVE is checked in project's virtualenv_location which is introduced in https://github.com/pypa/pipenv/pull/3243/files#diff-7c0c15ee4eba8c237e3318a395816a13R430. So setting up PIPENV_ACTIVE in do_shell has to be after finishing reading virtualenv_location which is a dynamic property.

The checklist

• Associated issue
• A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

[frostming]

Seems similar to #3299 , /cc @uranusjr

[jxltom]

Actually they are different. #3299 is a behavior change but this one is merely fixing a bug.

It looks like the CI for this PR is stuck. 😢

[techalchemy]

CI is set to batch run so it sometimes takes awhile to start while it waits to see if you will push more changes

[techalchemy]

I think we can make a test for this, I’ll see before we merge

[jxltom]

That's what I'm thinking. It is better to have tests for this. But it looks like pipenv shell is not supported by our tests which I tried but failed as some kind of tty console stuff. I didn't find any tests on pipenv shell in tests actually. @techalchemy Any suggestions for implementing this test? Thanks

[techalchemy]

Hm I didn’t get a chance to try this yet but I’m guessing we need a fake tty wrapper to have it play nice. Possibly this is controlled via SESSION_IS_INTERACTIVE. Really we just need to open a subprocess that has sys.stdout and sys.stderr set to TextIOWrapper subclasses (so we can use some context manager to swap these in and back out) as long as the subclass provides isatty and fileno. Click has some useful implementations that might work, from click._compat import get_text_stdout_stream, get_text_stderr_stream

[jxltom]

@techalchemy Thanks for sharing! I'm going to have look. Free free to contribute to it if you already have some solutions.

[frostming]

I have a different approach to solving this:

Detect whether we are in a virtualenv with is_in_virtualenv and then do not fork subshell or activate if yes, showing a message instead.

[jxltom]

I've updated the PR fixing another bug in pipen run related with this. This PR defnitely requires more tests.

[jxltom]

@frostming What do you mean by showing a message? If that means show a warning or error message instead of re-activating it, I think this limits the usage of pipenv.

For example, probably I'm already in an activated virtualenv by source bin/activate, pipenv shell should still work since pipenv can load env vars from .env while virtualenv can not. This is very useful in some cases.

Another case is that even virtualenv is activated, pipenv run should work anyway.

[techalchemy]

To be honest I’m confused. If you’re in a virtualenv you made and activated yourself, pipenv should operate automatically as though you passes --system all the time except it will give a courtesy notice that you’re in a virtualenv that pipenv doesn’t own

The only time we should be trying to look for pipenv-owned virtualenvs is if PIPENV_IGNORE_VIRTUALENVS is set.

[jxltom]

Yes, with this PR, pipenv wil use activated virtualenv automatically when pipenv run|shell. Without this PR, pipenv will create pipenv-owned virtualenv even it is already in a activated virtualenv.

This is for reproduce for pipenv run:

• virtualenv abc
• cd abc && source bin/activate
• pipenv run which pip

This is for reproduce for pipenv shell:

• virtualenv abc
• cd abc && source bin/activate
• pipenv shell

[frostming]

I came up with an approach to testing against the subshell:

c = p.pipenv("shell", block=False)
c.send("python -c 'import sys; print(sys.executable)'")
c.send("exit")
c.block()
# check output
assert c.out ...

@jxltom Please tell me if it works. test_project.py: test_run_in_virtualenv should be a good place to put the tests.

[jxltom]

Thanks! @frostming , I'm going to have a look.

[frostming]

Maybe you can write a contextmanager to hide the openning/ending bits.

[frostming]

Ugh.. it doesn't work for posix..

[jxltom]

@frostming We may need emulating a tty for testing this

[techalchemy]

At one time I had a full test for this exact situation

[techalchemy]

This isn't broken for me locally...

[techalchemy]

the correct behavior is to use the activated virtualenv unless PIPENV_IGNORE_VIRTUALENVS is set. If you go to a directory with no Pipfile, pipenv currently creates a pipfile first and then tells you info from the activated virtualenv rather than create a new one.

Seems like it works?

  ³ tempenv-6c963194107a0  ~/g/pythonfinder   feature/add-typechecking     pipenv --venv
Courtesy Notice: Pipenv found itself running within a virtual environment, so it will automatically use that environment, instead of creating its own for any project. You can set PIPENV_IGNORE_VIRTUALENVS=1 to force pipenv to ignore that environment and create its own instead. You can set PIPENV_VERBOSITY=-1 to suppress this warning.
/home/hawk/.virtualenvs/tempenv-6c963194107a0
  ³ tempenv-6c963194107a0  ~/g/pythonfinder   feature/add-typechecking     pipenv run which python
/home/hawk/.virtualenvs/pythonfinder-AjR26NJ2/bin/python
  ³ tempenv-6c963194107a0  ~/g/pythonfinder   feature/add-typechecking     cd /tmp/test
  ³ tempenv-6c963194107a0  /t/test  pipenv run which python
Creating a Pipfile for this project…
/home/hawk/.virtualenvs/tempenv-6c963194107a0/bin/python

[jxltom]

Sounds like it is working for you @techalchemy

Did you try this? It should output yourpath/abc/bin/pip instead of /home/yourusername/.local/share/virtualenvs/abc-HoOUeXpF/bin/pip

• virtualenv abc
• cd abc && source bin/activate
• pipenv run which pip

[jxltom]

Update, for reproducing this, you have to create a pipenv-owned virtualenv first...

• virtualenv abc && cd abc
• pipenv --three
• source bin/activate
• pipenv run which pip

Finally it will output /home/yourusername/.local/share/virtualenvs/abc-HoOUeXpF/bin/pip but it should be yourpath/abc/bin/pip

[techalchemy]

so did we conclude here that things are mostly working OK?

[jxltom]

Yeah, it still a bug but only happens when shell/run is excecuted within activated virtualenv (which is not owned by pipenv) while pipenv-owned virtualenv already exists.

[techalchemy]

and the bug is that they don't use pipenv's environment? or what is the bug

[jxltom]

The bug is that pipenv run|shell is not using environment created and activated by virtualenv, but using pipenv-owned environment (only happens when it exist).

Please check this #3340 (comment) for reproducing.

[jxltom]

Update: I've added test for this PR. As for the tests for pipenv shell, I think we can do that in another separate PR.

[frostming]

@jxltom For tests, you should create a pipenv project outside of the virtualenv, which may need to rewrite the virtualenv fixture to a context manager so that it won't be activated at the beginning of test function.

[jxltom]

@frostming Yeah, but this bug can also be reproduced within an activated virtualenv at the beginning only if PIPENV_IGNORE_VIRTUALENVS is set.

I can confirm this test can not pass without this PR's patch, so it is working anyway...

[techalchemy]

Thanks for handling this bizarre edge case, definitely a confusing one but a lot more clear with a failing test

[techalchemy]

scratch this comment, we just had a bug with warning on run, we are all set on this