Fix race condition when installing 2+ editable non-VCS pkgs at once #3237
Conversation
| @@ -8,6 +8,7 @@ | |||
| import pytest | |||
|
|
|||
| from flaky import flaky | |||
| import delegator | |||
There was a problem hiding this comment.
I guess this line can be removed?
There was a problem hiding this comment.
Ah, good point. That was left over from an earlier iteration. I'll fix that.
|
|
||
|
|
||
| @pytest.mark.files | ||
| @pytest.mark.needs_internet |
There was a problem hiding this comment.
Does it really need internet? You are using the mocked pypi test server in this case.
There was a problem hiding this comment.
No, you're right it does not in the final iteration. Will fix that.
| """ | ||
| pkgs = { | ||
| "requests-2.19.1": "requests/requests-2.19.1.tar.gz", | ||
| "flask-0.12.2": "flask/flask-0.12.2.tar.gz", |
There was a problem hiding this comment.
| "flask-0.12.2": "flask/flask-0.12.2.tar.gz", | |
| "flask-0.12.2": "flask/Flask-0.12.2.tar.gz", |
There was a problem hiding this comment.
Thanks, updated in new commit!
| "requests-2.19.1": "requests/requests-2.19.1.tar.gz", | ||
| "flask-0.12.2": "flask/flask-0.12.2.tar.gz", | ||
| "six-1.11.0": "six/six-1.11.0.tar.gz", | ||
| "jinja2-2.10": "jinja2/jinja2-2.10.tar.gz", |
There was a problem hiding this comment.
| "jinja2-2.10": "jinja2/jinja2-2.10.tar.gz", | |
| "jinja2-2.10": "jinja2/Jinja2-2.10.tar.gz", |
There was a problem hiding this comment.
Thanks, updated in new commit!
christek91
force-pushed
the
master
branch
3 times, most recently
from
e39b05f
to
7da5551
Compare
This regression was recently introduced, and only affects non-vcs packages. The effect of the race is that installations of multiple editable non-VCS sourced packages at once may cause some of them to be un-importable. The fix is to make editable package installs Blocking just like VCS installs are.
christek91
force-pushed
the
master
branch
2 times, most recently
from
81a22cc
to
699de3a
Compare
| pipfile_string += "'{0}' = {{path = '{1}', editable = true}}\n".format(pkg_name, unzip_path) | ||
|
|
||
| with PipenvInstance(pypi=pypi, chdir=True) as p: | ||
| print(pipfile_string) |
There was a problem hiding this comment.
Please remove the debugging prints.
|
once the debug line is gone im good to merge this, i didn't think about adding editable as a conditional there but i'm glad you were able to find that. Hopefully the code cleanup efforts will make contributing easier |
This regression was recently introduced, and only affects non-vcs
packages. The effect of the race is that installations of multiple
editable non-VCS sourced packages at once may cause some of them to be
un-importable.
The fix is to make editable package installs Blocking just like VCS
installs are.
Thank you for contributing to Pipenv!
The issue
This fixes a race condition that is hit when installing multiple local editable packages at once. There was a regression because
editableinstallations used to be blocking, but a change in the recent release changed it so that only VCS installations are blocking.While most VCS installations are probably
editable, not alleditable` installations are VCS ones. So that change inadvertently brought back #919.I've also created a new Issue for the new regression as the PR template suggests. It is: #3236
The fix
My fix makes sure that all
editableinstallations are blocking, regardless of if they are VCS installs or not.The test I wrote for this is a little janky to setup since I couldn't use git to install the test packages since VCS installations don't hit the race. I put a comment in the code explaining the context.
The checklist
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 #.