Opened 15 years ago
Closed 14 years ago
#2763 enhancement closed fixed (fixed)
md5 and sha module will be deprecated in python 2.6
Reported by: | therve | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | Python-2.6 |
Component: | core | Keywords: | |
Cc: | Ralph Meijer, Jean-Paul Calderone | Branch: |
branches/hashlib-2763-3
branch-diff, diff-cov, branch-cov, buildbot |
Author: | wsanchez, exarkun |
Description
We have to use the hashlib module available since python 2.5.
Note that I saw this because stdio tests are failing on python trunk. I'm not sure it should be sensitive to warnings emitted...
Attachments (3)
Change History (55)
comment:1 Changed 15 years ago by
comment:2 Changed 15 years ago by
Cc: | Ralph Meijer added |
---|
comment:3 Changed 15 years ago by
Milestone: | → Python-2.6 |
---|
comment:4 Changed 14 years ago by
Owner: | changed from therve to devinus |
---|---|
Status: | new → assigned |
Changed 14 years ago by
Attachment: | fix-md5-and-sha.patch added |
---|
comment:5 Changed 14 years ago by
Keywords: | review added |
---|
comment:6 Changed 14 years ago by
Owner: | devinus deleted |
---|---|
Status: | assigned → new |
comment:7 Changed 14 years ago by
Keywords: | review removed |
---|---|
Owner: | set to devinus |
You should fall back to importing sha and md5 if the hashlib import fails
Something like
try: from hashlib import md5, sha1 as sha except ImportError: from md5 import md5 from sha import sha
comment:8 Changed 14 years ago by
<itamar> instead, why not have the twisted.python.compat
module install a fake hashlib module on Python versions that don't provide it and then use hashlib
everywhere?
comment:9 Changed 14 years ago by
Status: | new → assigned |
---|
try: import hashlib except ImportError: import md5 import sha class hashlib(object): """Mimic a small subset of the Python 2.5 hashlib module.""" __module__ = 'hashlib' @classmethod def new(cls, name, string=''): if name in ('MD5', 'md5'): return md5.new(string) elif name in ('SHA1', 'sha1'): return sha.new(string) raise ValueError, "unsupported hash type" @classmethod def md5(cls, string=''): return md5.new(string) @classmethod def sha1(cls, string=''): return sha.new(string) sys.modules['hashlib'] = hashlib
I had all of the files using the old md5 and sha modules converted over, but some were failing the test suite, so I'll need more time.
comment:10 Changed 14 years ago by
Cc: | Jean-Paul Calderone added |
---|
There's <http://code.krypto.org/python/hashlib/>. I wonder if we can somehow make use of this.
Changed 14 years ago by
Attachment: | hashlib.patch added |
---|
comment:11 Changed 14 years ago by
Keywords: | review added |
---|
Attached hashlib.patch got rid of thw warnings for me and all tests passed.
comment:12 Changed 14 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from devinus to Wilfredo Sánchez Vega |
Status: | assigned → new |
22:14 < wsanchez> How would .compat install the fake module? I can spend the time to clean up all the other files, just not sure what you had in mind 22:14 < wsanchez> We can do import twisted.python.hashlib which imports the right thing; that'd be easy 22:15 < exarkun> I think that would be fine. 22:15 < exarkun> Preferable to stuffing fakes into sys.modules, in fact. 22:15 < wsanchez> Or from twisted.python.compat import md5, sha1, but that seems a little less nice 22:15 < exarkun> Having twisted.python.hashlib.md5 and twisted.python.hashlib.sha1 sounds good. 22:16 < wsanchez> Yeah, I wouldn't want to monkey with sys.modules; that seems like looking for some kind of sneaky pain later 22:16 < wsanchez> OK I'll do that.
comment:13 Changed 14 years ago by
Also replacing 'sha1 as sha' imports to simply 'sha1'. Makes searching for uses much easier and will make it easier to switch to directly using Python's hashlib in the future.
Changed 14 years ago by
Attachment: | hashlib.v2.patch added |
---|
comment:14 Changed 14 years ago by
Keywords: | review added |
---|
comment:16 Changed 14 years ago by
Author: | → wsanchez |
---|---|
Branch: | → branches/hashlib-2763 |
comment:17 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:18 Changed 14 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:19 Changed 14 years ago by
Keywords: | review removed |
---|---|
Status: | reopened → new |
There are some test failures on Windows.
comment:20 Changed 14 years ago by
Actually, I suppose that was premature. There seem to be mad failures on most platforms. Maybe all. And the test suite is taking ages.
comment:21 Changed 14 years ago by
Failures appear limited to conch:
File "c:\twistedbuildbot24\WXP32-full2.4-scmikes-select\Twisted\twisted\conch\ssh\transport.py", line 1226, in _getCipher counter=_Counter(iv, mod.block_size)) exceptions.TypeError: 'module' object is not callable
And so far only Windows has completed (Ubuntu lost the builder). It's going…very…slowly…
comment:23 Changed 14 years ago by
Keywords: | review added |
---|
OK, all tests are passing now:
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/hashlib-2763
comment:24 Changed 14 years ago by
Keywords: | review removed |
---|
Note that tests passing, but you're not using hashlib...
"import hashlib" in twisted.python.hashlib tries to make it import itself. You have to use __import__
, or rename the module in twisted.python.
comment:25 Changed 14 years ago by
Keywords: | review added |
---|---|
Status: | new → assigned |
OK:
__all__ = ["md5", "sha1"] try: hashlib = __import__("hashlib") md5 = hashlib.md5 sha1 = hashlib.sha1 except ImportError: from md5 import md5 from sha import sha as sha1
comment:26 Changed 14 years ago by
Owner: | Wilfredo Sánchez Vega deleted |
---|---|
Status: | assigned → new |
comment:27 Changed 14 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:28 Changed 14 years ago by
Keywords: | review removed |
---|
- twisted/python/hashlib.py
- no copyright statement
- no test-case-name
- no module docstring
- no unit tests
- only the
__import__
should be protected by the try/except ImportError. The two name bindings should happen outside. __all__
is cool;hashlib
should probably be_hashlib
anyway, though, because sometimes people don't pay attention.
- twisted/conch/scripts/ckeygen.py
- copyright statement should be updated
- the module has some other random junk that might as well be cleaned up:
- module docstring is commented out
- there's still a cvs id marker which should be deleted
- imports appear in the wrong order
printFingerprint
uses deprecated apis
- the module has no unit test coverage whatsoever; any code being changed must have tests
- twisted/cred/util.py
- no test-case-name
- no unit tests
- actually, this module is entirely unused in twisted. Should be deprecated, I think.
- twisted/words/protocols/oscar.py
- no unit tests
- copyright date should be updated
- old
__future__
import should be deleted
- twisted/words/protocols/msn.py
- update copyright date
- some trailing whitespace should be stripped
handle_CHL
is untested
- twisted/mail/maildir.py
- copyright date needs updating
- module docstring should be reformatted to follow coding standard
StringListMailbox
has no unit tests
I suspect there's more minor points and untested changes, but I'm exhausted.
comment:29 Changed 14 years ago by
comment:30 Changed 14 years ago by
Many of the review points are completely unrelated to this ticket and should be on a different branch, but whatever. For example, missing unit tests for StringListMailbox
has nothing to do with hashlib, and wasn't introduced by this fix. While general code cleanup is nice and all, one fix shouldn't be blocked by unrelated fixes.
r25401 updates the copyright on every file changed on that branch as needed.
comment:32 Changed 14 years ago by
Keywords: | review removed |
---|
- Since Glyph merged #1376, the
_fingerprint
function in twisted/conch/scripts/ckeygen.py can be replaced withKey.fingerprint
(the branch needs to be merged forward for this). - There's an extraneously added
raise
to the except suite inprintFingerprint
which should be removed. - twisted/conch/test/test_ckeygen.py's copyright date should just say 2008, since it didn't exist in any of those prior years.
- in twisted/conch/test/test_ckeygen.py, there should be another blank line (for a total of two) between
KeyGenTests.setUp
andKeyGenTests.test_printFingerprint
. - in twisted/conch/ssh/transport.py, the commented out checks for the digest_size attribute are now doubly obsolete and should be deleted.
- twisted/news/database.py mixes up imports a bit and can lose the
nested_scopes
future import - twisted/python/otp.py has no test coverage, lots of trailing whitespace, and other crummyness
- twisted/web/monitor.py has no test coverage; the module can't even be imported.
- The change to twisted/web2/server.py seems unrelated
- twisted/words/protocols/oscar.py should have its copyright date updated and the unused imports (time, nested_scopes) removed.
- twisted/words/test/test_msn.py has an unused import,
sys
, which should be removed
A bunch of these points are for code that I wrote after my last review anyway, so I'll merge the branch forward and address them.
comment:33 Changed 14 years ago by
Author: | wsanchez → exarkun, wsanchez |
---|---|
Branch: | branches/hashlib-2763 → branches/hashlib-2763-2 |
(In [25426]) Branching to 'hashlib-2763-2'
comment:36 Changed 14 years ago by
(In [25430]) deprecate twisted.python.otp
I tried writing tests for it first, but it's either broken or not well documented enough for me to figure out how to use. In any case, there are other OTP libraries for Python, and there is nothing specific or beneficial to Twisted about this module. It also hasn't really been touched since it was added seven years ago.
No tests for this deprecation because I don't know how to test module level warnings.
refs #2763
comment:37 Changed 14 years ago by
(In [25431]) delete twisted.web.monitor (addresses review point 8)
This module has been unimportable for 4 years. Someone noticed, and even cared enough to file a ticket, but not enough to write any unit tests. So, removing. This isn't a backward incompatible change because the behavior is the same before and after: the module cannot be imported.
refs #2763
comment:38 Changed 14 years ago by
comment:39 Changed 14 years ago by
Owner: | changed from Jean-Paul Calderone to Wilfredo Sánchez Vega |
---|---|
Status: | assigned → new |
Took care of all the points except for 9. I'm not really sure what's up with that.
comment:40 Changed 14 years ago by
Reverted twisted/web2/server.py
. No clue how that change got onto that branch. :-/
comment:41 Changed 14 years ago by
Keywords: | review added |
---|
comment:42 Changed 14 years ago by
Owner: | changed from Wilfredo Sánchez Vega to Jean-Paul Calderone |
---|
comment:43 Changed 14 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
Anybody can review this (and having someone other than me give it a look would be great).
comment:44 Changed 14 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Mostly this is a monster yawn of course. I have some small problems with the new tests you've written, and another comment:
twisted.words.test.test_msn.NotificationTests.test_challenge
Reading this test, I have no idea how to tell if it is correct or not. Given the nature of what this is testing, this is perhaps inevitable, but maybe you could compute the result rather than including it literally, and include a reference to why this is the right thing to do?
twisted.test.test_newcred.DeprecatedUtilTests.test_respond
Same complaint really. Also, I think it would result in clearer failures if you put the assertion about the length of warnings before the assertions about the contents of the list.
- Hasn't this branch missed the chance to be included in Twisted 8.2 by now? If so, the deprecation messages need to be changed.
Finally, has this branch been tested on Python 2.6?
comment:45 Changed 14 years ago by
Author: | exarkun, wsanchez → wsanchez, exarkun |
---|---|
Branch: | branches/hashlib-2763-2 → branches/hashlib-2763-3 |
(In [25448]) Branching to 'hashlib-2763-3'
comment:46 Changed 14 years ago by
comment:47 Changed 14 years ago by
comment:48 Changed 14 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Points 1-3 addressed in the previous two commits. The branch has been tested on Python 2.6:
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/hashlib-2763 (search for py2.6). The -3 branch is building now. Results at:
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/hashlib-2763-3
comment:49 Changed 14 years ago by
Keywords: | review removed |
---|
I'm still not completely convinced by test_respond, but I don't think I care. Land it.
comment:50 Changed 14 years ago by
Owner: | set to Jean-Paul Calderone |
---|
comment:51 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [25457]) Merge hashlib-2763-3
Author: wsanchez, exarkun Reviewer: exarkun, mwhudson Fixes: #2763
Replace uses of md5
and sha
modules in Twisted with use of a new twisted.python.hashlib
module which transparently uses the new hashlib
standard library module if it is available
or falls back to md5
and sha
if not.
comment:52 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
The stdio tests should definitely be changed to not break as a result of this deprecation.