Use str to pass versions to avoid debundling issue

[uranusjr]

Fix #9348. SpecifierSet.contains automatically parses a string into Version, so we can just avoid using parsed_version here to avoid issues from a poorly-debundled downstream. The change is pretty cheap and straightforward, and I feel it’s easier to include the workaround so we can avoid the need to explain things when users inevitably report the issue here.

[stefanor]

I can see other places this crops up, too:

pip/src/pip/_internal/resolution/resolvelib/candidates.py

Line 183 in e7d6e54

self._version = self.dist.parsed_version

(reproduceable with pip install -e .)

pip/src/pip/_internal/resolution/resolvelib/candidates.py

Line 209 in e7d6e54

if self._version is not None and self._version != dist.parsed_version:

causing:

WARNING: Discarding https://files.pythonhosted.org/packages/c3/f6/369da6782ac505a0d6666d4d506bcf56f65c5242c441f6e64616aeba2da1/world-4.0.tar.gz#sha256=ecfd0ce99e13ed819f8b6489b145ea1d9da2cd2324c4cc3f0ab5deaac7d537e0 (from https://pypi.org/simple/world/). Requested world from https://files.pythonhosted.org/packages/c3/f6/369da6782ac505a0d6666d4d506bcf56f65c5242c441f6e64616aeba2da1/world-4.0.tar.gz#sha256=ecfd0ce99e13ed819f8b6489b145ea1d9da2cd2324c4cc3f0ab5deaac7d537e0 has inconsistent version: filename has '4.0', but metadata has '4.0'

etc. etc. Basically, anything that deals with dist.parsed_version is at risk. I notice other areas in pip use parse(str(dist.parsed_version)).

I ended up with this patch:

From: Stefano Rivera <[email protected]>
Date: Mon, 1 Mar 2021 10:57:10 -0800
Subject: Re-parse pkg_resources Versions from str

When debundling pkg_resources and packaging use different Version
classes, causing trouble.

Based on: https://github.com/pypa/pip/pull/9467

Bug-Upstream: https://github.com/pypa/pip/issues/9348
---
 src/pip/_internal/req/req_install.py                  | 12 ++++++++++--
 src/pip/_internal/resolution/resolvelib/candidates.py |  8 ++++----
 src/pip/_internal/resolution/resolvelib/resolver.py   |  3 ++-
 src/pip/_internal/wheel_builder.py                    |  3 ++-
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py
index 866d18f..548c00d 100644
--- a/src/pip/_internal/req/req_install.py
+++ b/src/pip/_internal/req/req_install.py
@@ -434,8 +434,16 @@ class InstallRequirement(object):
         if not existing_dist:
             return
 
-        existing_version = existing_dist.parsed_version
-        if not self.req.specifier.contains(existing_version, prereleases=True):
+        # pkg_resouces may contain a different copy of packaging.version from
+        # pip in if the downstream distributor does a poor job debundling pip.
+        # We avoid existing_dist.parsed_version and let SpecifierSet.contains
+        # parses the version instead.
+        existing_version = existing_dist.version
+        version_compatible = (
+            existing_version is not None and
+            self.req.specifier.contains(existing_version, prereleases=True)
+        )
+        if not version_compatible:
             self.satisfied_by = None
             if use_user_site:
                 if dist_in_usersite(existing_dist):
diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py
index 83b6c98..5211a17 100644
--- a/src/pip/_internal/resolution/resolvelib/candidates.py
+++ b/src/pip/_internal/resolution/resolvelib/candidates.py
@@ -3,7 +3,7 @@ import sys
 
 from pip._vendor.packaging.specifiers import InvalidSpecifier, SpecifierSet
 from pip._vendor.packaging.utils import canonicalize_name
-from pip._vendor.packaging.version import Version
+from pip._vendor.packaging.version import Version, parse as parse_version
 
 from pip._internal.exceptions import HashError, MetadataInconsistent
 from pip._internal.models.wheel import Wheel
@@ -191,7 +191,7 @@ class _InstallRequirementBackedCandidate(Candidate):
     def version(self):
         # type: () -> _BaseVersion
         if self._version is None:
-            self._version = self.dist.parsed_version
+            self._version = parse_version(self.dist.version)
         return self._version
 
     def format_for_error(self):
@@ -212,7 +212,7 @@ class _InstallRequirementBackedCandidate(Candidate):
         name = canonicalize_name(dist.project_name)
         if self._name is not None and self._name != name:
             raise MetadataInconsistent(self._ireq, "name", dist.project_name)
-        version = dist.parsed_version
+        version = parse_version(dist.version)
         if self._version is not None and self._version != version:
             raise MetadataInconsistent(self._ireq, "version", dist.version)
 
