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

produce a virtualenv zipapp #1287

Merged
merged 4 commits into from Jan 25, 2019
Merged

produce a virtualenv zipapp #1287

merged 4 commits into from Jan 25, 2019

Conversation

asottile
Copy link
Contributor

this "works" -- I would like some feedback on the approach however:

$ python3 tasks/make_zipapp.py 
zipapp created at /tmp/x/virtualenv/virtualenv.pyz
$ python virtualenv.pyz venv
New python executable in /tmp/x/virtualenv/venv/bin/python
Installing setuptools, pip, wheel...
done.
$ python3 virtualenv.pyz venv3
Using base prefix '/usr'
New python executable in /tmp/x/virtualenv/venv3/bin/python3
Also creating executable in /tmp/x/virtualenv/venv3/bin/python
Installing setuptools, pip, wheel...
done.
$ du -hs virtualenv.pyz 
2.0M	virtualenv.pyz

@pfmoore
Copy link
Member

pfmoore commented Jan 20, 2019

I like this approach a lot. Distributing virtualenv as a standalone pyz file that can be run with any Python interpreter sounds like a much better approach. From what I can tell, the pyz file will still support the -p option, even though it's not particularly needed for a standalone pyz.

There are a lot of projects that depend on virtualenv that likely expect it to be installed (tox, pew, pipenv, ...) - otherwise, I'd be in favour of making this the official distribution method for virtualenv. It may be worth reaching out to those projects to see how hard it would be for them to switch from needing an installed virtualenv, to simply needing a virtualenv command available.

@gaborbernat
Copy link
Contributor

The main impediment Paul for tox e.g. is that while one can guarantee virtualenv exist via install requires, one cannot do the same for pyz. This method would only work if downstream tools themselves would be pyz, however one downside of doing so is that no direct command to the tool is available, but rather someone needs to pass the pyz to the interpreter. Therefore I would say not viable. asottile please add some tests for this so we can validate it works for all flavours of python.

@pfmoore
Copy link
Member

pfmoore commented Jan 20, 2019

@gaborbernat Thanks for the clarification on tox's position. I thought something like that would be the issue. I've done some work bundling various tools, including tox and virtualenv, as zipapps using shiv. It needs a little fiddling to ensure that the unbundled zipapp is on sys.path for subprocess calls (by setting PYTHONPATH, which causes tox to issue a warning). I'd wondered whether there was a way (probably involving changes to tox and other such tools) to work around that, but I've not really had the time to look into it properly. It's very much a niche use case, but it's a pretty useful one (having Python dev tools available standalone as pyz apps is really convenient for certain types of workflow).

I'd still like to see the pyz form as a supported approach, even if only as an additional option.

@gaborbernat
Copy link
Contributor

Yeah, 😁 agreed. later on tox could look into pyz/shiva/pex as well. Some people requested and we accepted to distribute it e.g. via docker too.

@asottile asottile changed the title WIP: produce a virtualenv zipapp produce a virtualenv zipapp Jan 20, 2019
@asottile
Copy link
Contributor Author

I've written some tests, re-enabled install_wheel to run with search_dirs=None, and fixed -p -- the diff is probably best viewed with ?w=1

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach. The tests need some tweaking though, documentation extended, and I feel like we should avoid unwrapping the support files at every invocation and instead have a cache dir.

tasks/make_zipapp.py Show resolved Hide resolved
docs/changelog/1092.feature.rst Outdated Show resolved Hide resolved
tasks/make_zipapp.py Show resolved Hide resolved
tasks/make_zipapp.py Show resolved Hide resolved
tasks/make_zipapp.py Show resolved Hide resolved
# Probably some boot script; just in case virtualenv is installed...
@contextlib.contextmanager
def virtualenv_support_dirs():
"""Context manager yielding either [virtualenv_support_dir] or []"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe more accurate to say: locate virtualenv support files directory

virtualenv.py Show resolved Hide resolved
virtualenv.py Show resolved Hide resolved
virtualenv.py Show resolved Hide resolved
virtualenv.py Outdated Show resolved Hide resolved
@asottile
Copy link
Contributor Author

coincidentally, this branch also fixes:

PYTHONPATH=virtualenv-16.2.0-py2.py3-none-any.whl python -m virtualenv venv

tests/test_zipapp.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
virtualenv.py Outdated
from urllib.parse import urljoin
from urllib.request import pathname2url
with search_dirs_context() as search_dirs:
wheels = find_wheels(["setuptools", "pip"], search_dirs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this generates a long diff because of the indent, what if we would just extract the indented part into its own function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I suggested viewing the diff with ?w=1 in this comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granted that works as a bypass, though would we move this into a sub-function we could avoid changing the lines altogether, not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they'd still change because I'd have to move them -- plus it would increase the api surface of virtualenv (as far as I can tell this function isn't a documented interface but I've found a bunch of calls to it on github >.<)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not if you move it to a private function, right after this function. The private function call would be an insert and everything below would be all is same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not add lines for no reason, what's the problem with the indent?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep this open then until I get to polish it myself

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now only if we can fix #1291 👍

@gaborbernat gaborbernat merged commit cfd9c81 into pypa:master Jan 25, 2019
@gaborbernat
Copy link
Contributor

Released as 16.3.0.
thanks

@asottile asottile deleted the zipapp branch January 29, 2019 03:41
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

3 participants