conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
456 stars 101 forks source link

Add support for pip hash checking #629

Closed sfinkens closed 2 months ago

sfinkens commented 2 months ago

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
  2. 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?
netlify[bot] commented 2 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 96c132698cebc7c66934eb0127f50848b4ca2db4
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.

maresb commented 2 months ago

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 commented 2 months ago
  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.

maresb commented 2 months ago

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 commented 2 months ago

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

Good point! :+1: