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

Separate validation of RECORD #147

Merged
merged 12 commits into from
Dec 9, 2022
Merged

Separate validation of RECORD #147

merged 12 commits into from
Dec 9, 2022

Conversation

BlueGlassBlock
Copy link
Contributor

@BlueGlassBlock BlueGlassBlock commented Nov 13, 2022

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
Copy link
Member

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).

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing this PR @BlueGlassBlock! I really appreciate it.

I do have a few suggestions.

I think this is my first review on a PR by you so I'll clarify that any comment starting with nit: is something that I don't consider being worth an extended discussion. It's more of a nit-pick and I don't think it's worth having an extended discussion on that -- if you disagree with my suggestion or don't really wanna bother with that suggestion, please feel welcome to mark it as resolved without a discussion. :)

src/installer/sources.py Outdated Show resolved Hide resolved
src/installer/sources.py Outdated Show resolved Hide resolved
src/installer/_core.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_sources.py Outdated Show resolved Hide resolved
tests/test_sources.py Outdated Show resolved Hide resolved
tests/test_sources.py Outdated Show resolved Hide resolved
tests/test_sources.py Outdated Show resolved Hide resolved
Add corresponding doc to `WheelSource.validate`
switch to `validation_error` property
Fix `RECORD.jws`
@BlueGlassBlock
Copy link
Contributor Author

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

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing!

src/installer/_core.py Outdated Show resolved Hide resolved
@BlueGlassBlock
Copy link
Contributor Author

Synced and changed. :)

@BlueGlassBlock
Copy link
Contributor Author

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

@pradyunsg
Copy link
Member

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

@BlueGlassBlock
Copy link
Contributor Author

Force pushed. That commit is saved on another branch.

@pradyunsg pradyunsg enabled auto-merge (squash) December 9, 2022 12:37
@pradyunsg pradyunsg disabled auto-merge December 9, 2022 12:37
@pradyunsg pradyunsg closed this Dec 9, 2022
@pradyunsg pradyunsg reopened this Dec 9, 2022
@pradyunsg pradyunsg merged commit 49a3540 into pypa:main Dec 9, 2022
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.

2 participants