-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix #5874: Filter out candidates with hash mismatches. #6464
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
Closed
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c9065db
Fix #5874: Filter out candidates with hash mismatches.
alexbecker bcc8de3
handle candidates with no hash in their URL
alexbecker b704238
fix fake hash causing broken test
alexbecker a972286
fix mypy import error
alexbecker 2785d08
only warn when best candidate is skipped due to hash check
alexbecker 289e75e
remove local function test_against_hashes, it does not add clarity
alexbecker a6ad9a6
move filtering into get_best_candidate; remove get_best_candidates
alexbecker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Only select candidates in hash-checking mode that will pass hash checks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually, there should be a difference between supplying
--require-hashes
(i.e. hash mode with an empty hash list) and not (not hash mode).candidates.get_best()
should only receieve the hash comparer in hash mode, andNone
otherwise (andget_best
probably needs to check forNone
instead of a falsy hash comparer).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
get_best
should not filter on falsyhashes
. If I supply--require-hashes
without any hashes and filter onhashes
, this filters out all candidates and results in the following error:If I don't filter on falsy
hashes
then using--require-hashes
without any hashes results in:I think the latter is much more helpful, and clearly some work went into it via the
MissingHashes
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although perhaps this is a better place to implement
MissingHashes
logic?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried replacing
MissingHashes
with araise
inget_best
. It works, except that it makes the error about missing hashes take precedence over the error about not having versions pinned. Currently not having versions pinned in hash mode takes precedence. I am not sure whether this order of precedence is intentional.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it’s a good idea to use
MissingHashes
here (but I don’t think you can just move it here?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer to not filter, with a comment about how if
hashes
isNone
, an error will be raised inunpack_url
which explains what hash to pin to. If desired, in a future PR I could refactor awayMissingHashes
and raise the same error inget_best
instead (which is what I meant by "perhaps this is a better place to implementMissingHashes
logic"). But I'd rather not mix refactoring with feature work, and it would require some care to get the order of precedence for error messages right (there's a lot of error handling logic right now inRequirementPreparer.prepare_linked_requirement
which would need to get moved around).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a plan how the refactoring would be done? If so, maybe it would be better to refactor first, and implement the feature on top of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that such a refactoring has a downside the current implementation of
MissingHashes
will provide the correct hash to the user even if there is no remote hash, whereas if I were to move theraise
intoget_best
it would happen before the candidate is downloaded and so I could only provide the hash to the user if it was provided by the index. So I think it is not worth doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright then, let’s work on what we have and improve (if possible) in the future.