Avoid Debian-patched pip in the shared venv #394
Conversation
|
Test plan:
This confirms that my fix works! Replacing step 3 with Now to fix unit tests. |
src/pipx/shared_libs.py
Outdated
| # current upstream pip version. Otherwise there's a flood of | ||
| # bug reports about things not working, like | ||
| # https://github.com/pipxproject/pipx/issues/386 | ||
| self.upgrade(["-I"] + pip_args, verbose) |
There was a problem hiding this comment.
Will this be a NO OP when running outside of Debian? Would not want to make all platforms slower to patch a Debian bug.
There was a problem hiding this comment.
I don't know. Some measurements would be nice to have. I don't have any non-Debian-based systems to test against, unless maybe I could come up with something using Docker and Alpine?
There was a problem hiding this comment.
lol, -I makes this step faster
$ docker run -ti --rm python:3.8-alpine sh
... some experiments to get the commands right ...
... which incidentally populate ~/.pip/cache so the measurements are independent of network speed ...
/ # rm -rf env && time python3 -m venv env --clear && time env/bin/python -m pip --disable-pip-version-check install --upgrade pip setuptools wheel
real 0m 1.75s
user 0m 1.59s
sys 0m 0.15s
Collecting pip
Using cached https://files.pythonhosted.org/packages/54/0c/d01aa759fdc501a58f431eb594a17495f15b88da142ce14b5845662c13f3/pip-20.0.2-py2.py3-none-any.whl
Collecting setuptools
Using cached https://files.pythonhosted.org/packages/a0/df/635cdb901ee4a8a42ec68e480c49f85f4c59e8816effbf57d9e6ee8b3588/setuptools-46.1.3-py3-none-any.whl
Collecting wheel
Using cached https://files.pythonhosted.org/packages/8c/23/848298cccf8e40f5bbb59009b32848a4c38f4e7f3364297ab3c3e2e2cd14/wheel-0.34.2-py2.py3-none-any.whl
Installing collected packages: pip, setuptools, wheel
Found existing installation: pip 19.2.3
Uninstalling pip-19.2.3:
Successfully uninstalled pip-19.2.3
Found existing installation: setuptools 41.2.0
Uninstalling setuptools-41.2.0:
Successfully uninstalled setuptools-41.2.0
Successfully installed pip-20.0.2 setuptools-46.1.3 wheel-0.34.2
real 0m 2.53s
user 0m 2.05s
sys 0m 0.19s
/ # rm -rf env && time python3 -m venv env --clear && time env/bin/python -m pip --disable-pip-version-check install -I --upgrade pip setuptools wheel
real 0m 1.77s
user 0m 1.61s
sys 0m 0.15s
Collecting pip
Using cached https://files.pythonhosted.org/packages/54/0c/d01aa759fdc501a58f431eb594a17495f15b88da142ce14b5845662c13f3/pip-20.0.2-py2.py3-none-any.whl
Collecting setuptools
Using cached https://files.pythonhosted.org/packages/a0/df/635cdb901ee4a8a42ec68e480c49f85f4c59e8816effbf57d9e6ee8b3588/setuptools-46.1.3-py3-none-any.whl
Collecting wheel
Using cached https://files.pythonhosted.org/packages/8c/23/848298cccf8e40f5bbb59009b32848a4c38f4e7f3364297ab3c3e2e2cd14/wheel-0.34.2-py2.py3-none-any.whl
Installing collected packages: pip, setuptools, wheel
Successfully installed pip-20.0.2 setuptools-46.1.3 wheel-0.34.2
real 0m 1.83s
user 0m 1.47s
sys 0m 0.18s
without -I: 2.53s
with -I: 1.83s
overhead of python -m venv env --clear: 1.75s (same in both cases)
There was a problem hiding this comment.
which incidentally populate ~/.pip/cache so the measurements are independent of network speed ...
Unless I'm misunderstanding something - this comment highlights that time measured avoids one of the most relevant impacts - all users will have to download pip. Here are the docs for -I
-I, --ignore-installed Ignore the installed packages, overwriting them. This can
break your system if the existing package is of a different version
or was installed with a different package manager!
So this would always ignore the system pip (possibly re-downloading it) for everyone.
There was a problem hiding this comment.
That is a good point, but as you saw in my terminal log, pip is downloaded and installed irrespective of the -I flag.
I'm not familiar with venv and I don't know where it found the old outdated versions of pip/setuptools that got installed in my experimental venv. Do you know how I could reproduce (in Docker?) a situation where venv installs up-to-date versions if pip/setuotools that don't require an upgrade?
There was a problem hiding this comment.
pip is downloaded and installed irrespective of the -I flag.
That is a good point, but as you saw in my terminal log, pip is downloaded and installed irrespective of the -I flag.
The log shows pip was cached, but in any case, the shared libs directory is created rarely, and installing a version of pip that is guaranteed to be good is appealing. Being able to install packages from the internet is required for pipx to work, so I don't think pip, setuptools, and wheel being downloaded every once in a while will have a significant negative impact on user experience.
Is this actually overwriting the system pip or just the pip in the venv? If it's the former I don't think we should land it because we could be breaking OS's. If it's the latter, I think we should merge this since it will make pipx hermetic to whatever weird system things are going on. Does anyone know?
There was a problem hiding this comment.
Another q: should --force-reinstall be preferred? https://stackoverflow.com/questions/51913361/difference-between-pip-install-options-ignore-installed-and-force-reinstall indicated the following
"Otherwise, you may get obscure import errors after reinstall due to stale modules still available in sys.path."
There was a problem hiding this comment.
There is a scary warning in the pip docs:
Ignore the installed packages, overwriting them. This can break your system if the existing package is of a different version or was installed with a different package manager!
https://pip.pypa.io/en/stable/reference/pip_install/
I don't know enough about the relationship between OS packages vs how --ignore-installed or --force-reinstall affects them, so unfortunately I don't feel comfortable merging this.
docs/changelog.md
Outdated
| @@ -5,6 +5,7 @@ dev | |||
| - [bugfix] Replaced implicit dependency on setuptools with an explicit dependency on packaging (#339). | |||
| - [refactor] Moved all commands to separate files within the commands module (#255). | |||
| - [bugfix] Continue reinstalling packages after failure | |||
| - [bugfix] Fixed incompatibility with Debian/Ubuntu changes made to their version of pip (#386). | |||
There was a problem hiding this comment.
Considering the bug is within Debian packaging I consider this more an enhancement than bug fix; something along the line (Support Debian 20s pip patching).
There was a problem hiding this comment.
I've checked the "Allow edits from maintainers" box.
I'm a bit confused about the phrase "Support Debian 20s pip". What's "20s"? pip version?
I haven't mentioned specific Debian/Ubuntu/pip versions because I haven't done the research to figure out where the incompatibility got introduced. I know things worked in Ubuntu 19.10 and broke in 20.04. I don't run Debian myself and have trouble remembering its version numbers and codenames.
There was a problem hiding this comment.
Would
| - [bugfix] Fixed incompatibility with Debian/Ubuntu changes made to their version of pip (#386). | |
| - [enhancement] Insulate from problems caused by Debian/Ubuntu changes made to their version of pip (#386). |
work for you?
There was a problem hiding this comment.
Debian decided lately to package pip by trying to devendor six from within pip... the problem is the way they did this unvendoring does not work with venvs... Not to mention the pip within the venv is supposed to be the latest pip (or the bundled version), but either way not a reference to the operating system pip, but a full copy on its own that install/uninstallable.
Anyways I've purposefully did not do any changes here as IMHO this should be fixed on Debian side and not our end. Just throwing my ideas out there, however not planning to accept this PR myself. Maybe other maintainers are more agreeable to it, in which case I'll not stop it 👍
There was a problem hiding this comment.
The optimal solution is for Debian to fix this issue. But from a user perspective, it looks like pipx is broken, and there is no way for us to control Debian. Is there a timeline for the fix on the Debian side?
There was a problem hiding this comment.
Do you have a link to the bug in the Debian bug tracker? (This assumes anyone reported it to Debian already.)
Once it's fixed in Debian, it should be possible to backport the fix to Ubuntu 20.04 LTS by creating a SRU. It's just tedious work.
In the meantime I suspect users will keep filing duplicate bug reports or looking for alternative projects...
There was a problem hiding this comment.
Doesn't look like anyone's reported it to Debian, so I went ahead and did just now: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=958764
|
I'm reposting my code comment reply here: @mgedmin wrote:
Unless I'm misunderstanding something - this comment highlights that time measured avoids one of the most relevant impacts - all users will have to download pip. Here are the docs for -I So this would always ignore the system pip (possibly re-downloading it) for everyone. I don't think that's great. I think the approach in #388 is sufficient for getting a workable pip out of how it is packaged in Debian that does not force everyone else to reinstall pip. |
|
Where are we on this? Debian has fixed its pip installation thanks to @ivanov (#388 (comment)). Is this still needed or should it be closed? Its fine if it causes a slight slowdown because it only affects the shared libraries, which are installed rarely, and thus the impact to the user will be minimal. I am curious if adding this would sidestep various inconsistencies in the environment though, which might be nice since it would ensure the same pip source code is used for everyone. In that case we may want to consider adding this even though Debian has fixed its vendoring upstream. |
mgedmin
force-pushed
the
debian-what-you-doin
branch
from
6ab2540 to
1040577
Compare
|
Rebased on master to resolve the changelog merge conflict. |
cs01
force-pushed
the
debian-what-you-doin
branch
from
1040577 to
70f027b
Compare
Should fix #386.
This fix should be more lightweight than #387, and it is simpler than #388. It tries to find an acceptable middle ground between explicitly supporting Debian's changes to their pip and causing pain to users in order to make a statement.
This patch needs testing. I haven't come up with a test plan yet (I don't want to mess up my currently-working actual pipx setup), but I'll try to do something with docker.