search

pypa / installer

A low-level library for installing from a Python wheel distribution.
https://installer.readthedocs.io/
MIT License
126 stars 51 forks source link

Separate validation of `RECORD` #147

Closed BlueGlassBlock closed 1 year ago

BlueGlassBlock commented 1 year ago

This PR adds validate_record method to WheelSource and a boolean parameter validate_record to install function.

Currently, validate_record implementation of WheelFile checks the presence of RECORD, entries of files in the wheel, and whether there's a corresponding hash for the entry. (RECORD.p7s and RECORD.jwt are ignored from here)

validate_record parameter of install indicates whether WheelSource.validate_record will be called during installation.

I've already modified the fancy_wheel builder from conftest.py to include hashes (and sizes) of files.

The names and places are quite different from #100 and #105, please inform me about what I need to change.

Please review and request changes so that I can push this PR forward 😄

The line ending seems messed up, so please squash when it's ready for merge

pradyunsg commented 1 year ago

Thanks for this PR @BlueGlassBlock, and appreciate your patience on this.

I'm doing a brain-dump TBH, so apologies for the somewhat unpolished phrasing.

Instintively, I feel like it's valuable to have validation live outside of WheelSource, keeping the the actual "details" of how to perform the validation outside of the wheel itself. But, let's check whether that's valid. The motivation for that was to let the tool that holds the wheel to not rely on installer. get_contents already requires the ability to figure out the relevant RECORD for a file entry, so WheelSource already needs that. wheel even has a function meant for this implemented on their end, so... ok... this makes sense as a method in WheelSource. It still shouldn't raise an error defined within from installer tho -- at best, this should be WheelSource.validation_error (like PEP 517's UnsupportedOperation).

BlueGlassBlock commented 1 year ago

Modified quite a bit. Please review and tell me what to change :)

BlueGlassBlock commented 1 year ago

Synced and changed. :)

BlueGlassBlock commented 1 year ago

Oh, maybe the CLI should be updated too? But I think that should be separated into another PR.

pradyunsg commented 1 year ago

Could you remove 14cefe1? I'd prefer to handle that in a separate PR. :)

BlueGlassBlock commented 1 year ago

Force pushed. That commit is saved on another branch.