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

Rewrite venv discovery logic #3134

Merged
merged 3 commits into from
Nov 2, 2018
Merged

Rewrite venv discovery logic #3134

merged 3 commits into from
Nov 2, 2018

Conversation

uranusjr
Copy link
Member

@uranusjr 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 Compare October 31, 2018 01:05
@techalchemy
Copy link
Member

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

Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

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

@techalchemy techalchemy merged commit d985784 into master Nov 2, 2018
@techalchemy techalchemy deleted the venv-location-rewrite branch November 2, 2018 01:33
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