Skip to content
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

Rewrite virtualenv #697

Closed
wants to merge 78 commits into from
Closed

Rewrite virtualenv #697

wants to merge 78 commits into from

Conversation

dstufft
Copy link
Member

@dstufft dstufft commented Jan 5, 2015

This Pull Request was previously #691 however I've moved the rewrite branch to pypa/virtualenv to make it more official.

This is a major change to the virtualenv code base, and as thus I want to get some sign off and opinions from the other @pypa/virtualenv-developers. I don't consider this idea a forgone conclusion without input from the other core developers.

This rewrites the entirety of virtualenv, this is a "dangerous" thing to do which risks breaking things that have been working and generally complete rewrites are considered a bad idea. However while attempting to fix errors like #694, #671, #666, #650, #672, #687, #674, #648, etc I ran into situation where I believe it's not feasible to fix all of the issues with virtualenv given the current methods it employs. In particular I believe that the way -p is implemented makes it extremely fragile if not impossible to work accurately in all situations.

In particular I believe there are a number of issues, most of which stem from architectural issues with virtualenv itself. The (incomplete) list of issues are:

  • The -p functionality to restart the script, which is likely installed into one Python's site-packages directory, seems to be fundamentally flawed in a way where it's not reasonable to fix it in all situations.
  • The previous code relied on a number of hacks in order to implement isolation. This comes from a time when Python had no first class support for isolation and the only way to do it was to implement hacks. This also forces us to maintain a different set of hacks for each target interpreter and sometimes different hacks for different interpreters for different platforms. However since Python 3.3 Python itself has a virtualenv like module called venv. While I do not believe that venv is (or should) be a complete replacement for virtualenv, I do believe that the underlying isolation mechanism is likely to be far more robust given that it comes from the interpreter itself. This also pushes the maintenance burden of implementing the isolation to the people best able to control what the interpreter does.
  • The code is extremely hard to read, it is not well factored and over time it's only grown more unwieldy and more gnarly.
  • Over time the code has grown more and more hacks to solve specific problems. A lot of these hacks are not documented or commented at all and it's extremely difficult to determine which hacks apply any longer or which are no longer required.
  • There is almost a non existent test suite. I have wanted to attempt to write a test suite previously however the way the code is structured it makes it extremely difficult to actually write tests.
  • The virtualenv.py module attempts to run as a single file, however it can't actually do that because it requires the virtualenv_support directory to exist alongside it. I believe that it's saner and easier to have a single directory virtualenv/ directory which contains everything that needs to exist for an install to work. This also removes giant base64 encoded blobs from needing to be maintained inside of the virtualenv.py file.
  • The current means of isolation relies on installing a custom patched site.py that we maintain inside of virtualenv. However this site.py misses features from later versions of Python which means that there are subtle differences in how Python behaves from within a virtual environment and without.

In contrast the rewrite is structured in a different way.

In order to still support the -p option without needing to implement the hackish "restart in target interpreter" logic, the rewrite instead (for the "legacy" isolation method) inspects the target environment for any values it needs. It does this by running a subprocess which will shove all of the required values into a dictionary, and then dump that as json. When the virtualenv script pulls these values out it then loads the json and then has all of the values it needs in order to construct the virtual environment without needing to execute itself with the target interpreter.

Given that newer versions of Python has built in isolation the rewrite wants to take advantage of that. It will detect if the target Python has the venv module, and if they do it will execute a short script (via -c) that will do the actual creation of the virtual environment (without any scripts or tools installed). This will also allow alternative implementations (like PyPy) to implement the isolation themselves so that they can ensure that isolation works correctly in their interpreter without needing to maintain a set of instructions inside of virtualenv for handling their interpreter.

The rewrite also attempts to start fresh with none of the additional hacks. This will mean that we'll likely be broken in certain situations and we'll need to determine how the old virtualenv solved that problem and reapply any additional hacks. However this time we can hopefully ensure that we document/comment what those hacks are and why we're doing it. We'll also ideally write a good set of tests for those hacks so that if we ever remove them in the future we'll have a test suite that will tell us if we broke anything by removing them. Another benefit is that since the hacks are (almost?) always related to the legacy isolation mechanism they can all be quarantined to the virtualenv.builders.legacy.LegacyBuilder class instead of leaking all over the code base.

The new code base is also implemented in a much more modular way. Instead of a ball of spaghetti where the implementation of isolation is spread all over the place, the new code uses "Builder" classes which handle creating the actual environments. In addition (thanks to @ionelmc) it utilizes a "flavor" system which abstracts away the differences between Windows and Posix in the builder code.

In addition to the better (imo) layout of the code base, the new code base is also going to mandate that there is 100% unit test coverage at all times. This will help ensure that we continue to work through refactoring and the like in the future.

No matter which isolation method is in use, virtualenv itself will install it's own scripts and install it's own tools (pip and setuptools). This is so we can have consistent scripts and tool versions no matter which isolation mechanism is in play. It also makes it easier to get newer versions of the tooling and scripts without requiring people to update their entire Python interpreter. In other words if the venv isolation mechanism is available, we will only use it to create the isolated environment and nothing else.

The high level overview of what the major differences in capabilities are:

  • --relocatable no longer is supported as it never worked very well and I don't believe it's possible to actually make it work in all cases. It appears the major use case for this is to avoid re-compiling software, for which I believe Wheels are a better solution.
  • The virtual environment always contains copies instead of attempting to use symlink. The new site.py file in the rewrite allows us to copy a lot less files over than previously required. Almost the entire size of an empty virtual environment in the new rewrite is the executable files, which are required to be copied no matter what.
  • Support a clean and simple API for using virtualenv instead of the hackish bootstrap script method that the previous versions used to use.
  • The deprecated --distribute option is gone.
  • The --no-pip and --no-setuptools options now operate independently of each other. In addition there are now --setuptools and --pip options which will negate the --no-* options if they were given previously.
  • Instead of sys.real_prefix we have sys.base_prefix and sys.base_exec_prefix in order to match what the venv module does.
  • The new code base is tested on Windows using Appveyor.
  • The new code drops the virtualenv-X.Y entry points which didn't work correctly with Wheel anyways and are unneeded since it doesn't really matter what version of Python virtualenv is installed with.
  • The new code only uses setuptools in its setup.py and will always use the script wrappers in that situation.

That major things left to do are:

  • Write all of the unit tests (mostly done).
  • Write all of the functional tests.
  • Ensure/fix the legacy isolation mechanism on PyPy (Jython and IronPython too maybe?).
  • Fix virtualenv-in-virtualenv (including for environments created pre-rewrite) so that it correctly uses the "real" Python.
  • Update the documentation to match the state of the repository now.

There is an open question about how best to handle the transition. If we just merge this and release it then we risk regressing behavior for a lot of people since this is essentially brand new untested code. In the previous Pull Request @ncoghlan suggested releasing it on PyPI as something like virtualenv-rewrite for a period of time and asking people to switch to using that instead. I think that's a good idea and that's how I would plan the migration to go I think.

Summary of outstanding issues that get fixed/closed:

Summary of outstanding pull requests that get fixed/closed:

I am pretty sure that this fixes more issues and pull requests than I've listed here. However I don't feel like going through every single issue and determining which ones are still valid when they would require me to actually attempt to reproduce the issues instead of simply looking at the issue and determining if the new code can handle that situation.

dstufft and others added 30 commits January 4, 2015 20:27
…something wrong with the output: on 2.7 they are unicode literals).
…windows the subprocess must have the SYSTEMROOT env var, otherwise it cannot load DLLs.
@matthew-brett
Copy link

Sorry to come to this discussion late. Can I ask - what is the plan here?

Would it be worth making this rewrite into a temporary pip package such as virtualenv-dev to make it easier to battle-test in a number of environments? I guess it would need a horrible warning that this package was about to go away.

@matthew-brett
Copy link

Oh sorry - I see now that you already have a virtualenv-rewrite package.

That branch (not this one) also solved my Windows virtualenv woes on Python 3.5 (as well as working on 2.7. 3.3 3.4). I was getting this error on Python 3.5:

Running virtualenv with interpreter c:\Python35\python.exe
Using base prefix 'c:\\Python35'
New python executable in venv-venv\Scripts\python.exe
ERROR: The executable venv-venv\Scripts\python.exe is not functioning
ERROR: It thinks sys.prefix is 'c:\\repos' (should be 'c:\\repos\\venv-venv')
ERROR: virtualenv is not compatible with this system or executable
Note: some Windows users have reported this error when they installed Python for "Only this user" or have multiple versi
ons of Python installed. Copying the appropriate PythonXX.dll to the virtualenv Scripts/ directory may fix this problem.

I wasn't getting this error : #796 - but perhaps the rewrite fixes that too?

@ionelmc
Copy link

ionelmc commented Nov 23, 2015

I wasn't getting this error : #796 - but perhaps the rewrite fixes that too?

The rewrite fixes that because it will use the venv that ships with Python 3.

@ionelmc
Copy link

ionelmc commented Nov 23, 2015

Oh sorry - I see now that you already have a virtualenv-rewrite package.

I have published that as an experiment. In retrospect I think it's a bad idea since the legacy virtualenv will still be installed if you install that. It's like a last one wins situation, and in some cases it's hard to predict which one will run.

@matthew-brett
Copy link

I wonder if there is a way forward here. Is there general agreement that this direction is the way forward? Is it possible that the risk from accepting so much code without much review is outweighed by the cost of maintaining the older code-base? Would it be worth biting the bullet and agreeing on a fork to a project with another name, to get wider testing?

@BrownTruck
Copy link

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master. Unfortunately, this pull request does not cleanly merge against the current master branch.

If you do nothing, this Pull Request will be automatically closed by @BrownTruck since it cannot be merged.

If this pull request is still valid, please rebase it against master (or merge master into it) and resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to rebase/merge and resubmit this Pull Request, here is an example message that you can copy and paste:

This Pull Request was previously #691 however I've moved the rewrite branch to pypa/virtualenv to make it more official.

This is a major change to the virtualenv code base, and as thus I want to get some sign off and opinions from the other @pypa/virtualenv-developers. I don't consider this idea a forgone conclusion without input from the other core developers.

This rewrites the entirety of virtualenv, this is a "dangerous" thing to do which risks breaking things that have been working and generally complete rewrites are considered a bad idea. However while attempting to fix errors like #694, #671, #666, #650, #672, #687, #674, #648, etc I ran into situation where I believe it's not feasible to fix all of the issues with virtualenv given the current methods it employs. In particular I believe that the way -p is implemented makes it extremely fragile if not impossible to work accurately in all situations.

In particular I believe there are a number of issues, most of which stem from architectural issues with virtualenv itself. The (incomplete) list of issues are:


The -p functionality to restart the script, which is likely installed into one Python's site-packages directory, seems to be fundamentally flawed in a way where it's not reasonable to fix it in all situations.
The previous code relied on a number of hacks in order to implement isolation. This comes from a time when Python had no first class support for isolation and the only way to do it was to implement hacks. This also forces us to maintain a different set of hacks for each target interpreter and sometimes different hacks for different interpreters for different platforms. However since Python 3.3 Python itself has a virtualenv like module called venv. While I do not believe that venv is (or should) be a complete replacement for virtualenv, I do believe that the underlying isolation mechanism is likely to be far more robust given that it comes from the interpreter itself. This also pushes the maintenance burden of implementing the isolation to the people best able to control what the interpreter does.
The code is extremely hard to read, it is not well factored and over time it's only grown more unwieldy and more gnarly.
Over time the code has grown more and more hacks to solve specific problems. A lot of these hacks are not documented or commented at all and it's extremely difficult to determine which hacks apply any longer or which are no longer required.
There is almost a non existent test suite. I have wanted to attempt to write a test suite previously however the way the code is structured it makes it extremely difficult to actually write tests.
The virtualenv.py module attempts to run as a single file, however it can't actually do that because it requires the virtualenv_support directory to exist alongside it. I believe that it's saner and easier to have a single directory virtualenv/ directory which contains everything that needs to exist for an install to work. This also removes giant base64 encoded blobs from needing to be maintained inside of the virtualenv.py file.
The current means of isolation relies on installing a custom patched site.py that we maintain inside of virtualenv. However this site.py misses features from later versions of Python which means that there are subtle differences in how Python behaves from within a virtual environment and without.


In contrast the rewrite is structured in a different way.

In order to still support the -p option without needing to implement the hackish "restart in target interpreter" logic, the rewrite instead (for the "legacy" isolation method) inspects the target environment for any values it needs. It does this by running a subprocess which will shove all of the required values into a dictionary, and then dump that as json. When the virtualenv script pulls these values out it then loads the json and then has all of the values it needs in order to construct the virtual environment without needing to execute itself with the target interpreter.

Given that newer versions of Python has built in isolation the rewrite wants to take advantage of that. It will detect if the target Python has the venv module, and if they do it will execute a short script (via -c) that will do the actual creation of the virtual environment (without any scripts or tools installed). This will also allow alternative implementations (like PyPy) to implement the isolation themselves so that they can ensure that isolation works correctly in their interpreter without needing to maintain a set of instructions inside of virtualenv for handling their interpreter.

The rewrite also attempts to start fresh with none of the additional hacks. This will mean that we'll likely be broken in certain situations and we'll need to determine how the old virtualenv solved that problem and reapply any additional hacks. However this time we can hopefully ensure that we document/comment what those hacks are and why we're doing it. We'll also ideally write a good set of tests for those hacks so that if we ever remove them in the future we'll have a test suite that will tell us if we broke anything by removing them. Another benefit is that since the hacks are (almost?) always related to the legacy isolation mechanism they can all be quarantined to the virtualenv.builders.legacy.LegacyBuilder class instead of leaking all over the code base.

The new code base is also implemented in a much more modular way. Instead of a ball of spaghetti where the implementation of isolation is spread all over the place, the new code uses "Builder" classes which handle creating the actual environments. In addition (thanks to @ionelmc) it utilizes a "flavor" system which abstracts away the differences between Windows and Posix in the builder code.

In addition to the better (imo) layout of the code base, the new code base is also going to mandate that there is 100% unit test coverage at all times. This will help ensure that we continue to work through refactoring and the like in the future.

No matter which isolation method is in use, virtualenv itself will install it's own scripts and install it's own tools (pip and setuptools). This is so we can have consistent scripts and tool versions no matter which isolation mechanism is in play. It also makes it easier to get newer versions of the tooling and scripts without requiring people to update their entire Python interpreter. In other words if the venv isolation mechanism is available, we will only use it to create the isolated environment and nothing else.

The high level overview of what the major differences in capabilities are:



--relocatable no longer is supported as it never worked very well and I don't believe it's possible to actually make it work in all cases. It appears the major use case for this is to avoid re-compiling software, for which I believe Wheels are a better solution.
The virtual environment always contains copies instead of attempting to use symlink. The new site.py file in the rewrite allows us to copy a lot less files over than previously required. Almost the entire size of an empty virtual environment in the new rewrite is the executable files, which are required to be copied no matter what.
Support a clean and simple API for using virtualenv instead of the hackish bootstrap script method that the previous versions used to use.
The deprecated --distribute option is gone.
The --no-pip and --no-setuptools options now operate independently of each other. In addition there are now --setuptools and --pip options which will negate the --no-* options if they were given previously.
Instead of sys.real_prefix we have sys.base_prefix and sys.base_exec_prefix in order to match what the venv module does.
The new code base is tested on Windows using Appveyor.
The new code drops the virtualenv-X.Y entry points which didn't work correctly with Wheel anyways and are unneeded since it doesn't really matter what version of Python virtualenv is installed with.
The new code only uses setuptools in its setup.py and will always use the script wrappers in that situation.


That major things left to do are:



 Write all of the unit tests (mostly done).

 Write all of the functional tests.

 Ensure/fix the legacy isolation mechanism on PyPy (Jython and IronPython too maybe?).

 Fix virtualenv-in-virtualenv (including for environments created pre-rewrite) so that it correctly uses the "real" Python.

 Update the documentation to match the state of the repository now.


There is an open question about how best to handle the transition. If we just merge this and release it then we risk regressing behavior for a lot of people since this is essentially brand new untested code. In the previous Pull Request @ncoghlan suggested releasing it on PyPI as something like virtualenv-rewrite for a period of time and asking people to switch to using that instead. I think that's a good idea and that's how I would plan the migration to go I think.

Summary of outstanding issues that get fixed/closed:


Closes #694 - Cannot create virtualenv after upgrading OS X 10.6.8 Python to 2.7.
Closes #692 - virtualenv-2.7 no longer exists.
Closes #671 - virtualenv -p python3.4 fails with ImportError: No module named 'copy_reg'
Closes #666 - virtualenv 1.11.6 not working with pypy 2.4 and pypy3 2.4
Closes #659 - Installing a virtualenv doesn't link pyc files.
Closes #656 - bootstrap -> No module named pip
Closes #654 - --always-copy flag doesn't copy
Closes #640 - The executable python does not exist
Closes #626 - tests not included in tarballs on pypi
Closes #625 - ImportError for Python3
Closes #622 - 1.11.6 creates right shebang but wrong file name
Closes #617 - Skip "fix_local_scheme" for "virtualenv ."? (symlinks all files)
Closes #615 - Problem with symlinked paths
Closes #605 - option always-copy doesn't work with OOTB CentOS 6.5
Closes #587 - Python2.7 site.py Not Properly Symlinked
Closes #586 - virtualenv wheel's entry-point script is version specific to the python its built with
Closes #576 - ImportError: No module named optparse.
Closes #571 - site.getsitepackages() doesn't exist in virtualenv
Closes #565 - Breakage in --always-copy - fails with: ImportError: No module named time
Closes #560 - The tests/ directory is not included in the tarball.
Closes #558 - Make --relocatable even more relocatable
Closes #557 - --system-site-packages fails if PIP_REQUIRE_VIRTUALENV=true
Closes #555 - DeprecationWarning
Closes #530 - test_always_copy_option() fails
Closes #500 - add in a test framework similar to pip
Closes #464 - Why I read « Helper script to rebuild virtualenv.py from virtualenv_support » in comment but the code don't use virtualenv_support folder content ?
Closes #434 - Missing site.get*() methods on 3.2 and later
Closes #414 - Review the use of "embedded binary blobs" for support scripts
Closes #405 - need test to confirm rebuild-script.py has been run
Closes #368 - Consolidate the test suite
Closes #355 - site.py not compatible with python 2.7
Closes #340 - Generated bootstrap script's install_pip isn't specific enough
Closes #319 - AssertionError while creating virtualenv on windows 7
Closes #286 - Add code comment re: base64-encoded code in virtualenv.py
Closes #204 - --relocatable doesn't work with PyPy
Closes #172 - virtualenv's main should take arguments
Closes #125 - Improved support for --exec-prefix
Closes #90 - --relocatable is not reliable
Closes #11 - --relocatable does not point activate script to the correct path
Closes #2 - --clear doesn't clear out bin/


Summary of outstanding pull requests that get fixed/closed:


Closes #686 - Document when --always-copy was introduced (in PR #409)
Closes #672 - Avoid sys.path pollution when switching interpreters
Closes #663 - Autodetect if symlinks are supported on target file system
Closes #657 - Add a note, that bootstrap script can't install pip.
Closes #647 - Fix formating of activate_this.py
Closes #638 - Split out this setup.py fix from #635

Closes #636 - Fix to better support python installs where prefix != exec_prefix.
Closes #634 - A collection of fixes for when prefix != exec_prefix.
Closes #597 - [WIP] Do some testing
Closes #561 - Add the tests/ directory to the tarball.
Closes #547 - Use decorator to classmethod.
Closes #544 - Source distributions should include tests
Closes #508 - Add getsitepackages() to site module (Issue #355)
Closes #485 - Simplify setting up symlinks/copying files
Closes #479 - Making activate.bat relocatable.
Closes #473 - Relative local symlinks
Closes #389 - Made make_relative_path handle symlinks correctly.
Closes #236 - relocatable activate for bash, dash, zsh, ksh, fish, Windows


I am pretty sure that this fixes more issues and pull requests than I've listed here. However I don't feel like going through every single issue and determining which ones are still valid when they would require me to actually attempt to reproduce the issues instead of simply looking at the issue and determining if the new code can handle that situation.

---

*This was migrated from pypa/virtualenv#697 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

@BrownTruck
Copy link

This Pull Request was closed because it cannot be automatically reparented to the master branch and it appears to have bit rotted.

Please feel free to re-open it or re-submit it if it is still valid and you have rebased it onto master or merged master into it.

@ofek
Copy link
Sponsor Contributor

ofek commented Sep 30, 2017

Is there still interest in this?

@ncoghlan
Copy link
Member

ncoghlan commented Oct 2, 2017

I still think this is a good idea, but I think the tooling landscape has changed sufficiently to make it worthwhile to reassess whether an in-place rewrite is still preferable to the alternative option of introducing a second venv management library:

  • Option 1: make the rewrite a direct successor to the existing virtualenv, as this PR does, such that all virtualenv usage is implicitly upgraded to the new approach (with the associated backwards compatibility risks) unless folks explicitly restrict themselves to an older version of virtualenv
  • Options2: make the rewrite a distinct project, but also update higher level integration projects (tox, vex, pew, pipenv, pyenv, hatch, etc) to switch to depending on the new library, rather than the traditional one

The UX difference between the two approaches is essentially as follows:

  • For option 1, virtualenv < 16 would become the de facto name of "traditional virtualenv". If anyone wanted to release bug fixes for that (or make it compatible with new Python versions), then they'd need to do a new 15.X.Y release from the main virtualenv repo
  • For option 2, the default would be for people to stay on the old implementation, unless they made the conscious decision to switch to the new one (or else were using a higher level integration project that made the switch on their behalf)

Putting it like that, I think it's clear that despite the potential compatibility risks, the in-place rewrite is likely still the better approach, as it offers the clearest long-term path to switching entirely to PEP 405 based virtual environments as various other projects are able to start dropping support for Python 2.7.

The backwards compatibility risks could then be mitigated by:

  1. Providing notice (perhaps even via a virtualenv 15.1.1 release) that the implementation strategy will be changing in the next major version
  2. Publishing a beta virtualenv rewrite that passes its own test suite
  3. Getting some of the key integration projects (as listed above) to run their own tests against the beta version (or even the git branch before virtualenv publishes the beta release)
  4. Iterate until the virtualenv devs are happy that the remaining compatibility issues are going to be relatively esoteric ones
  5. Actually make the release

@obestwalter
Copy link

I think that sounds like a really good plan and will be happy to help getting this tested with tox in the beta stage.

@pfmoore
Copy link
Member

pfmoore commented Oct 2, 2017

One thought I had, which I've been meaning to try when I get time, was to simply check at the start of virtualenv, whether the request was one that could be satisfied by the core venv library (basically that means there's no -p option, and none of the more esoteric options like --relocatable, plus we're using a version of Python that has venv). If so, then simply defer to venv. That may well result in cosmetically different behaviour (venv is less verbose, the resulting layout of the virtual environment is slightly different) but the end result should be a working virtualenv anyway (modulo corner cases like environments that virtualenv supports but venv doesn't).

This is basically an alternative implementation strategy for @ncoghlan's "Option 1", with a lot less complexity than @dstufft's approach, at the cost of a significantly higher risk that we break backward compatibility (albeit only in ways that can be argued as "implementation details" now that venv exists).

Treating this less as "rewrite virtualenv to use the core machinery" and more like "make virtualenv a frontend for core venv, with a fallback to the old code where needed" seems to me to be lower-risk in the long term.

Thoughts?

@dstufft
Copy link
Member Author

dstufft commented Oct 3, 2017

I personally would still like to finish the rewrite... I just haven't been able to make time for virtualenv lately with Warehouse chewing up most of my OSS time. I'm hoping that once I get that launched I'll have more time for this... but I can't promise that. So that being said, if someone wants to do something else, please don't feel like you need to wait on me.

@gaborbernat
Copy link
Contributor

I'll try to take a look at this as part of PyCon 2018 sprints.

@techtonik
Copy link

A unified interface for remote execution and local directory sync/sharing would be nice. Vagrant, multipass, minikube, snapcraft, tox - they could benefit from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet