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.

Sign up for GitHub

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

Jump to bottom

Properly handle pre-existing .venv files #2690

Closed
wants to merge 12 commits into from
Closed

Properly handle pre-existing .venv files #2690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
42de3d1
Properly handle pre-existing .venv files
MarkKoz Aug 2, 2018
d2d5aa6
Merge branch 'master' into master
techalchemy Aug 4, 2018
fed71c7
Remove old test line
techalchemy Aug 4, 2018
610f83b
fix inclusion of project name in wrong test
techalchemy Aug 5, 2018
4346d36
Merge branch 'master' into master
MarkKoz Aug 6, 2018
bab6429
switch test_venv_file_exists back to test_venv_file_with_name
MarkKoz Aug 6, 2018
93d158b
simplify venv location retrieval in tests
MarkKoz Aug 6, 2018
4844a0b
remove requests package install in tests
MarkKoz Aug 6, 2018
1fa14ea
add missing decorator for test_venv_file_with_name
MarkKoz Aug 6, 2018
e4a845f
update docstrings for tests
MarkKoz Aug 6, 2018
86f9f4f
add test for venv file with a relative path
MarkKoz Aug 6, 2018
ced489c
Merge branch 'master' into master
techalchemy Aug 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion pipenv/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,18 @@ def virtualenv_exists(self):
def get_location_for_virtualenv(self):
if self.is_venv_in_project():
return os.path.join(self.project_directory, ".venv")
return str(get_workon_home().joinpath(self.virtualenv_name))

name = self.virtualenv_name
Copy link
Member

Choose a reason for hiding this comment

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

No need to check for the presence of the project directory, the instantiation of a project in the first place depends on the project directory existing

given the current situation this can just become something like

def get_location_for_virtualenv(self):
    if self.is_venv_in_project()
        return os.path.join(self.project_directory, ".venv")
    elif os.path.exists(self.path_to('.venv')):
        venv_contents = None
        try:
            with open(self.path_to('.venv'), 'r') as fh:
                venv_contents = fh.read()
        except OSError:
            pass
        else:
            venv_name = venv_contents.strip()
            if any(sep in venv_name for sep in [os.path.sep, os.path.altsep]):
                return _normalized(venv_name)
            elif venv_name:
                return str(get_workon_home().joinpath(venv_name))
    return str(get_workon_home().joinpath(self.virtualenv_name))

Copy link
Member

Choose a reason for hiding this comment

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

thoughts @uranusjr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to check for the presence of the project directory

Wouldn't that mean the same check in is_venv_in_project is also redundant?

if self.project_directory:
venv_path = os.path.join(self.project_directory, ".venv")
if os.path.isfile(venv_path):
Copy link
Member

Choose a reason for hiding this comment

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

If you use Path(venv_path) you can use the check .exists() and not Path(venv_path).isdir() for files. You should do the same path object instantiation on the files contents to make sure the virtualenv exists.

Copy link
Contributor Author

@MarkKoz MarkKoz Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why not isdir() rather than just isfile()? Furthermore, how would using pathlib over os.path help? Seems to me like an equivalent check can be written with os.path. I like pathlib but I've used os.path just to remain consistent with the rest of the code.

You should do the same path object instantiation on the files contents to make sure the virtualenv exists.

Is this just because the path wont be created if it doesn't exist when the venv is being created? If this is the case, it should only apply to the base directory and not the venv folder itself, right? Regardless, what would my code do if it determines the path doesn't exist? Create it? Throw an error?

Copy link
Member

Choose a reason for hiding this comment

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

There are edge cases where isfile and isdir can both be false, such as device files. While it doesn’t really matter practically, it is semantically more correct to check for “not directory” because those special files are still readable. It also matches the directory check in is_venv_in_project, which makes the code easier to reason with.

I’m wondering though, it may be a better idea to merge the two checks. is_venv_in_project already checks for directory, so when you reach this part, an exists check is enough. We shouldn’t reach here if it’s a directory; if it exists, we want to try reading it (and fails with a straight-up error if that fails).

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 agree that it's better to rely on is_venv_in_project's directory check. exists should be sufficient here.

What should be done when the directory specified in a .venv file does not exist? That hasn't been addressed yet.

Copy link
Member

Choose a reason for hiding this comment

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

It would need to fail back to use self.virtualenv_name. Now you mentioned it, I realise the code is a bit tangled here and the implementation doesn’t work well. I’ll try to find some time to sort the structure out so this can progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

virtualenv just creates the directory if it doesn't exist. I consider that to be a feature, so I don't think we need to fall back to self.virtualenv_name after all.

with open(venv_path, "r") as f:
name = f.read()
# Assume file's contents is a path if it contains slashes.
if '/' in name or '\\' in name:
return name

return str(get_workon_home().joinpath(name))

def get_installed_packages(self):
from . import PIPENV_ROOT, PIPENV_VENDOR, PIPENV_PATCHED
Expand Down
65 changes: 55 additions & 10 deletions tests/integration/test_dot_venv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,17 @@ def test_reuse_previous_venv(PipenvInstance, pypi):
assert c.return_code == 0
assert normalize_drive(p.path) in p.pipenv('--venv').out


@pytest.mark.dotvenv
def test_venv_file_exists(PipenvInstance, pypi):
"""Tests virtualenv creation & package installation when a .venv file exists
at the project root.
def test_venv_file_with_name(PipenvInstance, pypi):
"""Tests virtualenv creation when a .venv file exists at the project root
and contains a venv name.
"""
with PipenvInstance(pypi=pypi, chdir=True) as p:
file_path = os.path.join(p.path, '.venv')
venv_name = 'test-project'
with open(file_path, 'w') as f:
f.write('')
f.write(venv_name)

with temp_environ(), TemporaryDirectory(
prefix='pipenv-', suffix='temp_workon_home'
Expand All @@ -58,12 +60,55 @@ def test_venv_file_exists(PipenvInstance, pypi):
if 'PIPENV_VENV_IN_PROJECT' in os.environ:
del os.environ['PIPENV_VENV_IN_PROJECT']

c = p.pipenv('install requests')
c = p.pipenv('install')
assert c.return_code == 0

venv_loc = Path(p.pipenv('--venv').out.strip())
assert venv_loc.joinpath('.project').exists()
assert Path(venv_loc.name) == Path(venv_name)


@pytest.mark.dotvenv
def test_venv_file_with_path(PipenvInstance, pypi):
"""Tests virtualenv creation when a .venv file exists at the project root
and contains an absolute path.
"""
with temp_environ(), PipenvInstance(chdir=True, pypi=pypi) as p:
with TemporaryDirectory(
prefix='pipenv-', suffix='-test_venv'
) as venv_path:
if 'PIPENV_VENV_IN_PROJECT' in os.environ:
del os.environ['PIPENV_VENV_IN_PROJECT']

file_path = os.path.join(p.path, '.venv')
with open(file_path, 'w') as f:
f.write(venv_path.name)

c = p.pipenv('install')
assert c.return_code == 0

venv_loc = None
for line in c.err.splitlines():
if line.startswith('Virtualenv location:'):
venv_loc = Path(line.split(':', 1)[-1].strip())
assert venv_loc is not None
venv_loc = Path(p.pipenv('--venv').out.strip())
assert venv_loc.joinpath('.project').exists()
assert venv_loc == Path(venv_path.name)


@pytest.mark.dotvenv
def test_venv_file_with_relative_path(PipenvInstance, pypi):
"""Tests virtualenv creation when a .venv file exists at the project root
and contains a relative path.
"""
with temp_environ(), PipenvInstance(chdir=True, pypi=pypi) as p:
if 'PIPENV_VENV_IN_PROJECT' in os.environ:
del os.environ['PIPENV_VENV_IN_PROJECT']

file_path = os.path.join(p.path, '.venv')
venv_path = 'foo/test-venv'
with open(file_path, 'w') as f:
f.write(venv_path)

c = p.pipenv('install')
assert c.return_code == 0

venv_loc = Path(p.pipenv('--venv').out.strip()).resolve()
assert venv_loc.joinpath(".project").exists()
assert venv_loc == Path(venv_path).resolve()