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

Rewrite venv discovery logic #3134

Merged
merged 3 commits into from Nov 2, 2018

Conversation

Projects
None yet
2 participants
@uranusjr
Member

uranusjr commented Oct 31, 2018

The issue

Found an edge case in project.get_location_for_virtualenv(). If PIPENV_VENV_IN_PROJECT is true, .venv is always used as a directory, not read as a file.

To reproduce:

$ mkdir my-project
$ cd my-project
$ export PIPENV_VENV_IN_PROJECT=1
$ touch Pipenv
$ echo './.i-want-my-venv-here' > .venv
$ pipenv lock   # This will fail because Pipenv wants to use .venv as the virtual environment.

The fix

I reorganised the discovery function. Comments are added so branching is more straightforward (hopefully). The new logic works like this:

  1. Is there a project root yet? If not…
    • Use $PWD/.venv if PIPENV_VENV_IN_PROJECT is set
    • Use the $WORKON_HOME/[name]-[hash] scheme by default
  2. Does .venv exist in project root? If not, use the same logic as 1. (replacing $PWD with project root)
  3. Is .venv a directory? If so, use it directly.
  4. Read .venv as a file.
    • If the content looks like a path, resolve it as a relative path to project root to use.
    • Otherwise use the directory of that name in $WORKON_HOME.

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 #.

@uranusjr uranusjr force-pushed the venv-location-rewrite branch 2 times, most recently from a67c424 to 4988eb2 Oct 31, 2018

@uranusjr uranusjr force-pushed the venv-location-rewrite branch from 4988eb2 to 0847017 Oct 31, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Oct 31, 2018

I'm +1 on this, tests are passing on it and it looks much cleaner, I prefer not to have to think this stuff through ever again personally so i want to delete as much stuff out of the project class in the next few weeks as possible

@techalchemy

looks good to me, I'd merge it but I don't want to mess up my current PR build :p

techalchemy added some commits Nov 1, 2018

@techalchemy techalchemy merged commit d985784 into master Nov 2, 2018

0 of 2 checks passed

pipenv CI (Linux) queued
Details
pipenv CI (Windows) queued
Details

@techalchemy techalchemy deleted the venv-location-rewrite branch Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment