Properly handle pre-existing .venv files #2690

MarkKoz · 2018-08-02T08:17:05Z

Fixes #2680 as discussed in the issue with bonus support for custom paths in the .venv file. Not sure what to categorise this under for the news fragment so I will wait for someone's advice on that before writing up the news fragment.

Would like reviewers to especially look at the test cases written. Could the repeated code be moved elsewhere? Are path comparisons properly handled? - concerns are accounting for symlinks and case-insensitivity on Windows.

uranusjr · 2018-08-02T08:34:43Z

Hi, thanks for the work! I would recommend splitting this into two PRs instead—we are very close to feature freeze for a new version, and while the exists → isdir change can easily get in (as a bugfix), the .venv file detection would require additional reviews.

MarkKoz · 2018-08-02T08:47:06Z

Sure. How do you recommend I split the PR? Create two new ones or try to salvage this PR by rewriting history? Will have to work on it tomorrow either way.

uranusjr · 2018-08-02T08:51:12Z

Either would do, whatever is more convenient for you :)

techalchemy

If you include the relevant check it keeps the control flow logic the same I think, so if @uranusjr is ok with that I also would be

techalchemy · 2018-08-02T14:20:44Z

pipenv/project.py

+        name = self.virtualenv_name
+        if self.project_directory:
+            venv_path = os.path.join(self.project_directory, ".venv")
+            if os.path.isfile(venv_path):


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.

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?

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

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.

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.

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.

MarkKoz · 2018-08-02T17:56:10Z

Should a test case still be written for the exists → isdir bugfix when I split the PRs? If so, that test case will probably need to be removed by this PR since the two test cases I've already written will make the other one redundant.

uranusjr · 2018-08-03T03:29:08Z

It would be best to have a test for the isdir change, but redundant tests are not a problem.

MarkKoz · 2018-08-03T04:04:22Z

Just rewriting history to split the PRs. Will try prioritising the bugfix PR and then come back to this PR once I've finished the former.

techalchemy · 2018-08-04T23:15:54Z

pipenv/project.py

@@ -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


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))

thoughts @uranusjr ?

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?

MarkKoz · 2018-08-05T04:36:44Z

tests/integration/test_dot_venv.py

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


The merge should have kept test_venv_file_with_name over test_venv_file_exists (or kept both). We do want to write something to the file so we can test if the written name is used to find/create a venv of the same name in workon home.

hmmmmm I think I prioritized your test over the one he wrote, but that wasn't really intentional, I was just trying to triage and get everything moving so I can get a release out soon(tm). There is a test right below that one though and it does write a name of a directory to the file which is why I undid my inclusion of it here

The merge removed this project name in favor of creating a temporary directory in the test below it and using that directory's location as the path in the ,venv file.

The difference between the two tests was one wrote a path to the file, the other just a name. If you think that the former adequately covers the latter case too then all is fine as is.

I think those should be merged into one with pytest fixtures. The fixture would be something like

@pytest.mark.fixtures('content, venv_location', [ ('just-a-name', Path(WORKON_HOME, 'just-a-name')), ('/an/absolute/path', Path('/an/absolute/path')), ('a/relative/path', Path(project_root, 'a/relative/path')), ])

(I added a third test case. If a relative path is specified, the venv should be created relative to the project’s root.)

Good call, but does it really need to be a fixture if we'll only have one test that uses it? Could just use @pytest.mark.parametrize.

I'm not sure how WORKON_HOME and project_root be passed. My first thought was fixtures, but pytest currently doesn't support the use of fixtures in @pytest.mark.parametrize. Would it work if it was left a fixture, and fixtures were used for the fixture's params?

Ah yes, excellent call, I was totally thinking about parametrize, not fixtures!

I don’t think we need to refactor tests to merge this PR but the point about a path vs a name is valid. You can’t parametrize a temporary directory as an input though so that would still need a separate test

Yeah, I played around with this idea of parametrisation but couldn't come up with any elegant solution - it'd create a bigger mess than it is intended to prevent.