@@ -396,7 +396,7 @@ class AlreadyInstalledCandidate(Candidate):
     @property
     def version(self):
         # type: () -> _BaseVersion
-        return self.dist.parsed_version
+        return parse_version(self.dist.version)
 
     @property
     def is_editable(self):
diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py
index 30b860f..84421d4 100644
--- a/src/pip/_internal/resolution/resolvelib/resolver.py
+++ b/src/pip/_internal/resolution/resolvelib/resolver.py
@@ -4,6 +4,7 @@ import os
 
 from pip._vendor import six
 from pip._vendor.packaging.utils import canonicalize_name
+from pip._vendor.packaging.version import parse as parse_version
 from pip._vendor.resolvelib import ResolutionImpossible
 from pip._vendor.resolvelib import Resolver as RLResolver
 
@@ -141,7 +142,7 @@ class Resolver(BaseResolver):
             elif self.factory.force_reinstall:
                 # The --force-reinstall flag is set -- reinstall.
                 ireq.should_reinstall = True
-            elif installed_dist.parsed_version != candidate.version:
+            elif parse_version(installed_dist.version) != candidate.version:
                 # The installation is different in version -- reinstall.
                 ireq.should_reinstall = True
             elif candidate.is_editable or dist_is_editable(installed_dist):
diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py
index dbc34d0..f7e15af 100644
--- a/src/pip/_internal/wheel_builder.py
+++ b/src/pip/_internal/wheel_builder.py
@@ -9,6 +9,7 @@ import zipfile
 
 from pip._vendor.packaging.utils import canonicalize_name, canonicalize_version
 from pip._vendor.packaging.version import InvalidVersion, Version
+from pip._vendor.packaging.version import parse as parse_version
 from pip._vendor.pkg_resources import Distribution
 
 from pip._internal.exceptions import InvalidWheelFilename, UnsupportedWheel
@@ -200,7 +201,7 @@ def _verify_one(req, wheel_path):
             "got {!r}".format(dist.version, w.version),
         )
     if (_get_metadata_version(dist) >= Version("1.2")
-            and not isinstance(dist.parsed_version, Version)):
+            and not isinstance(parse_version(dist.version), Version)):
         raise UnsupportedWheel(
             "Metadata 1.2 mandates PEP 440 version, "
             "but {!r} is not".format(dist.version)

[uranusjr]

Eventually this is a downstream packaging issue, and your best chance to get a timely answer is to report these to the downstream packager (e.g. Debian). Pip can help as best we can, but there are close to infinite ways the downstream can break the interoperability.

[uranusjr]

With that said, I’d be happy to review the above as a pull request!

[stefanor]

Sorry, if it wasn't clear, I was reviewing your patch in the context of being a downstream packager. It was a good starting point, but I needed to go further.

Happy to file that as an MR

[uranusjr]

I see, sorry for the confusion.

This particular patch will probably soon be irralevant since we’re going to remove the code path to it soon (#9631). Your other changes look right to me though, and I’d be happy to merge them.

[pradyunsg]

I'm not gonna prevent someone from merging this or whatever; but I don't like this. :)

I'd much prefer downstream redistributors who are patching pip be the ones that hold a patch for these cases; because the cause of the issue is something that they've done.

[uranusjr]

I’m hoping the pip._internal.metadata abstraction to solve this once and for all; we can do all the string conversions there so other parts of pip only needs to deal with one kind of Version and SpecifierSet. But that’s a long road ahead…

Alternatively though, maybe SpecifierSet can be made more permissive when it receives something that’s not the exact right type? Instead of complaining, it can simply stringify the value and re-parse it into a version.

https://github.com/pypa/packaging/blob/a71c83345b8005e9da37992d0103c595a4475b43/packaging/specifiers.py#L160-L163

 if not isinstance(version, (LegacyVersion, Version)):
-    version = parse(version)
+    version = parse(str(version))
 return version

[stefanor]

I'd much prefer downstream redistributors who are patching pip be the ones that hold a patch for these cases; because the cause of the issue is something that they've done.

I understand your logic there. The blame attached with it isn't entirely deserved, though. Yes, we should probably get our setuptools in Debian debundled, too. However, in Debian, pip and setuptools don't have the same groups of maintainers, so I can't trivially fix both. But, back to practical matters:

pip provides has a mechanism for debundling the dependencies. I know this isn't recommended by pip upstream, as it carries more risk of incompatibility. However, carrying it in pip upstream has the advantage of letting the mechanism be used by all distributions, in predictable ways, rather than everyone solving this with their own set of hacks.
I'd recommend taking patches to support DEBUNDLED = True operation, upstream.

Even better would be running CI on DEBUNDLED = True. I'm happy to do some work to make that happen.

[pfmoore]

The blame attached with it isn't entirely deserved, though.

I don't view it as a matter of blame, more a case that it's more likely that the people doing the debundling understand what and where needs patching than the people who don't debundle and don't test the debundled version. So ultimately by having the people doing the debundling carry the necessary patches, we get a better end user experience.

However, carrying it in pip upstream has the advantage of letting the mechanism be used by all distributions, in predictable ways, rather than everyone solving this with their own set of hacks.

Equally, though, "someone" (the proverbial "someone" 🙂) could maintain a project that centralised the debundling patches in a distribution-independent way - it doesn't have to be the pip devs that do that.

I'd recommend taking patches to support DEBUNDLED = True operation, upstream.
Even better would be running CI on DEBUNDLED = True. I'm happy to do some work to make that happen.

But please remember, we don't actually want people to debundle. It adds extra maintenance effort on the pip team (even if that effort is just closing bugs saying "not our problem, go and complain to your distro"). See the documentation for the reasoning - but I assume you've probably already read this.

Also, running with DEBUNDLED=true needs a fiddly set of steps to set up (and that's not something we're interested in making smoother). We're not going to try to automate that process so we can run it under CI, that's exactly the sort of thing we expect the distributions who debundle to do.

🤷 I basically take the same view as @pradyunsg - I don't like spending pip developer effort on the debundling scenario, and I'd rather we didn't. I won't spend any of my time on it. But I'm not going to make a crusade of it - if other pip devs want to work on this, I'm not going to stop them.

[pradyunsg]

@stefanor I just saw your offer on the Python on Debian Gist! (I'd unsubscribed from that Gist, because... well, the heavy weights from various committees were involved and I'm a squirrel with no fancy titles :P)

If weekends work for you, would you like to pick a slot on https://calendly.com/pradyunsg? If not, drop me a line on [email protected] and we can arrange something over email. :)

[pradyunsg]

I started typing a response here, and then it became very long, and then I realised that I should really involve more people and then (snip some thoughts), so... I've filed #9677. :)

[eli-schwartz]

to avoid issues from a poorly-debundled downstream

The blame attached with it isn't entirely deserved, though. Yes, we should probably get our setuptools in Debian debundled, too. However, in Debian, pip and setuptools don't have the same groups of maintainers, so I can't trivially fix both.

I recommend the following steps:

1. do not devendor pip
2. submit bug report for debian's setuptools package, to devendor it
3. wait for the setuptools bug to be resolved
4. devendor pip

I don't see why Debian bureaucratic issues necessitates increasing the pip upstream workload, if it doesn't result in better code.

And, speaking as a packager from a distro that devendors both, I wouldn't expect devendoring only one of them to work.

You may also wish to consider inspiration from https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/python-setuptools#n28

More generally, this is a repeat of #5429, because setuptools and its .extern logic is strictly inferior to pip's _vendor/__init__.py even if you do devendor it... and this sort of issue might unexpectedly crop up again and again, so setuptools needs to solve this properly... or you need to patch setuptools like we do.

So again, speaking from the distro side of things, I'd NACK this patch. However, I am not affiliated with pypa/pip, so take that with a grain of salt...

[eli-schwartz]

Apparently the linked issue is not debian specific after all... It is, however, due to emulating those same conditions on arch. I would not consider it safe to install package "A" which depends on "B" from my distro, then install my own copy of "B", and still expect the distro copy of "A" to work. It could easily break.

Not sure what the best solution to that is, though.

[uranusjr]

I’m going to just merge this since the change it causes a regression is practically zero, and the benefit is there.

[doko42]

thanks for sorting out the devendoring issues. Not sure if this bug report is the correct place, but there's another area why at least Debian and Ubuntu patch distutils and setuptools, and now also seen in #9617.

The problem that I'm trying to address: Users of a Linux distro (users, not python developers) can break their system by calling

sudo python3 setup.py install
sudo pip install

overriding packages installed with the distro package manager. The implemented solution (at least for Debian) is to have all modules installed by the distro package manager into a directory /usr/lib/python3/dist-packages, and to re-direct installations of "sudo python3 setup.py install" to /usr/local/lib/python3.9/dist-packages.

Not only seen with Debian or Ubuntu, but also here:
https://fedoraproject.org/wiki/Changes/Making_sudo_pip_safe

I'll reply on #9617 as well, but maybe there would be a better place to discuss about a safe "sudo pip install".