Repository navigation
pip parses dist-info-metadata key from JSON indexes incorrectly #12042
Description
Description
When using a JSON-based package index, pip expects the dist-info-metadata key to always have a string value and does not parse the values described by the spec.
dist-info-metadata: An optional key that indicates that metadata for this file is available, via the same location as specified in PEP 658 ({file_url}.metadata). Where this is present, it MUST be either a boolean to indicate if the file has an associated metadata file, or a dictionary mapping hash names to a hex encoded digest of the metadata’s hash.
pip tries to apply the same string parsing for this value as it does for the data-dist-info-metadata value in PEP 658 and does not handle booleans or dicts.
This does not work as the entries according to PEP 691 look like:
{
"dist-info-metadata": {
"sha256": "8737b5dbb73f6b05e596c7c659f694a031de98b926e634716d9855fd365a5f2f"
},
"filename": "pre_commit-3.3.2-py2.py3-none-any.whl",
[...]
},Expected behavior
pip should handle values with boolean or dict type as described in PEP 691.
pip version
Tested with 23.1.2 and current main
Python version
3.11
OS
Linux (debian bullseye)
How to Reproduce
To reproduce, we need access to a registry which supports PEP 691 JSON indexes and also PEP 658 metadata.
pypi.org is supposed to support both of these for new file uploads, but there is currently a bug where it provides the metadata under the wrong JSON key (pypi/warehouse#13705), so it is hard to test at the moment. For now, the easiest way to reproduce is to temporarily patch your pip code to use the (incorrect) key to trigger the bug.
-
Temporarily patch your
pipcode so that it uses the key name provided by pypi.org:diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index e741c3283..5d1e0be4e 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -262,7 +262,7 @@ class Link(KeyBasedCompareMixin): url = _ensure_quoted_url(urllib.parse.urljoin(page_url, file_url)) pyrequire = file_data.get("requires-python") yanked_reason = file_data.get("yanked") - dist_info_metadata = file_data.get("dist-info-metadata") + dist_info_metadata = file_data.get("data-dist-info-metadata") hashes = file_data.get("hashes", {}) # The Link.yanked_reason expects an empty string instead of a boolean.
-
Try to install a package.
pip install --no-cache --only-binary :all: pre-commit==3.3.2to reproduce pip trying to parse a metadata dict as a stringpip install --no-cache --only-binary :all: pre-commit==3.0.0to reproduce pip trying to parse a booleanfalseas a string
Output
$ pip install --no-cache --only-binary :all: pre-commit==3.3.2
Collecting pre-commit==3.3.2
ERROR: Exception:
Traceback (most recent call last):
File "/home/ckuehl/proj/pip/src/pip/_internal/cli/base_command.py", line 169, in exc_logging_wrapper
status = run_func(*args)
^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/cli/req_command.py", line 248, in wrapper
return func(self, options, args)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/commands/install.py", line 377, in run
requirement_set = resolver.resolve(
^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/resolver.py", line 92, in resolve
result = self._result = resolver.resolve(
^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_vendor/resolvelib/resolvers.py", line 546, in resolve
state = resolution.resolve(requirements, max_rounds=max_rounds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_vendor/resolvelib/resolvers.py", line 397, in resolve
self._add_to_criteria(self.state.criteria, r, parent=None)
File "/home/ckuehl/proj/pip/src/pip/_vendor/resolvelib/resolvers.py", line 173, in _add_to_criteria
if not criterion.candidates:
File "/home/ckuehl/proj/pip/src/pip/_vendor/resolvelib/structs.py", line 156, in __bool__
return bool(self._sequence)
^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/found_candidates.py", line 155, in __bool__
return any(self)
^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/found_candidates.py", line 143, in <genexpr>
return (c for c in iterator if id(c) not in self._incompatible_ids)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/found_candidates.py", line 47, in _iter_built
candidate = func()
^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/factory.py", line 206, in _make_candidate_from_link
self._link_candidate_cache[link] = LinkCandidate(
^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 293, in __init__
super().__init__(
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 156, in __init__
self.dist = self._prepare()
^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 225, in _prepare
dist = self._prepare_distribution()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 304, in _prepare_distribution
return preparer.prepare_linked_requirement(self._ireq, parallel_builds=True)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/operations/prepare.py", line 510, in prepare_linked_requirement
metadata_dist = self._fetch_metadata_only(req)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/operations/prepare.py", line 374, in _fetch_metadata_only
return self._fetch_metadata_using_link_data_attr(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/operations/prepare.py", line 384, in _fetch_metadata_using_link_data_attr
metadata_link = req.link.metadata_link()
^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/models/link.py", line 417, in metadata_link
metadata_link_hash = LinkHash.parse_pep658_hash(self.dist_info_metadata)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ckuehl/proj/pip/src/pip/_internal/models/link.py", line 77, in parse_pep658_hash
name, sep, value = dist_info_metadata.partition("=")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'partition'
Same traceback for pre-commit==3.0.0 except the last line is instead
AttributeError: 'bool' object has no attribute 'partition'
Code of Conduct
- I agree to follow the PSF Code of Conduct.
Activity
pfmoore commentedon May 21, 2023
This needs an urgent fix, as pip will start failing on any package with a PEP 658 metadata file when PyPI release their fix. @pypa/pip-committers any thoughts? Should we release an emergency bug fix mid-cycle? How do we communicate the issue (as every version of pip back to 22.something will fail)? Is there anything Warehouse could do to mitigate this?
I’m happy to release an emergency fix, but I’m nervous about how we test it sufficiently in advance of Warehouse releasing their fix. And I don’t know how we handle the rollout issue.
pfmoore commentedon May 21, 2023
On reflection, I was forgetting that PyPI haven't backfilled yet. Assuming they hold off on the backfill until this is fixed, the problem isn't as severe as I'd feared - it will only impact new wheel uploads. As a result, I think that an emergency release with advice that if people hit the issue, they have to upgrade pip should be sufficient. We'll still need to monitor the situation, and PyPI will need to consider how to co-ordinate the backfill with uptake of new pip versions, but we should be OK.
chriskuehl commentedon May 21, 2023
At the moment warehouse is setting
"data-dist-info-metadata": falsefor files which don't have the metadata, so effectively all packages are broken right now with current pip versions. But perhaps warehouse could be changed to omit the key when there is no metadata file instead of settingfalse?sbidoul commentedon May 21, 2023
Do we know if our current PEP 658 implementation was tested with any real-world (private) index ? Could we use the same to test these fixes?
About the rollout I'm not sure either. It looks like the pip fix has to be released before the pypi.org fix, since all installs would otherwise fail without workaround ?
In that case it might also be an exceptional situation where we need to release a bug fix for a non current version (it's in since 22.3, actually), since some users may not be in a position to upgrade to 23.1 just yet, due to the recent removal of
setup.py installsupport.pradyunsg commentedon May 21, 2023
I'm in agreement that backporting and releasing bugfixes for older versions is a good idea (all the way back to when this was released), and something that we should do. I also agree that we should cut emergency-style bugfix releases for this (as is implied).
I'll take a look into the fix for this today (I'm at the airport for an 8+ hr flight, and this seems like a good thing to fill my hours + keep me awake), and review our PEP 658 implementation overall.
I'm happy to help with the release management work for this as well, although I'm unlikely to have a lot of "definitely free" time next week.
pfmoore commentedon May 21, 2023
Ouch, that's bad. But assuming PyPI fix this (the key is optional, so they can fix it) the damage is significantly limited. Enough to not need backports? I don't know. I was only thinking in terms of a 23.1.3 bugfix, which I'm happy to do, but @sbidoul has a point. I don't want to commit myself to doing backports, though, as I'm not sure I'll have the free time for that.
pradyunsg commentedon May 21, 2023
FWIW, the key has a typo right now. Once that typo is fixed, the pip bug will rear its head, so we should fix pip before that typo and I don't then pypi.org wouldn't need to do anything other than fix that typo.
pfmoore commentedon May 21, 2023
That seems reasonable, my only question is, to what extent do we care about users who haven't upgraded their copy of pip - is "there's a fix, you need to upgrade" enough?
notatallshaw commentedon May 21, 2023
Is there a workaround for users who don't upgrade Pip? Like a command line flag to avoid using the PEP 658 metadata file?
Just asking because there will be some noise from users if PyPi stops working even if upgrading to a bugfix is available, many users have strange setups that make things which should be easy not always easy.
pfmoore commentedon May 21, 2023
Basically no. That's the big problem here, if an index serves JSON, we'll use it in preference to HTML, and if they provide a
dist-info-metadatavalue we'll mis-parse it and fail. There's no flag that controls either of those two behaviours. And adding a flag is (of course) pointless, because the user could just as easily upgrade to a fixed version as to one with an opt-out.The best someone could do is write a service that proxies PyPI but without serving the JSON. But I'm not aware of anyone proposing to do that.
dstufft commentedon May 21, 2023
Just to be clear too, the above is true because of the specifics of this bug, but if pip preferred the HTML version, the same general problem could happen in reverse. There's nothing specific to JSON here other than the bug happens to be in the JSON API on PyPI.
This sort of bug is honestly a risk anytime there's some code path that isn't being used, that suddenly starts being used.
I think there just isn't a path forward for people who don't upgrade pip, even without the backfill sooner or later most projects are going to end up with releases with
dist-info-metadata.19 remaining items
dstufft commentedon Jun 7, 2023
You're not required to use all of them, PEP 691:
So you can just pick one of them to verify if that makes it easier.
pfmoore commentedon Jun 7, 2023
The infrastructure's all there to handle multiple hashes (the
Link.hashesattribute) it's just that the data structures used to parse thedist-info-metadatadon't handle multiples. BasicallyLink.metadata_linkis fine, it's the intermediateLinkHashstructure that's the limited one. I may just replace that class altogether. (I could just do something specifically for the JSON case, but that would be messy, and I want to keep the implementation clean, as I really don't want to introduce any new problems here).I'm also thinking we need some much better tests of this, so we can be sure we've actually fixed the issue before saying it's OK for PyPI to turn on their implementation.
Sorry, I shouldn't be trying to pair-program via GitHub issues 😉
dstufft commentedon Jun 7, 2023
The good news is we should be able to turn on PyPI's implementation of PEP 714 before pip lands their implementation, since the whole purpose of the strategy in the PEP is that it enables PyPI to land it without breaking the existing logic in pip. Not that much better tests would be a bad thing of course :) but I know how gnarly pip's code base can be to test too, so there's not a huge rush to land it in pip prior to PyPI anymore.
pfmoore commentedon Jun 7, 2023
Hmm, that's a good point and probably worth doing, when we get to that point. It makes me a lot more comfortable that I can check that I haven't made a dumb mistake before we unleash the pip fix on the world 🙂
dstufft commentedon Jun 27, 2023
PyPI's implementation of PEP 714 just landed thanks to @ewdurbin :)
pfmoore commentedon Jun 27, 2023
Pip's will land as soon as I can get past the *&^%$£$% RAM disk error on Windows CI.
chore(deps): bump pip from 22.3.1 to 23.2 in /.github/workflows (#73)
chore(deps): Update pip requirement from ~=23.1 to ~=23.2 in /package…
chore(deps-dev): Bump pip from 23.1.2 to 23.2 in /packages/jsii-pacma…
Bump pip from 23.1.2 to 23.2 (PR #6275)
Bump pip from 23.1.2 to 23.2 (PR #6279)
Bump pip from 23.1.2 to 23.2 in /.github/workflows (#22)
chore(deps): Bump pip from 23.1.2 to 23.2.1 (#1426)