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

Add support for pip hash checking #629

Merged
merged 5 commits into from
Apr 26, 2024
Merged

Conversation

sfinkens
Copy link
Contributor

Description

Fix #624 by

  1. Adding support for pip's --hash option in the requirement parser, for example
- pip:
    - my_module === 1.2.3 --hash=sha256:1234
  1. Using that hash to create a lockfile

Questions

Is this going into the right direction? And some more specific questions:

  1. The unit test is mostly copied from other another test. Should this be factorized?
  2. This solution only works for a single hash. The documentation states that you can specify multiple hashes for the same package, for example for different platforms. As far as I can see this is not supported by conda-lock's hash model. But maybe it's not needed, because conda-lock supports different platforms anyway.
  3. I created two new subclasses which hold the hash, mostly because I didn't want to break anything. But I'm not sure if that's the best approach.
  4. I'm not sure how to resolve the mypy issue in pypi_solver.py (PoetryDependency not having the get_hash_model method). I agree with mypy that the solution is not really consistent, because some dependencies still carry the hash in their URL. Maybe PoetryURLDependency and PoetryVCSDependency could be extended with a get_hash_model method as well?

Copy link

netlify bot commented Apr 26, 2024

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 96c1326
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/662bc8a0d800b600082a96d0
😎 Deploy Preview https://deploy-preview-629--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sfinkens sfinkens changed the title Fix pip hashmode Add support for pip hash checking Apr 26, 2024
@sfinkens sfinkens marked this pull request as ready for review April 26, 2024 11:09
@sfinkens sfinkens requested a review from a team as a code owner April 26, 2024 11:09
@maresb
Copy link
Contributor

maresb commented Apr 26, 2024

Thanks so much @sfinkens for all your work on this! I like your approach.

  1. It's fine as-is. I would really like to get the test suite refactored, and I'd be very grateful for any help you could provide. But I see that as a different issue.
  2. I think it's fine for now. We could support it later if there's demand for it, but I suspect it isn't worth the effort.
  3. Looks good to me.
  4. As for the mypy issue in particular, I think all you need to do is to inline _dependency_provides_hash. Then you should be solid from a typing perspective. Could you be more specific about the inconsistency you're concerned about? (I'm sure you're thinking more deeply that I am on this subject.)

@sfinkens
Copy link
Contributor Author

  1. Ok, I'll put that on my "spare time" list :)
  2. Agreed!
  3. Ok!
  4. In pypi_solver.get_requirements() there's an if block
def get_requirements():
    if op.package.source_type == "url":
        hash = ...  # variant 1
    elif op.package.source_type == "git":
        hash = ...  # variant 2
    else:
        hash = ...  # variant 3

and I was wondering if that logic could be delegated to the corresponding dependency classes

def get_requirements():
    hash = op.package.dependency.get_hash_model()
    # ...

class PoetryURLDependency:
    def get_hash_model()
        # variant 1

class PoetryVCSDependency:
    def get_hash_model():
        # variant 2

class PoetryDependencyWithHash:
    def get_hash_model():
         # variant 3

We couldn't get rid of the if block entirely, but maybe make it a bit smaller. Then mypy wouldn't complain anymore, because every dependency class provides that method. But now that I think about it, that's maybe beyond the scope of this PR.

conda_lock/pypi_solver.py Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor

maresb commented Apr 26, 2024

Regarding 4. I find the if nice and explicit. Unless there's a really clean and obvious class hierarchy, tucking away logic into class methods seems unnecessarily indirect. Since this is all a bit of a hack, I'd rather have that hack in one place upfront in the if block, if that makes any sense. :)

@sfinkens
Copy link
Contributor Author

sfinkens commented Apr 26, 2024

Since this is all a bit of a hack, I'd rather have that hack in one place

Good point! 👍

@maresb maresb merged commit 0e63d0e into conda:main Apr 26, 2024
10 checks passed
@maresb maresb mentioned this pull request May 15, 2024
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.

Error creating lockfile from rendered environment
2 participants