-
-
Notifications
You must be signed in to change notification settings - Fork 523
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 builtin venv module for Python 3 #630
Conversation
Codecov Report
@@ Coverage Diff @@
## master #630 +/- ##
==========================================
+ Coverage 90.77% 90.79% +0.01%
==========================================
Files 11 11
Lines 2373 2389 +16
Branches 402 404 +2
==========================================
+ Hits 2154 2169 +15
Misses 144 144
- Partials 75 76 +1
Continue to review full report at Codecov.
|
Hi @rpkilby, thanks for the PR. It would have been better to open an issue about this first to discuss how and when this can be done. tox has a commitment to support the same python versions as pip, so as long as pip does not drop the support for 2.6 we won't. For pip this will happen in version 10 and @dstufft just wrote a few day ago that there is no ETA for pip 10. I also seem to remember that virtualenv -> venv replacement was already tried last summer, when I joined the project. It had to be reverted, because it broke too many things (I did not have a clue back then, so I might muddle things up). What I am pretty sure about is that this is not a trivial change and that it might break a lot of stuff out there, so this needs careful planning and testing and would have to be part of a major release. |
Hi @obestwalter. This has previously been discussed in #436, with the groundwork for this PR laid out in this comment. That said, I didn't receive relevant feedback after that, so had been waiting for python 3.3's EOL to resubmit this as a more complete PR. Given that most of the work was already done in #438, more work went into writing the PR's description than actually updating the code 😄. In short, it seemed easier to go ahead and open a new PR and have the discussion here, instead of creating both an issue and a PR.
Gotcha - didn't realize this was goal. Is there a reason behind this? As stated, dropping 2.6 isn't necessary for the PR and can be easily reverted. That said, I'm not in a huge rush to see this merged - waiting for pip 10 to be released seems reasonable.
Could you point me to the relevant commits/PR? I've tried looking for relevant PRs/issues, but searching for "venv" in this repo isn't exactly helpful 😄. In general, I would like to push this PR forward, even if it won't be merged for a while. Do you have any specific guidance on what can be done to improve the PR in the meantime? |
By the way, here's the pip installs for tox from PyPI for the last month:
|
I would assume the count for 2.7 is inflated, as a lot of CI setups will use 2.7 as the base version to run tox from. Interesting data though - I hadn't heard about the pypinfo project. |
9d311d6
to
b16e325
Compare
tox/venv.py
Outdated
""" | ||
args = [str(python), '-c', 'import sys; print(sys.real_prefix)'] | ||
|
||
process = subprocess.Popen(args, stdout=subprocess.PIPE) |
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've opted not to use action.popen()
here, given that it causes a lot of tests to fail (test are expecting a single popen call, but this would create a second call, so the count assertions fail).
and if so, use it as the base path to determine the real python | ||
executable path. | ||
""" | ||
args = [str(python), '-c', 'import sys; print(sys.real_prefix)'] |
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.
sys.real_prefix
is only set by virtualenv. If we are not presently in a virtualenv, this attr doesn't exist and the command will fail.
So this took a lot more effort than expected, but I think the underlying issues are mostly solved. I plan on cleaning up the commit history later, as it's a little messy, but for now it details how the current solution came about. As to the actual CI failures, the underlying issue was that it's apparently not possible to create a python 3 venv while working inside an active virtualenv (I've submitted more details in pypa/virtualenv/issues/1095). For whatever reason, the installation is faulty, and it's effectively an... alias(?) to the parent virtualenv (eg, installing a package will end up placing it in the virtualenv's bin/lib, not the venv's bin/lib). This problem surfaces in Travis in two ways:
Since we can't control how Travis creates venvs, the solution is to leverage |
Hi @rpkilby,
AFIAU this is upheld by many projects that could be regarded as part of the infrastructure of Python being used by many projects in a wide range of setups. For those projects pip is kind of the gold standard of which Python versions you need to support if you want to support all the relevant versions.
No sorry, can't. I am not even 100% that this was about venv. This happened at the sprint last year, when tox was still living on bitbucket and the PRs weren't moved to Github, so you would have to look here: https://bitbucket.org/hpk42/tox/ - all I can remember was that it changed the way how venvs were created and everybody agreed that this is a reasonable change and should not cause any problems, but it did cause problems and they were in the weirdness area that you describe happening on Travis. |
Good point. Projects I've participated in haven't been considered "core python infrastructure", so this compat longevity has never been a consideration. Given the above, this PR can't be merged until pip 11 (or whenever pip officially drops python 3.3 support). In the mean time, would you (or anyone on the tox team) object to me publishing a |
sorry, i missclicked |
@rpkilby I'm not sure what's the benefit you're aiming for with moving over venv; most of the issues you raised in your initial commit are things that eventually virtualenv will not be supported; until that happens I would prefer to not fork the project (and at that point this project would move over to venv also). |
Hi @gaborbernat, two reasons:
Basically, a |
I haven't really followed this, but I wonder - would it be feasible to make virtualenv creation pluggable, with hooks implementing the current default behavior, and then making a tox-venv plugin instead of a fork? |
@The-Compiler - fantastic idea. This should be possible, given that that testenv creation is one of the supported hooks. http://tox.readthedocs.io/en/latest/plugins.html#tox.hookspecs.tox_testenv_create |
@rpkilby if you make it a plugin we are happy to host it under tox-dev 👍 for better exposure |
Hi all, I've created the tox-venv plugin and uploaded it to PyPI. I've tested it on django-filter, and it seems to work. As you can see, the before and after shows that the I'm happy to move the plugin to the tox-dev org as suggested by @gaborbernat, or to just leave it under my personal account. Let me know what you want to do here. I've also updated the PR, but the setuptools issue has caused the py34 build to fail. Otherwise, the PR also looks to be in good shape. |
@rpkilby do you need this merged for the plugin to work? |
@gaborbernat nope - the plugin works as is. The code is actually pretty minimal (see: hooks.py). Most of the work was just figuring out why everything was breaking initially. |
in that case this PR can be closed, right? |
I'd say it's safe to close for now. This can be readdressed once pip 11 comes out and py26/py33 are dropped. Either that or when 2020 finally roles around and py2k support ends 😄 |
Thank you for creating tox-venv. I was about to file an issue where matplotlib wouldn't install due to pypa/virtualenv#54, but I was able to readily work around the issue by simply installing |
Hi all, I'm reopening this for reconsideration, now that setuptools, pip, and tox no longer support Python 2.6 and Python 3.3. |
There's an issue about doing this. However this pr is no longer compatible with the current master branch as such I'll close it. We'll migrate the code base of tox-venv. |
I'd like to propose that the builtin
venv
module be used for python 3 virtual environments instead ofvirtualenv
(resolves #436), and this is now possible given python 3.3's impending EOL.There is more discussion in #436, but the short version is that
virtualenv
is not fully compatible with newer versions of python. Currently, deprecation warnings are raised, but eventually these will become exceptions. There was a rewrite a while back, but the PR was closed due to bit rot. Additionally, development/maintenance ofvirtualenv
seems to have largely ended (a total of three non-merge commits this year), so it's unlikely that the issue will be fixed byvirtualenv
.It's worth noting that this PR is dependent on dropping python 3.3 support, as the
--copies
option was not supported byvenv
yet. I've also gone ahead and dropped python 2.6 support, but this isn't strictly necessary.Changelog
Start using the builtin venv module for python 3 virtual environments, and drop python 2.6 & 3.3 support.
Checklist
if an enhancement PR please create docs and at best an exampleCONTRIBUTORS
;