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

Set the requirement context on hash error #8445

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

uranusjr
Copy link
Member

This improves the message shown by the hash error to include the requirement that caused it.

This partially addresses test_hashed_install_failure_later_flag. The behaviour is still different from the legacy resolver’s, which collects all hash errors and raise an aggregated HashErrors exception instead.

Before:

Hashes are required in --require-hashes mode, but they are missing from some requirements.
Here is a list of those requirements along with the hashes their downloaded archives actually had.
Add lines like these to your requirements files to prevent tampering. (If you did not enable
--require-hashes manually, note that it turns on automatically when any package has a hash.)

    unknown package --hash=sha256:9ee5702ccd5d16d54ac9dcfce4a7c8ac90020cfe4e6b494ca44c68492add364c

After:

Hashes are required in --require-hashes mode, but they are missing from some requirements.
Here is a list of those requirements along with the hashes their downloaded archives actually had.
Add lines like these to your requirements files to prevent tampering. (If you did not enable
--require-hashes manually, note that it turns on automatically when any package has a hash.)

    package-a==0.1.0 --hash=sha256:9ee5702ccd5d16d54ac9dcfce4a7c8ac90020cfe4e6b494ca44c68492add364c

This improves the message shown by the hash error to include the
requirement that caused it.
@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jun 16, 2020
@pradyunsg
Copy link
Member

Is there some reason we can't aggregate the hash errors? Or is it omitted to minimize implementation complexity?

@uranusjr
Copy link
Member Author

I guess it’s kind of both or neither. Currently the distribution is prepared lazily (when Candidate.dist is called), meaning that the hash error could literally be raised anywhere during the resolution, making aggregation difficult. It would require us to rethink how and when we call the preparer and significantly change the implementation.

@pfmoore
Copy link
Member

pfmoore commented Jun 18, 2020

I don't think we want to eagerly prepare the distribution just for this issue. It might be a good thing to do, but there's a risk we hurt performance by preparing stuff we don't need to. We don't have test cases that would allow us to easily check that.

I'd be OK with a follow-up PR specifically to investigate the impact of eagerly preparing. It's possible it was a premature optimisation.

@brainwane
Copy link
Contributor

@pfmoore @uranusjr @pradyunsg OK to go ahead and merge this, in the interests of moving along things from the "in-progress" column to "Done"? :-)

@McSinyx
Copy link
Contributor

McSinyx commented Jun 24, 2020

@pfmoore, per a private discussion with @pradyunsg yesterday, Candidate.dist (or the call to Candidate._prepare) is needed for 2 separate things

  1. The dependencies (Candidate.iter_dependencies) which is to be passed to resolvelib
  2. The distribution to be installed later on (for pip install) or to give the user (for pip wheel)

With GH-7819 in mind, we can acquire different methods to get (1) (e.g. shallow wheel, to-be-matured-JSON API), while (2) can be ensure after dependency resolution either automatically or manually getting Candidate.dist.

Now if hashing is required, in (1) what calls Candidate.iter_dependencies can aggregate the errors. My intention here is to ask for this to be merged as-is and defer the aggregation of HashError to be taken care after an realization GH-7819 is merged upstream.

Relating to AbstractDistribution preparation, may I please have some additional review over GH-8411?

@pfmoore
Copy link
Member

pfmoore commented Jun 24, 2020

@pradyunsg Gentle ping, this is awaiting your approval. I'm assuming from @McSinyx' comment, referring to a discussion that you had with them, that you're on board with sorting out aggregation later, but I'll let you confirm that explicitly.

@pradyunsg
Copy link
Member

I think that's context from a slightly different conversation.

@pradyunsg
Copy link
Member

Gentle ping, this is awaiting your approval.

I thought I'd already done this! >.<

Merging!

@pradyunsg pradyunsg merged commit 439e16f into pypa:master Jun 24, 2020
@pradyunsg pradyunsg removed their request for review June 24, 2020 13:11
@uranusjr uranusjr deleted the hash-error-message branch June 24, 2020 13:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants