rewrite feedback

[gaborbernat]

This is a PR meant to allow people to give feedback on the rewrite. For such big rewrite, I'm not sure Github UI is good so I'd recommend checking out the rewrite branch. Opened the PR though to allow people to comment in the code inline - or attach their feedback as a comment. If you take the time and give feedback I'd like to say thank you very much, appreciated 🙇

A bit of context on the design, the core rewrite idea is to make the project as fast as possible and relatively easy to extend/maintain. We now have a config file support, which allows users to choose their own defaults globally (virtualenv -h now shows where defaults come from, experience prooved that's there's no such thing as good default for everyone, and these can be also used to globally fix when some dependency pip/setuptools breaks the world). Furthermore, we still support python 2; and plan to support it for the foreseeable future - pypy is still supported, and we plan to support that. As such all code must support python 2.7+. That being said, the rewrite simplifies the python 2 env creation, to something that looks a lot more like python 3 style via a pyenv.cfg.

The idea is also to support a clean programmatic API for tox/pipenv/etc (e.g. what's the site-package folder for this env, without creating it; reuse environment discovery, etc).

The project is split into four main sections:

• interpreter discovery (when the user passes in -p 3.8-64 discover/find an interpreter that satisfies those requirements - supports PEP-514 or path discovery mechanism)
• interpreter creation (create the actual environment; either via delegation to venv, or ourselves - here we also implement ourselves creation code even when venv is available to avoid process creation overhead 🤷‍♂ which can be slow on some platforms especially)
• seed packages (install pip/setuptools - either the old way via pip, or a faster method via app data image - 95% of the environment creation is spent within this phase, pip is slow mostly because has to support a lot of features, the app data image installation works around this by needing to support a smaller subset of pip - with this 200ms creation time is now possible)
• activation scripts (now supports unicode paths, warns when a path would break, etc. - also now users can add plugins to add their own activation scripts)

CC @pfmoore @pradyunsg @RonnyPfannschmidt

PS: There still are some release blockers beyond this review -> https://github.com/pypa/virtualenv/milestone/7

[pfmoore]

Some initial (very simple) tests on Windows look OK.

• Installing the new virtualenv in a venv works fine (which is good, as it makes it easier to test without affecting your system Python).
• Similarly, the new virtualenv can be installed using pipx (good for providing a single central installation).
• Using -p 3.8 resulted in a warning about invalid PEP 514 data in my registry for Python 2.7. No big deal as it's a warning, but as it's for a Python version I wasn't trying to use, the warning is unnecessary (and potentially scary for many users). I don't know how the data that was present - I no longer have Python 2.7 installed, although I did at one point. Maybe installing and uninstalling leaves it?

I tried to test with Python 2.7, using a custom Python 2.7 install (installed via Scoop) and virtualenv -p C:\Users\xxx\scoop\apps\python-2.7.17\1.0\Python-2.7.17\python.exe xx. The resulting environment didn't include a copy of python27.dll, and so python.exe failed. Virtualenv 16.7.9 doesn't have this issue with this Python installation. (I don't have anywhere I can install the "official" Python 2.7 at the moment, so I can't test this any further).

By the way, the new virtualenv seems to install pydoc.bat. The old one didn't - ideally I'd prefer this not to be installed, as it interferes with command completion (myenv\Scripts\py<TAB>). I realise this is a minor use case, but xkcd is on my side 🙂

But overall, this looks very nice. Good job!

[asottile]

(I didn't review the tests)

most of my comments are little things

some stuff I noticed from this:

• the new structure ends up with lots of circular imports :S
• while the new structure seems to separate logic nicely, it was kinda hard for me to follow the control flow (especially with the deep inheritance hierarchies)
• I'm very concerned about virtualenv growing dependencies, this breaks a lot of my usecases

I haven't had a chance to actually try this out yet, will give that a spin

[gaborbernat]

Using -p 3.8 resulted in a warning about invalid PEP 514 data in my registry for Python 2.7. No big deal as it's a warning, but as it's for a Python version I wasn't trying to use, the warning is unnecessary (and potentially scary for many users).

This seems a good idea, will try to add.

I tried to test with Python 2.7, using a custom Python 2.7 install (installed via Scoop) and virtualenv -p C:\Users\xxx\scoop\apps\python-2.7.17\1.0\Python-2.7.17\python.exe xx. The resulting environment didn't include a copy of python27.dll, and so python.exe failed.

Interesting, wonder why an official installation python does not need it. 🤔 this needs to be investigated and determined to see if it's reasonable or not to automatically and when to add it.

By the way, the new virtualenv seems to install pydoc.bat. The old one didn't - ideally I'd prefer this not to be installed, as it interferes with command completion (myenv\Scripts\py). I realise this is a minor use case, but xkcd is on my side 🙂

You can always disable the generation of the Batch activation scripts. pydoc working on other shells and not in Batch was a bug we solved now by adding it.

the new structure ends up with lots of circular imports :S

I know of 3 🤔 could not eliminate these three so kept it.

while the new structure seems to separate logic nicely, it was kinda hard for me to follow the control flow (especially with the deep inheritance hierarchies)

I personally find it easier to identify this way what's common across different things, and what's different. This might be just my maintainer POV. Having endless if/else branches in the old way made it hard to see just what each target interpreter needed, where things differed.

I'm very concerned about virtualenv growing dependencies, this breaks a lot of my usecases

Would like to hear with concrete examples here, and why those can't be solved via a zippapp (perhaps one where part of the build operation we bundle the dependencies). Vendoring would be my very last option.

[RonnyPfannschmidt]

first i want to recognize the sheer amount of work that went into this rewrite - thank you :)

also thanks to @asottile - your prior run review things made thins more easy

i like the overall restructuring and splitting things into smaller comprehensible things
the overall quality is just lovely, my key irks where about minor details of api style (which is neglect-able)

so again thank you and great work !

[RonnyPfannschmidt]

this could be a list for all use cases i currently see, is there a use-case requiring this to be a callable?

[gaborbernat]

I'm not sure I follow here 🤔

[RonnyPfannschmidt]

the control flow of this function is horrendous to follow, almost gives me hives

[gaborbernat]

reorganized under #1487

[pfmoore]

I tried to test with Python 2.7, using a custom Python 2.7 install (installed via Scoop) and virtualenv -p C:\Users\xxx\scoop\apps\python-2.7.17\1.0\Python-2.7.17\python.exe xx. The resulting environment didn't include a copy of python27.dll, and so python.exe failed.

Interesting, wonder why an official installation python does not need it. 🤔 this needs to be investigated and determined to see if it's reasonable or not to automatically and when to add it.

Maybe because an all-users Python 2 install using the python.org installer puts python27.dll in the Windows system directory, so the fact that it doesn't get copied doesn't matter in that case? (I think it does that, as I say no official Python 2 installation to hand at the moment to check this...)

By the way, the new virtualenv seems to install pydoc.bat. The old one didn't - ideally I'd prefer this not to be installed, as it interferes with command completion (myenv\Scripts\py). I realise this is a minor use case, but xkcd is on my side 🙂

You can always disable the generation of the Batch activation scripts. pydoc working on other shells and not in Batch was a bug we solved now by adding it.

Ah, if it's considered a "batch" activation script, then yes I could do this, as I use PowerShell, so the batch scripts are not very useful to me. Thanks for the suggestion.

[pfmoore]

with PEP-517/518 its increasingly unnecessary to have this build dependency, as pip would put it into its build env anyway, its however potentially still creating problems for certain legacy packages, as the pip cant build a wheel directly (unchecked, need to check if it holds true)

For some reason I can't reply inline to this comment :-(

Yes, when using the legacy (non-PEP 517) code path, pip will check if wheel is importable, and only build a wheel if it is. In the absence of wheel, it will fall back to setup.py install, which has a number of differences in semantics, as well as not being cache-able.

[pfmoore]

The resulting environment didn't include a copy of python27.dll, and so python.exe failed

Confirmed with a clean install. #1484 raised for this.

[gaborbernat]

I've addressed all points raised inside #1487 (minus three that have their own issues); will close this now, if anyone has further feedback please open a new PR and do it there; or reach out to me. Thanks for everyone for their effort.

[Julian]

Woohoo! In case it hasn't been said enough yet, @gaborbernat you are great thanks for moving this forward it is awesome.

[pradyunsg]

This is all pretty great looking. I haven't finished reviewing this yet, but it seems like you've closed the PR so I'll push these not-so-polished-or-complete review comments.

Items prefixed with "nit: " are exactly that -- nitpicks that IMO aren't worth anyone's time to spend discussing in detail if there are disagreements.

[pradyunsg]

I wonder if it's worth it to make this work, an orphan branch (see https://stackoverflow.com/a/30284471/1931274 for how to). Then we'd rename the current master to legacy (+ add a tag) and rename rewrite to master instead of doing a merge. :P

That'd also allow for the rewrite to be more representative of the degree of work. :P

[gaborbernat]

hmm pushing an orphan branch takes a while 🤔

[gaborbernat]

orphan branch created under https://github.com/pypa/virtualenv/commits/rewrite

[pradyunsg]

Why are some imports relative and some absolute? It'd be preferable to maintain the same style.

[gaborbernat]

mostly because I did not find a tool to automate it 😢 any suggestions? I'd prefer to make relative what's possible without (or at most 1 level .. and keep everything else as absolute)