Support wheel cache when using --require-hashes #11897

sbidoul · 2023-03-25T13:49:13Z

@ewdurbin would you like to test this and try to break it?

I'm not doing it for the bounty, as this is also one of the things in my way of actually using hashes. So hopefully we'll share a 🍺 if our paths cross one day.

TODO

investigate how this impacts pip wheel (I think it works but the warning should be silenced)

ewdurbin · 2023-03-25T13:50:32Z

Exciting! I will do my very best to try to get pip built locally using this (I would honestly have attacked this myself if I knew how to get the dev env working 😂)

sbidoul · 2023-03-25T13:56:37Z

knew how to get the dev env working

Nothing fancy really: pip install -e . -r tests/requirements.txt in a venv goes a long way.

sbidoul · 2023-03-25T15:40:56Z

Combined with #11872, one can use --no-binary and benefit from caching and hash checking at the same time 🥳

news/5037.feature.rst

pfmoore · 2023-04-01T18:41:10Z

I'd like to get this in 23.1. @ewdurbin if you can confirm it fixes the issue within the next week or so, that would be excellent. Please ask for help if you are still unsure how to test against the version with the PR.

ewdurbin · 2023-04-01T18:42:22Z

Thanks for the ping. PyCon prep has me pretty slammed but I need to do some warehouse work in the next day or so. I'll try running from this branch to confirm it works in dev.

ewdurbin · 2023-04-03T10:09:14Z

knew how to get the dev env working

Nothing fancy really: pip install -e . -r tests/requirements.txt in a venv goes a long way.

Hmmm, I guess I was tricked by the tox.ini in the past and never found success.

Testing this now.

ewdurbin · 2023-04-03T13:06:35Z

Hmmmmm I must be holding it wrong, but with the refactor of our actions in https://github.com/pypi/warehouse/pull/13357/files I would have expected the pip cache to pick up the wheels from the new "deps" initial job, but no dice: https://github.com/pypi/warehouse/actions/runs/4596723814/jobs/8118550985

pradyunsg · 2023-04-03T14:29:10Z

Cache not found for input keys: pip-Linux

There was no cache available when pip ran, based on the output of the cache action prior.

ewdurbin · 2023-04-03T16:30:27Z

Cache not found for input keys: pip-Linux

There was no cache available when pip ran, based on the output of the cache action prior.

That was the first run, so there was no cache. The deps build step did create it:

Then the downstream build matrix was able to get it:

sbidoul · 2023-04-03T17:16:44Z

@ewdurbin I don't immediately understand what's wrong with your test.

I rebased this PR in case there is some interaction with some previously unmerged pip 23.1 change.

FWIW, I just tried this script which shows it works fine.

#!/bin/bash
set -eaux -o pipefail

VENV=$(mktemp -d)
python -m venv $VENV
source $VENV/bin/activate
pip install -U git+https://github.com/pypa/pip@refs/pull/11897/head

REQS=$(mktemp)
export PIP_CACHE_DIR=$(mktemp -d)
export PIP_NO_BINARY=:all:

cat <<EOF > $REQS
lxml==4.9.2 \
  --hash sha256:2455cfaeb7ac70338b3257f41e21f0724f4b5b0c0e7702da67ee6c3640835b67 \
  --hash sha256:7b515674acfdcadb0eb5d00d8a709868173acece5cb0be3dd165950cbfdf5409
EOF

echo "======= first install builds from source"
pip install -r $REQS

echo "======= reinstall gets wheel from cache"
pip install --force-reinstall -r $REQS

ewdurbin · 2023-04-03T17:50:30Z

@sbidoul seems rebasing was the trick! all deps were installed from cache 🎉

This is result expected and satisfies #5037 as I understand it.

sbidoul · 2023-04-07T20:27:37Z

This one is now ready.

sbidoul · 2023-04-10T11:51:45Z

src/pip/_internal/operations/prepare.py

+                    "and re-downloading source."
+                )
+                req.link = direct_url_as_link(req.download_info)
+                link = req.link


Hmm, I'm afraid this part still needs some work. If req.download_info contains an invalid hash, it re-downloads an URL with an invalid hash fragment, which means we never get a good cache entry.

Ok, I resolved this by adding a new cached_wheel_source_link field to InstallRequirement. I also renamed original_link_is_in_wheel_cache to is_wheel_from_cache to better reflect its meaning.

sbidoul · 2023-04-10T14:42:41Z

So this is ready again 🟢

sbidoul · 2023-04-13T07:38:52Z

I'm fairly confident this change is correct and safe. I'll be available post-release to handle any issue as quickly as possible.
In the next iteration I plan to follow-up with #11943 and #6469.

uranusjr · 2023-04-13T22:02:12Z

src/pip/_internal/operations/prepare.py

+    link: Link,
+    download_dir: str,
+    hashes: Optional[Hashes],
+    warn_on_hash_mismatch: bool = True,


Not some that needs to change, I’m just suddenly curious how peopel choose between positive and negative flags. Personally I’d probably choose to implement this as suppress_hash_mismatch_warning=False.

In this case I think I wanted to avoid a double negation and save half a brain cycle when reading the only condition using this flag.

uranusjr · 2023-04-13T22:06:24Z

src/pip/_internal/req/req_install.py

+        # When is_wheel_from_cache is True, it means that this InstallRequirement
+        # is a local wheel file in the cache of locally built wheels.
+        self.is_wheel_from_cache = False
+        # When is_wheel_from_cache is True, this is the source link corresponding
+        # to the cache entry, which was used to download and build the cached wheel.
+        self.cached_wheel_source_link: Optional[Link] = None


Do we need two attributes for this? From what I can tell these are always set together and is_wheel_from_cache can be entirely implied by cached_wheel_source_link is not None.

This is indeed a logical next step in this refactoring. Done.

This more accurately reflects that it is not necessarily related to original_link, original_link being the direct URL link, and the wheel cache can also be populated from sdists URL discovered by the finder.

pfmoore · 2023-04-15T09:37:34Z

@sbidoul This is going to miss 23.1. If you want to get it included in that release, can you do anything in the next hour or two? Otherwise, I'll push it to 23.2.

pfmoore · 2023-04-15T09:41:01Z

Wow, that was quick! Thanks.

psf-chronographer bot added the bot:chronographer:provided label Mar 25, 2023

sbidoul force-pushed the cache-hash-checking-sbi branch 2 times, most recently from 9abf6b0 to a1c78e5 Compare March 25, 2023 15:37

sbidoul added this to the 23.1 milestone Mar 25, 2023

sbidoul marked this pull request as ready for review March 25, 2023 17:35

sbidoul force-pushed the cache-hash-checking-sbi branch from bd3673f to 76d8341 Compare March 27, 2023 14:25

uranusjr reviewed Mar 29, 2023

View reviewed changes

news/5037.feature.rst Outdated Show resolved Hide resolved

sbidoul force-pushed the cache-hash-checking-sbi branch from 6e3e346 to 7fb8feb Compare April 3, 2023 17:07

sbidoul mentioned this pull request Apr 4, 2023

Refactor checking of hashes embedded in user-provided URLs #11923

Draft

sbidoul commented Apr 10, 2023

View reviewed changes

sbidoul mentioned this pull request Apr 10, 2023

Have pip cache purge and pip cache remove handle empty directories. #9058

Open

5 tasks

sbidoul force-pushed the cache-hash-checking-sbi branch 3 times, most recently from 16b2aa8 to 8abc7e9 Compare April 10, 2023 13:27

sbidoul force-pushed the cache-hash-checking-sbi branch from b0c5edf to 2a471a7 Compare April 11, 2023 12:17

uranusjr reviewed Apr 13, 2023

View reviewed changes

sbidoul and others added 11 commits April 14, 2023 08:03

Add function to check hashes against known digests

8e2205d

Check hashes of cached built wheels agains origin source archive

40cd79d

Improve pip wheel wrt hash checking of cached built wheels

0e2a0db

Tweak a condition

a1af13c

Fix rst syntax

f86ba46

Cosmetics

ff8c8e3

Rename original_link_is_in_wheel_cache to is_wheel_from_cache

a6ef648

This more accurately reflects that it is not necessarily related to original_link, original_link being the direct URL link, and the wheel cache can also be populated from sdists URL discovered by the finder.

Add a couple of asserts

caafe6e

Introduce ireq.cached_wheel_source_link

bd746e3

Improve test

4beca6b

Further refactor is_wheel_from_cache

efe2d27

sbidoul force-pushed the cache-hash-checking-sbi branch from 2a471a7 to efe2d27 Compare April 14, 2023 06:13

sbidoul merged commit dbf4e68 into pypa:main Apr 15, 2023

sbidoul deleted the cache-hash-checking-sbi branch April 15, 2023 09:38

sbidoul mentioned this pull request Apr 16, 2023

Update docs about hash checking and wheel cache #11967

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support wheel cache when using --require-hashes #11897

Support wheel cache when using --require-hashes #11897

sbidoul commented Mar 25, 2023 •

edited

Loading

ewdurbin commented Mar 25, 2023

sbidoul commented Mar 25, 2023

sbidoul commented Mar 25, 2023

pfmoore commented Apr 1, 2023

ewdurbin commented Apr 1, 2023

ewdurbin commented Apr 3, 2023

ewdurbin commented Apr 3, 2023

pradyunsg commented Apr 3, 2023

ewdurbin commented Apr 3, 2023

sbidoul commented Apr 3, 2023 •

edited

Loading

ewdurbin commented Apr 3, 2023

sbidoul commented Apr 7, 2023

sbidoul Apr 10, 2023

sbidoul Apr 10, 2023

sbidoul commented Apr 10, 2023

sbidoul commented Apr 13, 2023

uranusjr Apr 13, 2023

sbidoul Apr 14, 2023

uranusjr Apr 13, 2023

sbidoul Apr 14, 2023

pfmoore commented Apr 15, 2023

pfmoore commented Apr 15, 2023

Support wheel cache when using --require-hashes #11897

Support wheel cache when using --require-hashes #11897

Conversation

sbidoul commented Mar 25, 2023 • edited Loading

ewdurbin commented Mar 25, 2023

sbidoul commented Mar 25, 2023

sbidoul commented Mar 25, 2023

pfmoore commented Apr 1, 2023

ewdurbin commented Apr 1, 2023

ewdurbin commented Apr 3, 2023

ewdurbin commented Apr 3, 2023

pradyunsg commented Apr 3, 2023

ewdurbin commented Apr 3, 2023

sbidoul commented Apr 3, 2023 • edited Loading

ewdurbin commented Apr 3, 2023

sbidoul commented Apr 7, 2023

sbidoul Apr 10, 2023

Choose a reason for hiding this comment

sbidoul Apr 10, 2023

Choose a reason for hiding this comment

sbidoul commented Apr 10, 2023

sbidoul commented Apr 13, 2023

uranusjr Apr 13, 2023

Choose a reason for hiding this comment

sbidoul Apr 14, 2023

Choose a reason for hiding this comment

uranusjr Apr 13, 2023

Choose a reason for hiding this comment

sbidoul Apr 14, 2023

Choose a reason for hiding this comment

pfmoore commented Apr 15, 2023

pfmoore commented Apr 15, 2023

sbidoul commented Mar 25, 2023 •

edited

Loading

sbidoul commented Apr 3, 2023 •

edited

Loading