Multiple hashers #467

Merged
merged 13 commits into from Jul 6, 2012

Projects

None yet

5 participants

@dstufft
Member
dstufft commented Mar 2, 2012

This branch enables the use of different hash names besides md5 for verifying a hash. It uses all of the built in hashes this includes sha1, sha224, sha384, sha256, sha512, and md5. It selects which hash name to use based off the fragment in the url (#md5=... for md5 and #sha256=... for sha256). The hash name is case sensitive.

This branch removes the backwards compatibility shim for Python 2.4 and hashlib.

@gvalkov gvalkov and 1 other commented on an outdated diff Mar 2, 2012
pip/download.py
@@ -321,16 +322,22 @@ def is_file_url(link):
return link.url.lower().startswith('file:')
-def _check_md5(download_hash, link):
- download_hash = download_hash.hexdigest()
- if download_hash != link.md5_hash:
- logger.fatal("MD5 hash of the package %s (%s) doesn't match the expected hash %s!"
- % (link, download_hash, link.md5_hash))
- raise InstallationError('Bad MD5 hash for package %s' % link)
+def _check_hash(download_hash, link):
+ if download_hash.name != link.hash_name:
+ logger.fatal("Hash name of the package %s (%s) doesn't match the expect hash name %s!"
@gvalkov
gvalkov Mar 2, 2012 Contributor

s/expect/expected

@dstufft
dstufft Mar 2, 2012 Member

fixed, thanks for that :)

@carljm carljm and 1 other commented on an outdated diff Mar 13, 2012
.travis.yml
branches:
only:
- develop
+ - multiple-hashers
@carljm
carljm Mar 13, 2012 Contributor

Seems like this is a change that shouldn't be merged to develop? I mean, once this is merged there's no point in testing the branch anymore.

@dstufft
dstufft Mar 13, 2012 Member

Removed my changes to .travis.yml, sorry about that.

@carljm carljm and 1 other commented on an outdated diff Mar 13, 2012
pip/download.py
-def _get_md5_from_file(target_file, link):
- download_hash = md5()
+def _get_hash_from_file(target_file, link):
+ try:
+ download_hash = hashlib.new(link.hash_name)
+ except ValueError:
+ logger.fatal("Unsupported hash name %s for package %s" % (link.hash_name, link))
@carljm
carljm Mar 13, 2012 Contributor

Why logger.fatal here, when this is not actually a fatal error? We just go on and ignore the hash, right?

@dstufft
dstufft Mar 13, 2012 Member

Because i'm a goof and used the wrong one. Should that be an Error or Warning?

@carljm
carljm Mar 13, 2012 Contributor

We should go on and ignore the hash, but currently instead I think this will crash with an obscure NameError later in the function. This case needs to be tested.

@carljm
carljm Mar 13, 2012 Contributor

So warning, I'd say.

@carljm
Contributor
carljm commented Mar 13, 2012

Looks great, needs tests! ;-)

@pnasrat
Contributor
pnasrat commented May 13, 2012

@dstufft do you need a hand with test approaches?

@travisbot

This pull request fails (merged 49b322b into 2f84e14).

@travisbot

This pull request fails (merged 137b7c5 into 2f84e14).

@dstufft
Member
dstufft commented May 26, 2012

The failing test on this is unrelated to the actual changes and seems to fail randomly. All the tests pass and I've included new tests in order to test the new behavior.

I've also made the change to go from fatal to warn as @carljm mentioned.

Hopefully these changes will be useful and will enable merging this in ;)

@dstufft
Member
dstufft commented May 26, 2012

In other words, I believe this is ready for merge.

@pnasrat
Contributor
pnasrat commented May 26, 2012

Yes the failing test is likely issue #530

@dstufft
Member
dstufft commented Jun 20, 2012

Just a quick ping and wanted to make sure that this is just waiting on commiter time and doesn't need anything further from me :)

@pnasrat
Contributor
pnasrat commented Jun 20, 2012

Yeah - although develop has moved so this is no longer auto mergable - if you fix up I'll take a look. The flaky test is skipped now so we should get good signal. Can you fix up so can merge and also add to NEWS and AUTHORS as necessary.

Cheers

@dstufft
Member
dstufft commented Jun 21, 2012

The tests are passing locally, It appears travis is not testing pip anymore but all tests pass for me.

@travisbot

This pull request passes (merged bb01108 into 4c96de6).

@dstufft
Member
dstufft commented Jun 21, 2012

Ah there it is :)

@pnasrat
Contributor
pnasrat commented Jul 1, 2012

Just one more thing - can you add user facing docs to docs/usage.txt for this feature please.

@dstufft
Member
dstufft commented Jul 4, 2012

Believe this should be all ready now.

@travisbot

This pull request fails (merged 549fdbc into 5562a6b).

@dstufft
Member
dstufft commented Jul 4, 2012

The above failure was not because of anything in this PR.

@pnasrat pnasrat merged commit c4abe2f into pypa:develop Jul 6, 2012
@dstufft dstufft deleted the dstufft:multiple-hashers branch Nov 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment