Fix parsing of JSON index dist-info-metadata values #12078

pfmoore · 2023-06-07T19:59:40Z

Fixes #12042

dstufft

I assume this is just fixing the pip bug, and not a draft of PEP 714, so with that assumption this looks fine to me.

dstufft · 2023-06-07T20:11:49Z

src/pip/_internal/models/link.py

+
+    hashes: Optional[dict[str, str]]
+
+    # TODO: Do we care about stripping out unsupported hash methods?


I don't have a strong preference since the values in _SUPPORTED_HASHES look fine to me (though it might be time to retire md5 from that list). In theory people can use a number of other hashes that this method may not pick up, but I think that can/should be handled as a separate task of maintaining the list of supported hashes.

Seems reasonable. This is mostly about handling garbage produced by incorrect servers (stuff like "sah256=abcbcba") than about limiting what hashes we use.

The attribute is already a weird tri-state thing (None = no data supplied, MetadataFile(None) = supplied with no hashes, and the full thing with hashes. There's also a dodgy MetadataFile({}) which we get if we end up stripping everything, which is probably like `MetadataFile(None), but consumers might not like an empty dict of hashes.

I think what I should probably do is validate the input up front, rather than in this class. Then we just assert here that hashes must be None or a dictionary containing only one or more supported hash types.

pfmoore · 2023-06-07T20:26:34Z

I assume this is just fixing the pip bug, and not a draft of PEP 714

At the moment, yes. Once I have the bug fixed, it will be easy to rename the attribute¹. I will do this in the same PR, as the bug fix is of little value without the renaming.

Technically, prefer the new name with a fallback to the old one 🙂 ↩

pfmoore · 2023-06-08T09:28:43Z

OK, PEP 714 has now been included in this. Once CI has passed, it's ready for any final review.

It shouldn't be merged until PEP 714 has been accepted, so I'll leave it as draft until then.

pfmoore · 2023-06-27T09:39:40Z

I'm confused. Lint works fine locally for me, even though pre-commit.ci fails in CI. (And if I make the fixes CI wants, local lint fails 🙁)

What should I do? Is nox -s lint no longer the correct lint command for local checking? If it is, where should I look for hidden state? I've deleted my whole .nox directory and that doesn't help.

Ping @pradyunsg as I think the pre-commit.ci stuff was added by you?

pfmoore · 2023-06-27T09:58:36Z

It looks like pre-commit.ci has been failing for some time, including on main: https://results.pre-commit.ci/repo/github/1446467

I'm a little annoyed with pre-commit.ci if they don't produce the same results as a local run of pre-commit 🙁

pradyunsg · 2023-06-27T11:29:41Z

It should behave the same as running it locally.

pradyunsg · 2023-06-27T11:39:29Z

❯ nox -s lint -- mypy
nox > Running session lint
nox > Re-using existing virtual environment at .nox/lint.
nox > python -m pip install pre-commit
nox > pre-commit run mypy --all-files
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

tests/unit/test_network_auth.py:196: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring)  # type: igno...
        ^
tests/unit/test_network_auth.py:208: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring)  # type: igno...
        ^
tests/unit/test_network_auth.py:224: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring)  # type: igno...
        ^
tests/unit/test_network_auth.py:245: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring)  # type: igno...
        ^
tests/unit/test_network_auth.py:281: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring)  # type: igno...
        ^
tests/unit/test_network_auth.py:357: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", KeyringModuleV2())  # ...
        ^
tests/unit/test_network_auth.py:381: error: Unused "type: ignore" comment
        monkeypatch.setitem(sys.modules, "keyring", keyring_broken)  # typ...
        ^

Yea, it does. I wonder if there's some sort of state on your end -- does running pre-commit clean followed by nox -s lint work?

(if you don't have pre-commit already installed, you can run the lint job and then use the CLI from .nox/lint/[Scripts or bin]/pre-commit to run it)

pradyunsg

LGTM, with a nit-pick.

pradyunsg · 2023-06-27T11:41:23Z

src/pip/_internal/models/link.py

+    if len(hashes) > 0:
+        return hashes
+    return None


nit:

Suggested change

if len(hashes) > 0:

return hashes

return None

if not hashes:

return None

return hashes

Sightly less C-like, and easier to read. :)

Fixed, thanks (commit incoming once I get pre-commit to behave).

pfmoore · 2023-06-27T13:46:47Z

I wonder if there's some sort of state on your end -- does running pre-commit clean followed by nox -s lint work?

Yes, it does. All sorted now. It's frustrating I can't immediately see where pre-commit is holding that state. I looked, in the project directory and in my home directory, but there wasn't anything obvious. I must admit I didn't look for a subcommand to do it.

Also, how have we had main failing with lint checks and not noticed? It looks like we don't have any particularly visible indicator that main is passing CI (I assume we do run CI against main...) Is that all it is, we don't make the results of CI against main visible so no-one noticed?

pfmoore · 2023-06-27T16:00:17Z

I'm getting really annoyed with that RAM disk failure...

pfmoore · 2023-06-27T20:51:11Z

Phew! Randomly re-running the tests FTW...

pfmoore added 2 commits June 7, 2023 20:58

Fix parsing of JSON index dist-info-metadata values

6c3db09

Add a news file

cc554ed

psf-chronographer bot added the bot:chronographer:provided label Jun 7, 2023

pfmoore added 2 commits June 7, 2023 21:11

Fix types to be 3.7-compatible

8f89997

Fix bad test data in test_parse_links_json

cfb4923

dstufft approved these changes Jun 7, 2023

View reviewed changes

pfmoore added 2 commits June 7, 2023 21:44

Missed a change to one of the tests

93b274e

Parse hash data before passing to MetadataFile

232cc9d

pfmoore force-pushed the core_metadata branch from 3f78ad9 to 232cc9d Compare June 8, 2023 09:09

Implement PEP 714 - rename dist-info-metadata

5168881

pfmoore marked this pull request as ready for review June 8, 2023 09:26

pfmoore marked this pull request as draft June 8, 2023 09:28

Merge branch 'main' into core_metadata

7e3f74f

pfmoore mentioned this pull request Jun 15, 2023

use .metadata distribution info when possible #11512

Closed

Merge branch 'main' into core_metadata

97cc6a4

pradyunsg approved these changes Jun 27, 2023

View reviewed changes

pradyunsg mentioned this pull request Jun 27, 2023

pip downloads packages twice if the data-dist-info-metadata is set #11847

Closed

1 task

pfmoore added 2 commits June 27, 2023 14:47

Fix lint issues

a0976d8

Reword the check for no hashes

c7daa07

pfmoore marked this pull request as ready for review June 27, 2023 13:47

pfmoore mentioned this pull request Jun 27, 2023

Revert "xfail test_pip_wheel_ext_module_with_tmpdir_inside" #12109

Merged

Merge branch 'main' into core_metadata

a934f38

pfmoore closed this Jun 27, 2023

pfmoore reopened this Jun 27, 2023

pfmoore closed this Jun 27, 2023

pfmoore reopened this Jun 27, 2023

pfmoore merged commit 778b42f into pypa:main Jun 27, 2023

pfmoore deleted the core_metadata branch June 27, 2023 20:51

github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing of JSON index dist-info-metadata values #12078

Fix parsing of JSON index dist-info-metadata values #12078

pfmoore commented Jun 7, 2023

dstufft left a comment

dstufft Jun 7, 2023

pfmoore Jun 7, 2023

pfmoore commented Jun 7, 2023

pfmoore commented Jun 8, 2023

pfmoore commented Jun 27, 2023 •

edited

Loading

pfmoore commented Jun 27, 2023

pradyunsg commented Jun 27, 2023

pradyunsg commented Jun 27, 2023

pradyunsg left a comment

pradyunsg Jun 27, 2023

pfmoore Jun 27, 2023

pfmoore commented Jun 27, 2023

pfmoore commented Jun 27, 2023

pfmoore commented Jun 27, 2023


		hashes: Optional[dict[str, str]]

		# TODO: Do we care about stripping out unsupported hash methods?

Fix parsing of JSON index dist-info-metadata values #12078

Fix parsing of JSON index dist-info-metadata values #12078

Conversation

pfmoore commented Jun 7, 2023

dstufft left a comment

Choose a reason for hiding this comment

dstufft Jun 7, 2023

Choose a reason for hiding this comment

pfmoore Jun 7, 2023

Choose a reason for hiding this comment

pfmoore commented Jun 7, 2023

Footnotes

pfmoore commented Jun 8, 2023

pfmoore commented Jun 27, 2023 • edited Loading

pfmoore commented Jun 27, 2023

pradyunsg commented Jun 27, 2023

pradyunsg commented Jun 27, 2023

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg Jun 27, 2023

Choose a reason for hiding this comment

pfmoore Jun 27, 2023

Choose a reason for hiding this comment

pfmoore commented Jun 27, 2023

pfmoore commented Jun 27, 2023

pfmoore commented Jun 27, 2023

pfmoore commented Jun 27, 2023 •

edited

Loading