Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix artifact URL recording for pip>=23.3. #2421

Merged
merged 5 commits into from
Jun 8, 2024
Merged

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jun 7, 2024

When support for Pip 23.3.1 was added in #2276 a latent bug in artifact
URL recording was exposed in cases where the index being used issued
re-directs. Fix up artifact URL recording to grab the primary index URL
and not subsequent re-directs.

Implementing the fix above led to a test failure that revealed another
bug whereby lock file artifact downloads were not respecting the locked
resolve target when it was a foreign platform, which is now fixed.

Finally, fixing the un-patched foreign platform target issue in lock
file artifact downloads revealed that artifact URLs with hashes were not
being taken advantage of in all cases. Now, when there is a version of
an artifact URL seen that contains hashes - the best of those is always
used to prevent needless post-processing to download and hash the
artifact at lock creation time.

Fixes #2414

@jsirois jsirois force-pushed the issues/2414 branch 2 times, most recently from 2e007a3 to 3e3f9b1 Compare June 7, 2024 20:31
The needed patches were only being applied for universal lock creation and not
for foreign (abbreviated or complete) platform lock creation.
…always applied to resolved requirements in a lock.
@jsirois jsirois requested review from zmanji, benjyw and huonw June 7, 2024 22:30
@jsirois jsirois marked this pull request as ready for review June 7, 2024 22:30
locker.analysis_completed()


@pytest.mark.parametrize("pip_version", ["23.2", "23.3.1"])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre fix commits this ran like:

:; tox -epy311-integration -- --devpi -vvsk test_issue_2414 -n0
...
tests/integration/cli/commands/test_issue_2414.py::test_redirects_dont_stomp_original_index_urls[23.2] PASSED
tests/integration/cli/commands/test_issue_2414.py::test_redirects_dont_stomp_original_index_urls[23.3.1] FAILED
...

Which is expected as per the OP in #2414. The failure for the 23.3.1 case looked like:

...
           Full diff:
E           - (ResolvedRequirement(pin=Pin(project_name=ProjectName(raw='wheel', validated=False, normalized='wheel'), version=Version(raw='0.43.0', normalized='0.43')), artifact=PartialArtifact(url=ArtifactURL(raw_url='https://files.pythonhosted.org/packages/7d/cd/d7460c9a869b16c3dd4e1e403cce337df165368c71d6af229a74699622ce/wheel-0.43.0-py3-none-any.whl', download_url='https://files.pythonhosted.org/packages/7d/cd/d7460c9a869b16c3dd4e1e403cce337df165368c71d6af229a74699622ce/wheel-0.43.0-py3-none-any.whl', normalized_url='https://files.pythonhosted.org/packages/7d/cd/d7460c9a869b16c3dd4e1e403cce337df165368c71d6af229a74699622ce/wheel-0.43.0-py3-none-any.whl', scheme='https', path='/packages/7d/cd/d7460c9a869b16c3dd4e1e403cce337df165368c71d6af229a74699622ce/wheel-0.43.0-py3-none-any.whl', fragment_parameters={}, fingerprints=()), fingerprint=None, verified=False), additional_artifacts=(PartialArtifact(url=ArtifactURL(raw_url='https://m.devpi.net/root/pypi/%2Bf/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', download_url='https://m.devpi.net/root/pypi/%2Bf/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', normalized_url='https://m.devpi.net/root/pypi/+f/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', scheme='https', path='/root/pypi/+f/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', fragment_parameters={}, fingerprints=()), fingerprint=Fingerprint(algorithm='sha256', hash='465ef92c69fa5c5da2d1cf8ac40559a8c940886afcef87dcf14b9470862f1d85'), verified=False),)),)
E           + (ResolvedRequirement(pin=Pin(project_name=ProjectName(raw='wheel', validated=False, normalized='wheel'), version=Version(raw='0.43.0', normalized='0.43')), artifact=PartialArtifact(url=ArtifactURL(raw_url='https://m.devpi.net/root/pypi/%2Bf/55c/570405f142630/wheel-0.43.0-py3-none-any.whl', download_url='https://m.devpi.net/root/pypi/%2Bf/55c/570405f142630/wheel-0.43.0-py3-none-any.whl', normalized_url='https://m.devpi.net/root/pypi/+f/55c/570405f142630/wheel-0.43.0-py3-none-any.whl', scheme='https', path='/root/pypi/+f/55c/570405f142630/wheel-0.43.0-py3-none-any.whl', fragment_parameters={}, fingerprints=()), fingerprint=Fingerprint(algorithm='sha256', hash='55c570405f142630c6b9f72fe09d9b67cf1477fcf543ae5b8dcb1f5b7377da81'), verified=False), additional_artifacts=(PartialArtifact(url=ArtifactURL(raw_url='https://m.devpi.net/root/pypi/%2Bf/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', download_url='https://m.devpi.net/root/pypi/%2Bf/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', normalized_url='https://m.devpi.net/root/pypi/+f/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', scheme='https', path='/root/pypi/+f/465/ef92c69fa5c5d/wheel-0.43.0.tar.gz', fragment_parameters={}, fingerprints=()), fingerprint=Fingerprint(algorithm='sha256', hash='465ef92c69fa5c5da2d1cf8ac40559a8c940886afcef87dcf14b9470862f1d85'), verified=False),)),)

tests/integration/cli/commands/test_issue_2414.py:93: AssertionError
...

Note the whl URLs differed as in the OP in #2414 case with re-directed https://files.pythonhosted.org URLs vs the expected https://m.devpi.net URLs.

Copy link
Collaborator

@zmanji zmanji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests make sense to me so this lgtm.

@jsirois jsirois merged commit 1811be0 into pex-tool:main Jun 8, 2024
26 checks passed
@jsirois jsirois deleted the issues/2414 branch June 8, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression of artifact URL due to change since pip 23.3
2 participants