-
Notifications
You must be signed in to change notification settings - Fork 180
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
Use venv instead of virtualenv when available. #526
Conversation
asv/plugins/virtualenv.py
Outdated
return False | ||
if LooseVersion(match.group(1)) < LooseVersion("3.3"): | ||
self._virtualenv_argv = [ | ||
executable, "-mvirtualenv", "--no-site-packages"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should use current python, when using virtualenv?
venv is always bundled, but Virtualenv is not necessarily installed in the target Pythons...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point I was making in #523 (comment) is that venv is unable to create virtualenvs for pythons others than the current interpreter, so if we want the master python to use venv, then any other python in the test matrix either needs to be >=3.3 (i.e., have venv itself), or have virtualenv installed.
As I only work with Py3 that restriction seems acceptable to me, but I can also understand if you disagree, in which case I don't think such an approach can ever be made to work (unless builtin venv picks up the ability to create venvs for other pythons, but why whould they?). So, please let me know if I should try to fix the tests, or if you simply won't accept such a design (again, it's up to you, of course).
Also, many tests seem to break. |
What I mean:
.
- When using venv, use it as you do. It's guaranteed to be built-in on
new enough Python versions.
- When using virtualenv, the old approach is probably preferable.
|
Namely, it is not clear to me why it is necessary to the way the
*virtualenv* package is used. The *venv* package needs to be called with
the Python corresponding to the version it is used for, and that's fine
as it is built-in.
|
Edit: "... necessary to change the way the virtualenv package ..."
|
Ah, I see, your suggestion is to add a third environment type, "venv", in addition to the two existing ones ("conda" and "virtualenv"); it would only work if the entire matrix uses recent enough Pythons. Is my understanding correct? (If yes that looks reasonable to me.) |
What I mean that it would be possible to do it here like this:
```
if target_python_version >= 3.3:
call target_python -mvenv ...
else:
# Do it the way it's currently done in asv
import virtualenv
call current_python self._virtualenv_path -p target_python ...
```
Currently, in this PR it is done like this:
```
if target_python_version >= 3.3:
call target_python -mvenv ...
else:
call target_python virtualenv ...
```
|
Ah yes, thanks for the clarification. Will work on that. |
ecce716
to
ccfa886
Compare
asv/plugins/virtualenv.py
Outdated
return False | ||
if LooseVersion(match.group(1)) < LooseVersion("3.3"): | ||
self._virtualenv_argv = [ | ||
sys.executable, "-mvirtualenv", "--no-site-packages"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a classmethod, so the initialization cannot be done here.
The argument name self
is misleading though, should be cls
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if I rename "self" to "cls" everywhere in the class (which I'm happy to do) it should not change the semantics of the method (basically even now I'm just setting the attribute on the class rather than on the attribute, which should not matter).
I cannot reproduce the failure of test_run_build_failure or of test_run_spec locally (looks like pip is not getting installed for a reason I can't fathom), they pass for me locally. (test_web_regressions needs the selenium test driver which I don't have installed but given that it's essentially the same failure I don't think it's relevant).
Of course renaming self->cls does not change the semantics, but makes it
less misleading.
.
Setting the attribute on the class is wrong, because many environment
objects can exist at the same time.
.
Moreover, you are not using the correct target python here, in
particular self._executable is not used anywhere.
.
So the test failures are all real, and should be resolved.
|
To maybe unpack the logic here:
.
The Virtualenv.matches classmethod is used for autodetecting which of
the available environment classes supports a given Python version. If
the user specified the environment type, this method will not be called.
|
I apparently can't make this work and cannot even reproduce the test failures locally, so I am giving up on this attempt. |
There's a "return True" missing in the virtualenv if branch.
.
The "pip not found" issue on the other hand could be ubuntu/debian
specific. Maybe their venv does not for some reason install pip by default?
.
The python distribution platforms are quite heterogenous, so it's not so
easy to build solutions that work everywhere.
|
Also, "def matches()" should return False on failed virtualenv import,
not raise an exception.
|
Thanks for the careful suggestions. |
I think the problem is that (1) travis python is actually in a
virtualenv, and (2) if you do
```
virtualenv -p /usr/bin/python3 fooenv
source fooenv/bin/activate
python -mvenv fooenv2
```
Then the created environment doesn't get pip installed for some reason.
Virtualenv breaks venv. Maybe virtualenv bug, perhaps worth reporting.
|
Wow, nice find. It looks like this actually is a problem with ensurepip(?). venv installs pip via ensurepip while it is set up (https://github.com/python/cpython/blob/3.6/Lib/venv/__init__.py#L238), but the call to ensurepip (while nested-env is active) results in
|
Maybe can be fixed by replacing all calls to `pip` by
`run_executable(['python', '-mpip', ...])`? Needs also corresponding
change in Environment.check_presence (which maybe should be called after
also after env creation in Environment.create, to check if the env
creation was actually successful.)
|
No, pip is just not installed in the inner env (despite itself believing the contrary). |
But it seems to work?
|
So it looks like that the outer-env pip leaks into the inner env.
I guess that's good enough... |
But it seems the venv is not actually isolated from the parent
virtualenv, pip installs stuff in the parent env, so it's really just
broken :(
|
So I guess at minimum, here it would be necessary to check if the target
Python is comes from a virtualenv and if yes, never use venv. Or wait
until CPython/virtualenv fixes their stuff. Sounds messy.
|
http://bugs.python.org/issue30811 I think accepting that virtualenv is a dependency even on recent Pythons (if not using conda) is a good enough compromise for me... |
Yes, I think the simplest solution is to not use venv for the present.
The CPython release cycle probably means it will take some time until
that is fixed.
|
Note that your bug report seems inconsistent --- e.g. uses python2
-mvirtualenv without -p flag to create py3 env, and the -minspect output
shows the import paths are in inner-env
|
That was a very sloppy report, I updated it. |
No activity: closing. |
Attempt at fixing #523 as suggested by @pv.