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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plug-in for bundled wheels #1840

Closed
frenzymadness opened this issue May 22, 2020 · 14 comments 路 Fixed by #1841
Closed

Plug-in for bundled wheels #1840

frenzymadness opened this issue May 22, 2020 · 14 comments 路 Fixed by #1841

Comments

@frenzymadness
Copy link
Contributor

frenzymadness commented May 22, 2020

Hello.

First of all, thank you for your work. New virtualenv looks very good and its codebase is clean and readable 馃憤 Second thing is that I don't want to start another discussion about bundled wheels and their support and updates. I just have a problem to solve and I'd like to do it in cooperation with you because I think that it might help other maintainers.

We'd like to update the python-virtualenv RPM package in Fedora from version 16.7.10 to 20.0.20. The problem we have to solve is that we want the virtualenv provided by the distribution to use our own wheels for pip/setuptools/wheel.

When searching for wheels, we first need to take a look into ensurepip folder like /usr/lib64/python3.4/ensurepip/_bundled 鈥 this folder exists only for some Pythons like 3.4 or 2.7 and the path is obtained from ensurepip module import ensurepip; print(ensurepip.__path__[0]). If we don't find a proper wheel there, we take a look into /usr/share/python-wheels where we always find a wheel for all three bundled packages. The latter might contain wheels not compatible with an older Python so ensurepip always has to take precedence.

We always solve this problem by a downstream patch but you can probably imagine that it's not an easy task to maintain a patch like this. So, I'd like to discuss our solutions with you and we might find a better way which makes it easier for us to use the latest virtualenv with our own wheels.

The main problem with the biggest impact on how the patches mentioned below look is that we always need to have a path to Python interpreter we create a virtual environment for to be able to get the ensurepip path mentioned above.

In solution 1, I've implemented a module with a simple function that yields paths to bundled wheels. I then use it in BaseEmbed to alter extra_search_dir and in pip_wheel_env_run for the same purpose. The patch is here: https://github.com/frenzymadness/virtualenv/pull/1/files

Then I have had an idea to implement it in a different way. I've implemented BUNDLE_SUPPORT as an object which gives me a correct path for each wheel. This looked promising but then, because it also needs to have a path to Python interpreter, it gets complicated. I had to change a lot of functions going down to get_bundled_wheel to pass creator or interpreter to it. The patch is here https://github.com/frenzymadness/virtualenv/pull/2/files

The important thing is that we cannot save the paths somewhere because the wheels are generated by other packages and they might be updated without python-virtualenv update.

What do you think about it? Might it be possible to provide an alternative mechanism (some kind of plug-in) to determine the correct path to bundled wheels?

I am not asking you to implement it for us but you might see something I don't and if we will find a good solution that makes it possible to extend virtualenv this way, I'll be glad to implement it.

@gaborbernat
Copy link
Contributor

gaborbernat commented May 22, 2020

For this to work and virtualenv to behave as expected (for both the app-data and pip seeder) what you'd like is:

  1. always prepend /usr/lib64/python3.4/ensurepip/_bundled and /usr/share/python-wheels to the extra search directory here https://github.com/pypa/virtualenv/blob/master/src/virtualenv/seed/embed/base_embed.py#L20
  2. disable bundled wheels here https://github.com/pypa/virtualenv/blob/master/src/virtualenv/seed/embed/wheels/acquire.py#L37
  3. alter to use get_wheels instead of get_bundled_wheel to acquire pip.

1 and 2 both seem like fairly straight forward 1-line patches so not sure if we'd need a plugin for that. 3 is a virtualenv internal behaviour change which IMHO makes sense to allow/do, but unless is really urgent I'd prefer to put it on top #1821 that needs to heavily alter that acquire file. I'd expect we have something for it within the next 2 weeks.

Once we have 3 we can document how to apply patch 1 and 2 for distribution owners. Would that work for you?

@frenzymadness
Copy link
Contributor Author

frenzymadness commented May 22, 2020

Thanks @gaborbernat for the quick reply. That's almost exactly what I have in my solution 1. Point 1 is not oneliner because the ensurepip path differs for different Pythons (or does not exist at all for some of them) and therefore I have to alter the extra search directory when I know the creator/interpreter.

Point 3 is something I can do in my patch and test it. It might make it smaller for now.

I'll follow the issue and try to help as much as I can.

@frenzymadness
Copy link
Contributor Author

frenzymadness commented May 22, 2020

Point 3 is something I can do in my patch and test it. It might make it smaller for now.

This won't be that easy because get_bundled_wheel takes only a package name and its version but get_wheels needs for_py_version, wheel_cache_dir, extra_search_dir, packages, app_data, download and I don't have them in pip_wheel_env_run.

@gaborbernat
Copy link
Contributor

gaborbernat commented May 22, 2020

I did not say it's easy, that's why needs to be a change in core, it's more involved but doable.

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 11, 2020

cc @kitterma who might also be interested in this from part of Debian

@kitterma
Copy link

kitterma commented Jun 11, 2020

Thanks. I am interested. We don't ship ensurepip, so it'll be a little different for us. I'm all for getting stuff upstream to minimize patching and improving distro commonality where we can.

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 20, 2020

@frenzymadness
Copy link
Contributor Author

frenzymadness commented Jun 22, 2020

The documentation looks good to me, thank you! It might be also a good idea to add links to distribution repositories where you can inspire from when you need to change the behavior in a similar way. For example, Fedora repository is here: https://src.fedoraproject.org/rpms/python-virtualenv/tree/master

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 22, 2020

This now has been released under https://virtualenv.pypa.io/en/20.0.24/changelog.html#v20-0-24-2020-06-22

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 22, 2020

@kitterma can you please link yours too here so we can add?

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 22, 2020

@frenzymadness can you ping me with the change request when you upgrade https://src.fedoraproject.org/rpms/python-virtualenv/tree/master

@kitterma
Copy link

kitterma commented Jun 22, 2020

I have not upgrade to 20.0.24 yet, but it will be here when I do: https://salsa.debian.org/python-team/modules/python-virtualenv

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 22, 2020

@kitterma would be much appreciated if you can ping me here when you do so 馃憤 Let me know if the above documentation needs extending/amending.

@frenzymadness
Copy link
Contributor Author

frenzymadness commented Jun 25, 2020

We have successfully update Fedora RPM package to 20.0.25 and I can gladly say that the latest changes make our downstream patch smaller than before.

https://src.fedoraproject.org/rpms/python-virtualenv/tree/master

@pypa pypa locked and limited conversation to collaborators Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants