conda icon indicating copy to clipboard operation
conda copied to clipboard

Enable usage of supported checksum algorithms in RECORDS (fixes #13198)

Open iamthebot opened this issue 2 years ago • 9 comments

Description

In #13198 we identified that an issue still remains in conda's handling of PEP 376 where checksum algorithms shown as uppercase result in an AssertionError. This PR fixes the existing logic to handle all supported algorithms per that PEP in RECORD files. The latest packaging guidelines provide that this is everything in hashlib.algorithms_guaranteed except sha1 and md5.

One caveat-- while the PEP itself does not specify capitalization, it only specifies that the algorithm should be one of hashlib.algorithms_guaranteed-- those are all lowercase. In practice, however, some widely used packages seem to use uppercase. Conda probably shouldn't break in this case even if the algorithm name is technically non-conformant.

This PR also improves error messages significantly if a non-conforming package is encountered.

Checklist - did you ...

  • [x] Add a file to the news directory (using the template) for the next release's release notes?
  • [x] Add / update necessary tests?
  • [ ] Add / update outdated documentation?

iamthebot avatar Oct 10 '23 20:10 iamthebot

We require contributors to sign our Contributor License Agreement and we don't have one on file for @iamthebot.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (https://github.com/conda/infrastructure/pull/835), and ping the bot to refresh the PR.

conda-bot avatar Oct 10 '23 20:10 conda-bot

FYI I just signed the CLA but the CLA PR needs to be merged by a maintainer.

iamthebot avatar Oct 10 '23 21:10 iamthebot

Updated this PR to reflect the latest python packaging guidelines where all algorithms in hashlib.algorithms_guaranteed are supported except sha1 and md5.

iamthebot avatar Oct 10 '23 22:10 iamthebot

@conda-bot check

travishathaway avatar Oct 11 '23 12:10 travishathaway

@iamthebot,

Thanks a lot for this pull request. I already left a couple minor suggestions for improvements (we prefer using f-strings for this project).

I am wondering if we could ditch the use of assertions. When these fail, they result in ugly trac eback errors like the one you initially encountered. Instead, you could raise any error which inherits from conda.exceptions.CondaError and a much nicer error message will be shown.

Let me know what you think.

travishathaway avatar Oct 11 '23 13:10 travishathaway

@travishathaway went ahead and applied your PR suggestions. Also expanded on the relevant unit test a bit so we actually check the possible algorithms (this is very quick so shouldn't impact CI time too much). I also went ahead and removed unused code (_check_path_data is not referenced anywhere).

I didn't change the other assertions in this file just to keep the PR tightly scoped but we can do that in a followup PR.

Let me know what you think.

iamthebot avatar Oct 11 '23 20:10 iamthebot

@iamthebot,

Thanks for the clarification. I'm okay with the letting the assert statements stay in for now. Please see my comments regarding the failing test though.

travishathaway avatar Oct 17 '23 05:10 travishathaway

Thanks @travishathaway I've been a bit busy this week, will try to get to this tomorrow.

iamthebot avatar Oct 19 '23 03:10 iamthebot

Hi there, thank you for your contribution!

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this pull request to remain open please:

  1. Rebase and verify the changes still work
  2. Leave a comment with the current status

NOTE: If this pull request was closed prematurely, please leave a comment.

Thanks!

github-actions[bot] avatar Oct 18 '24 04:10 github-actions[bot]