Opened 12 years ago

Closed 10 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: ralphm, 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)

fix-md5-and-sha.patch (50.9 KB) - added by devinus 11 years ago.
hashlib.patch (44.3 KB) - added by Wilfredo Sánchez Vega 10 years ago.
hashlib.v2.patch (47.2 KB) - added by Wilfredo Sánchez Vega 10 years ago.

Download all attachments as: .zip

Change History (55)

comment:1 Changed 12 years ago by Jean-Paul Calderone

The stdio tests should definitely be changed to not break as a result of this deprecation.

comment:2 Changed 12 years ago by ralphm

Cc: ralphm added

comment:3 Changed 12 years ago by therve

Milestone: Python-2.6

comment:4 Changed 11 years ago by devinus

Owner: changed from therve to devinus
Status: newassigned

Changed 11 years ago by devinus

Attachment: fix-md5-and-sha.patch added

comment:5 Changed 11 years ago by devinus

Keywords: review added

comment:6 Changed 11 years ago by devinus

Owner: devinus deleted
Status: assignednew

comment:7 Changed 11 years ago by David Reid

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 11 years ago by Jean-Paul Calderone

<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 11 years ago by devinus

Status: newassigned
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 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

There's <http://code.krypto.org/python/hashlib/>. I wonder if we can somehow make use of this.

Changed 10 years ago by Wilfredo Sánchez Vega

Attachment: hashlib.patch added

comment:11 Changed 10 years ago by Wilfredo Sánchez Vega

Keywords: review added

Attached hashlib.patch got rid of thw warnings for me and all tests passed.

comment:12 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from devinus to Wilfredo Sánchez Vega
Status: assignednew
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 10 years ago by Wilfredo Sánchez Vega

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 10 years ago by Wilfredo Sánchez Vega

Attachment: hashlib.v2.patch added

comment:14 Changed 10 years ago by Wilfredo Sánchez Vega

Keywords: review added

comment:15 Changed 10 years ago by Wilfredo Sánchez Vega

Tests pass with the v2 patch on my OS X 10.5 box with Py2.5.

comment:16 Changed 10 years ago by Wilfredo Sánchez Vega

Author: wsanchez
Branch: branches/hashlib-2763

(In [25340]) Branch for #2763: md5 and sha module will be deprecated in python 2.6

comment:17 Changed 10 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: newclosed

(In [25341]) Add a twisted.python.hashlib module which imports hashlib or (if that fails) md5 and sha, which are deprecated in Python 2.6. Fixes #2763.

comment:18 Changed 10 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: closedreopened

comment:19 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Status: reopenednew

There are some test failures on Windows.

comment:20 Changed 10 years ago by Jean-Paul Calderone

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 10 years ago by Wilfredo Sánchez Vega

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:22 Changed 10 years ago by Wilfredo Sánchez Vega

Whoops. r25343 should address that.

comment:23 Changed 10 years ago by Wilfredo Sánchez Vega

Keywords: review added

comment:24 Changed 10 years ago by therve

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 10 years ago by Wilfredo Sánchez Vega

Keywords: review added
Status: newassigned

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 10 years ago by Jean-Paul Calderone

Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

comment:27 Changed 10 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:28 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
  1. twisted/python/hashlib.py
    1. no copyright statement
    2. no test-case-name
    3. no module docstring
    4. no unit tests
    5. only the __import__ should be protected by the try/except ImportError. The two name bindings should happen outside.
    6. __all__ is cool; hashlib should probably be _hashlib anyway, though, because sometimes people don't pay attention.
  2. twisted/conch/scripts/ckeygen.py
    1. copyright statement should be updated
    2. the module has some other random junk that might as well be cleaned up:
      1. module docstring is commented out
      2. there's still a cvs id marker which should be deleted
      3. imports appear in the wrong order
      4. printFingerprint uses deprecated apis
    3. the module has no unit test coverage whatsoever; any code being changed must have tests
  3. twisted/cred/util.py
    1. no test-case-name
    2. no unit tests
    3. actually, this module is entirely unused in twisted. Should be deprecated, I think.
  4. twisted/words/protocols/oscar.py
    1. no unit tests
    2. copyright date should be updated
    3. old __future__ import should be deleted
  5. twisted/words/protocols/msn.py
    1. update copyright date
    2. some trailing whitespace should be stripped
    3. handle_CHL is untested
  6. twisted/mail/maildir.py
    1. copyright date needs updating
    2. module docstring should be reformatted to follow coding standard
    3. StringListMailbox has no unit tests

I suspect there's more minor points and untested changes, but I'm exhausted.

comment:29 Changed 10 years ago by Jean-Paul Calderone

(In [25354]) address review points

except for StringListMailbox unit tests

refs #2763

comment:30 Changed 10 years ago by Wilfredo Sánchez Vega

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:31 Changed 10 years ago by Wilfredo Sánchez Vega

Keywords: review added

#3547 on StringListMailbox

comment:32 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
  1. Since Glyph merged #1376, the _fingerprint function in twisted/conch/scripts/ckeygen.py can be replaced with Key.fingerprint (the branch needs to be merged forward for this).
  2. There's an extraneously added raise to the except suite in printFingerprint which should be removed.
  3. twisted/conch/test/test_ckeygen.py's copyright date should just say 2008, since it didn't exist in any of those prior years.
  4. in twisted/conch/test/test_ckeygen.py, there should be another blank line (for a total of two) between KeyGenTests.setUp and KeyGenTests.test_printFingerprint.
  5. in twisted/conch/ssh/transport.py, the commented out checks for the digest_size attribute are now doubly obsolete and should be deleted.
  6. twisted/news/database.py mixes up imports a bit and can lose the nested_scopes future import
  7. twisted/python/otp.py has no test coverage, lots of trailing whitespace, and other crummyness
  8. twisted/web/monitor.py has no test coverage; the module can't even be imported.
  9. The change to twisted/web2/server.py seems unrelated
  10. twisted/words/protocols/oscar.py should have its copyright date updated and the unused imports (time, nested_scopes) removed.
  11. 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 10 years ago by Jean-Paul Calderone

Author: wsanchezexarkun, wsanchez
Branch: branches/hashlib-2763branches/hashlib-2763-2

(In [25426]) Branching to 'hashlib-2763-2'

comment:34 Changed 10 years ago by Jean-Paul Calderone

(In [25428]) address points 1-5

refs #2763

comment:35 Changed 10 years ago by Jean-Paul Calderone

(In [25429]) address point 6

refs #2763

comment:36 Changed 10 years ago by Jean-Paul Calderone

(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 10 years ago by Jean-Paul Calderone

(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 10 years ago by Jean-Paul Calderone

(In [25432]) cleanup imports, addresses review points 10 and 11

refs #2763

comment:39 Changed 10 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Wilfredo Sánchez Vega
Status: assignednew

Took care of all the points except for 9. I'm not really sure what's up with that.

comment:40 Changed 10 years ago by Wilfredo Sánchez Vega

Reverted twisted/web2/server.py. No clue how that change got onto that branch. :-/

comment:41 Changed 10 years ago by Wilfredo Sánchez Vega

Keywords: review added

comment:42 Changed 10 years ago by Wilfredo Sánchez Vega

Owner: changed from Wilfredo Sánchez Vega to Jean-Paul Calderone

comment:43 Changed 10 years ago by Jean-Paul Calderone

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 10 years ago by Michael Hudson-Doyle

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:

  1. 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?
  1. 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.
  1. 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 10 years ago by Jean-Paul Calderone

Author: exarkun, wsanchezwsanchez, exarkun
Branch: branches/hashlib-2763-2branches/hashlib-2763-3

(In [25448]) Branching to 'hashlib-2763-3'

comment:46 Changed 10 years ago by Jean-Paul Calderone

(In [25450]) try to clarify the assertions made in these hashy unit tests

refs #2763

comment:47 Changed 10 years ago by Jean-Paul Calderone

(In [25451]) change 8.2 deprecations to 8.3 deprecations

refs #2763

comment:48 Changed 10 years ago by Jean-Paul Calderone

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 10 years ago by Michael Hudson-Doyle

Keywords: review removed

I'm still not completely convinced by test_respond, but I don't think I care. Land it.

comment:50 Changed 10 years ago by Michael Hudson-Doyle

Owner: set to Jean-Paul Calderone

comment:51 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.