Do you mind the messy commit history? I could just commit on top of what we have to make the test write a name again, or I could just rewrite history and re-do the merge.

I usually squash the commits a bit before merging, so make it as messy as you need to :p

We're only interested in testing if the virtual environment can be successfully created.

MarkKoz · 2018-08-06T20:30:29Z

I don't know what's causing the build to fail on Buildkite - I can't see the results. Everything is fine on my end, other than test_environment_variable_value_does_not_change_hash which has failed from the beginning because it tries to access anaconda2 (isn't installed on my machine).

techalchemy · 2018-08-10T08:30:41Z

the build is failing for unclear reasons relating to a tlsv1 error now on buildkite, which is basically unusable I guess. I'll turn travis back on tomorrow if I can't work it out

sorry for the unreasonably long delay on this, we have been kind of sequestered putting together a bunch of critical elements of a refactor of the resolver

MarkKoz · 2018-08-10T21:35:14Z

It's fine; priorities are priorities. Don't worry I'm not going anywhere. Before continuing, I just wanted to make sure my changes weren't the cause of the build failure.

techalchemy · 2018-08-20T01:38:02Z

I really can't tell what's going on here but it's failing to connect to the local package index due to an SSL issue? this is the only branch / test suite that fails on buildkite and i really don't know why...

MarkKoz · 2018-08-20T01:56:59Z

Is it failing to connect while running one of my tests or at some other point? The major changes to the tests were changing how the venv location is retrieved, doing install instead of install requests, and of course adding a third test case. Perhaps these changes could be reverted one by one to narrow down the search for the offending code, if any.

However, I don't see anything obviously wrong with the tests and they do all pass on my machine. Maybe some underlying issue has been unearthed?

What's the course of action? I'm not sure how I can help here.

techalchemy · 2018-08-20T02:45:40Z

I don't see anything wrong either, let me check your branch out locally and see if I have any issues... I'm super confused about this one at the moment and I'm wondering if there is some unrelated ssl issue on the build machine

The tests look fine, and the connection that's failing is to localhost... I'll show you whenever buildkite's frontend is willing to let me copy things

techalchemy · 2018-08-21T07:02:43Z

=================================== FAILURES ===================================
_____________________________ test_mirror_install ______________________________
[gw0] linux2 -- Python 2.7.15 /root/.local/share/virtualenvs/pipenv-R0iOVi4H-2.7/bin/python2.7
 
PipenvInstance = <class 'tests.integration.conftest._PipenvInstance'>
pypi = <pytest_pypi.serve.Server object at 0x7fb6dd8198d0>
 
    @pytest.mark.install
    @flaky
    def test_mirror_install(PipenvInstance, pypi):
        with temp_environ(), PipenvInstance(chdir=True) as p:
            mirror_url = os.environ.pop(
                "PIPENV_TEST_INDEX", "https://pypi.python.org/simple"
            )
            assert "pypi.org" not in mirror_url
            # This should sufficiently demonstrate the mirror functionality
            # since pypi.org is the default when PIPENV_TEST_INDEX is unset.
            c = p.pipenv("install requests --pypi-mirror {0}".format(mirror_url))
>           assert c.return_code == 0
E           AssertionError: assert 1 == 0
E            +  where 1 = <Command 'pipenv install requests --pypi-mirror https://127.0.0.1:42425/simple'>.return_code
 
tests/integration/test_install_basic.py:55: AssertionError
----------------------------- Captured stdout call -----------------------------
pytest-httpbin server hit an exception serving request: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:726)
attempting to ignore so the rest of the tests can run
pytest-httpbin server hit an exception serving request: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:726)
attempting to ignore so the rest of the tests can run
pytest-httpbin server hit an exception serving request: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:726)
$ pipenv install requests --pypi-mirror https://127.0.0.1:42425/simple
Installing requests…
Looking in indexes: https://127.0.0.1:42425/simple
Collecting requests
  Could not fetch URL https://127.0.0.1:42425/simple/requests/: There was a problem confirming the ssl certificate: HTTPSConnectionPool(host='127.0.0.1', port=42425): Max retries exceeded with url: /simple/requests/ (Caused by SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)) - skipping
 
Creating a virtualenv for this project…
Pipfile: /tmp/pipenv-lkEOTs-project/Pipfile
Using /root/.local/share/virtualenvs/pipenv-R0iOVi4H-2.7/bin/python2.7 (2.7.15) to create virtualenv…
Already using interpreter /root/.local/share/virtualenvs/pipenv-R0iOVi4H-2.7/bin/python2.7
Using real prefix '/usr'
New python executable in /tmp/pipenv-lkEOTs-project/.venv/bin/python2.7
Also creating executable in /tmp/pipenv-lkEOTs-project/.venv/bin/python
Installing setuptools, pip, wheel...done.
 
Virtualenv location: /tmp/pipenv-lkEOTs-project/.venv
Creating a Pipfile for this project…
Error:  An error occurred while installing requests!
  Retrying (Retry(total=4, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)': /simple/requests/
  Retrying (Retry(total=3, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)': /simple/requests/
  Retrying (Retry(total=2, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)': /simple/requests/
  Retrying (Retry(total=1, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)': /simple/requests/
  Retrying (Retry(total=0, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),)': /simple/requests/
  Could not find a version that satisfies the requirement requests (from versions: )
No matching distribution found for requests

MarkKoz · 2018-08-22T03:26:01Z

Yeah, I also have no idea. Double checked that test and it indeed passes on my machine. Did you manage to test it locally. If so, how did that go?

techalchemy · 2018-08-22T07:24:08Z

sigh. I think I have this sorted out -- checked out your branch and worked from there... it has absolutely nothing to do with any of these changes and I have no idea how this is passing in master

techalchemy requested changes Aug 2, 2018

View reviewed changes

Properly handle pre-existing .venv files

42de3d1

Merge branch 'master' into master

d2d5aa6

techalchemy reviewed Aug 4, 2018

View reviewed changes

techalchemy added 2 commits August 4, 2018 19:33

Remove old test line

fed71c7

fix inclusion of project name in wrong test

610f83b

MarkKoz commented Aug 5, 2018

View reviewed changes

MarkKoz and others added 7 commits August 6, 2018 12:26

Merge branch 'master' into master

4346d36

switch test_venv_file_exists back to test_venv_file_with_name

bab6429

simplify venv location retrieval in tests

93d158b

remove requests package install in tests

4844a0b

We're only interested in testing if the virtual environment can be successfully created.

add missing decorator for test_venv_file_with_name

1fa14ea

update docstrings for tests

e4a845f

add test for venv file with a relative path

86f9f4f

Merge branch 'master' into master

ced489c

techalchemy mentioned this pull request Aug 24, 2018

Venv Test Fixes #2773

Merged

techalchemy closed this in #2773 Aug 24, 2018

Properly handle pre-existing .venv files #2690

Properly handle pre-existing .venv files #2690

Conversation

MarkKoz commented Aug 2, 2018

uranusjr commented Aug 2, 2018

MarkKoz commented Aug 2, 2018 • edited Loading

uranusjr commented Aug 2, 2018

techalchemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz commented Aug 2, 2018

uranusjr commented Aug 3, 2018

MarkKoz commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz commented Aug 6, 2018

techalchemy commented Aug 10, 2018 • edited Loading

MarkKoz commented Aug 10, 2018

techalchemy commented Aug 20, 2018

MarkKoz commented Aug 20, 2018

techalchemy commented Aug 20, 2018

techalchemy commented Aug 21, 2018

MarkKoz commented Aug 22, 2018

techalchemy commented Aug 22, 2018

MarkKoz commented Aug 2, 2018 •

edited

Loading

MarkKoz Aug 2, 2018 •

edited

Loading

MarkKoz Aug 5, 2018 •

edited

Loading

MarkKoz Aug 6, 2018 •

edited

Loading

MarkKoz Aug 6, 2018 •

edited

Loading

techalchemy commented Aug 10, 2018 •

edited

Loading