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

Address #5874: Prefer candidates with allowed hashes #6699

Merged
merged 5 commits into from
Jul 14, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Jul 11, 2019

This is a follow-on to PR #6687 that addresses issue #5874 (and the same issue #3634), which is to prefer candidates for install that match a user-provided hash (aka the allowed hashes).

PR #6687 makes this PR possible because it added support for creating a new CandidateEvaluator for each Requirement (since the allowed hashes can vary per requirement).

This PR uses an approach different from PR #6464. Instead of filtering the candidates post hoc inside get_best_candidate() and after sorting by preference, the approach in this PR uses index.py's concept of "applicable" candidates combined with using the sort preference itself to do the choosing. First, it uses the allowed hashes to determine which candidates are applicable: if at least one link has a matching hash, then links with non-matching hashes are considered not applicable and filtered out of consideration (and links with matching hashes or no hash are applicable and so are kept). Otherwise, if no link has a matching hash, the list of applicable links is kept the same and no links are filtered out for hash reasons. Then, in the phase of sorting applicable candidates, links with matching hashes are preferred over links whose hash doesn't match (or that don't have a hash).

In a follow-on PR, I can implement @dstufft's suggestion of logging a warning if a link is more preferred but has no hash listed: #5874 (comment)

@cjerdonek cjerdonek force-pushed the issue-5874-hash-checking branch from f44912a to ebff940 Compare July 11, 2019 09:30
@xavfernandez
Copy link
Member

Nice and clear 👍

@cjerdonek
Copy link
Member Author

Thanks, @xavfernandez! 😊

@cjerdonek cjerdonek force-pushed the issue-5874-hash-checking branch from ebff940 to 74504ff Compare July 14, 2019 09:53
@cjerdonek
Copy link
Member Author

Merging so I can work on a couple nice-to-have follow-ups to this.

@cjerdonek cjerdonek merged commit 2c36f4d into pypa:master Jul 14, 2019
@cjerdonek cjerdonek deleted the issue-5874-hash-checking branch July 14, 2019 16:10
@cjerdonek cjerdonek added C: finder PackageFinder and index related code C: requirement file Using `requirements.txt` type: enhancement Improvements to functionality labels Jul 14, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: finder PackageFinder and index related code C: requirement file Using `requirements.txt` type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants