-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support built-in “venv” #173
base: master
Are you sure you want to change the base?
Conversation
Oh man so |
Finally, only one to go! |
I think I’ve got it. Tests on Appveyor and Travis are run inside a virtualenv (through Tox in Appveyor’s case), and virtualenv does not create entry points if you create a venv inside it. To reproduce:
You can see |
So it seems the nested virtual environment thing is a known problem, and I’ve found enough information on BPO to solve it. Now if only I can make the Windows build pass… |
So it is impossible for |
Need a newer Virtualenv for 3.6, and newer Pytest for 3.5+, so the requirements.txt are modified to allow newer versions. Path of tmpdir on macOS is a symlink and need to be resolved explicitly to prevent the check from failing. Some tests are beefed up to help pinpoint the problem more accurately.
So we can use it in the venv backend implementation.
The venv backend is used by default when "venv" and "ensurepip" are available (i.e. Python 3.4+). This can be disabled with the PEW_USE_VIRTUALENV environ. Certain tests are skipped: @skip_venv Test makes assumptions that do not fit venv's structure. The functionality is implemented, but the check is logically incompatible with how venv does things. @skip_venv_site_packages Same as above, but specifically this has something to do with venv's central configuration file, and can be fixed by implementing a parallel test. @skip_venv_cp This uses the "pew cp" command, which is not implemented yet. I will try to eliminate them if possible.
Alright so I think this is ready for review. In summary, this patch adds support for creating and managing virtual environments with the built-in
A Tests are mostly unchanged, but I cleaned up some of them to make my debugging easier, and I think they are beneficial to the codebase. A few tests are marked as skipped because they rely to much on virtualenv internals, and I feel it is too difficult to rewrite them to work with both virtualenv and venv. I have no idea why the Nix build is failing. I didn’t change anything about it (I think), and lack knowledge on its internals to know why it breaks. |
So Windows is broken only with venv nested inside virtualenv? Not when using venv alone? I’m not sure whether |
virtualenv can’t handle nesting either, but is smart enough to find the “real” Python when creating the inner virtualenv, avoiding nesting altogether:
You can see from the last command’s output, it uses the original Python prefix, not the virtualenv one. venv, on the other hand, does not do this “real Python” lookup, and would happily use the inner Python, causing the issue. But this reminds me, how does virtualenv do this anyway? I haven’t thought about this. Maybe I should go check its source and see if it can provide a workaround. |
Aha! virtualenv patches Update: Alas, |
I was thinking about the possibility to disable virtualenv in Travis CI or AppVeyor? After all, nested venv is not a typical use case… |
So I implemented a way to “guess” where |
The only failing test is in Nix, where system denied access to |
This provide a more hard-nose effort to find a real Python in all places possible. Should work if you install Python from the standard CPython distribution. If not, I don't know what will.
# usually good enough, unless we're in a virtualenv (i.e. sys.real_prefix | ||
# is set), and sysconfig reports the wrong Python (i.e. in the virtualenv). | ||
import sysconfig | ||
bindir = sysconfig.get_config_var('BINDIR') |
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.
I think we should make a Path
out of bindir
straight away
Wow, thanks for the all the work on this.
edit: Nevermind, I see that you already propose it to the user when failing There's only a bit that I wouldn't approve straight away and that I'll have to bikeshed a bit: the Anyhow, I'll have a quick look at the Nix failure soon |
Ok, the Nix failure is one of those:
When venv creates a virtualenv, all of the activate scripts have permission The old virtualenv tool instead creates the permissions as I also reproduced this manually on Macos, by creating a venv in This was a bit frustrating to debug, since running instead the test inside Nix locally didn't trigger this (somehow, the permissions for the activate scripts are Anyhow, this isn't a spurious error due to environment/configuration, so it should be fixed... either upstream in virtualenv-clone, or by ensuring that such activate scripts have write permission before attempting a copy of the virtualenv. |
tests/utils.py
Outdated
|
||
import pytest | ||
|
||
skip_windows = functools.partial(pytest.mark.skipif, platform == 'win32') | ||
|
||
|
||
def use_venv(): | ||
return version_info >= (3, 4) and not os.environ.get('PEW_USE_VIRTUALENV') |
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.
I'd place this in pew._utils
, and reuse it in pew._venv
Wow, that is intense debugging. Well done! Since the Anyway, I agree this should be fixed. I’ll probably work on this (and other minor issues you mentioned) this weekend. Thanks for the effort and review! |
Actually, it's simpler than it relying on The mode is copied from the source file, so if the source activate.csh has been installed with read-only permissions, the virtualenv scripts will also be installed with read-only permissions. I don't think it's a good idea for venv to do this. |
Add the write permission to all activate scripts before copying the venv if needed, and restore all modified files after cloning. Why this is needed: #173 (comment)
this is super problematic, in my experimentations. |
Another argument to use venv: it seems to fix the issue where using |
Just to add some more links to this issue, this appears to be the |
I went down the rabbit hole of trying to get pipenv working with venv so it wouldn't break autocomplete in my Python interpreter...I ended up here. Trying to further my understanding of Python, development workflow, etc. has there been any progress in this space or should I read up more and try to make a meaningful contribution? |
@DougPlumley No progress. Contribution welcome of course. |
(Try to) fix #67.
Essentially this only adds a new way to create venvs, that calls
python -m venv
instead ofpython -m virtualenv
. It is used by default, unless either 1.venv
is not available, or 2. thePEW_NO_BUILTIN_VENV
PEW_USE_VIRTUALENV
environment variable is not empty.I played around this a little and everything seems to work; the two modules are just too similar. I still pull the logic into their own classes, just in case something acts differently and requires additional abstraction.
Some others things that might need considering:
venv
is completely silent when creating a virtualenv.virtualenv
is pretty verbose in comparison. Maybe it is better to add some additional logging?As mentioned in Start supporting pyvenv #67,I’ve decided to ignore that. Just use virtualenv ifensurepip
still has some issues. While the Debian problem is largely gone, the--system-site-package
one is not. It is possible to patch this manually in pew, but is it worth it?ensurepip
is not available.There is no knownIt seems to work perfectly.virtualenv-clone
equivalent forvenv
, so there’s nopew cp
forvenv
. I’ve skipped related tests (all failing), but an alternative is needed.