Opened 6 years ago

Closed 4 years ago

#5387 enhancement closed fixed (fixed)

Get rid of references and code specific to Python 2.4

Reported by: jesstess Owned by: Thijs Triemstra
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, Bartek, Thijs Triemstra Branch: branches/remove-py24-refs-5387-4
branch-diff, diff-cov, branch-cov, buildbot
Author: bartekci, MostAwesomeDude, thijs

Description (last modified by Thijs Triemstra)

Once we drop support for 2.4 in ticket 4962 and close #5385 (py2.2) and #5386 (py2.3). Some of these comments can just get removed. Some might require removal of code as well.

twisted/conch/test/test_filetransfer.py:        # In Python 2.4, the bad import has already been cleaned up for us.
twisted/internet/defer.py:    WARNING: this function will not work in Python 2.4 and earlier!
twisted/internet/defer.py:    generators.  If you need to be compatible with Python 2.4 or before, use
twisted/internet/unix.py:            # Abstract namespace sockets aren't well supported on Python 2.4.
twisted/python/dist.py:    older versions (currently the only supported older version is 2.4), checks
twisted/python/filepath.py:    Provide random data in versions of Python prior to 2.4.  This is an
twisted/python/filepath.py:            # Under Python 2.4 on Windows, WindowsError only has an errno
twisted/python/filepath.py:            # in Python 2.4 will result in a Windows error code of
twisted/python/reflect.py:                if (execName is None or # python 2.4+, post-cleanup
twisted/python/test/test_dist.py:    Tests for L{_checkCPython} when used on a Python 2.4-like platform, when
twisted/python/test/test_zipstream.py:        # In python 2.4 and earlier, consistency between the directory and the
twisted/python/util.py:    This is mostly necessary in Python 2.4 which implements L{id} to sometimes
twisted/test/test_defgen.py:## syntax error in Python 2.4 and before.
twisted/trial/runner.py:            # objects do not have nice looking string formats on Python 2.4.
twisted/trial/test/mockdoctest.py:# to test trial's doctest support with python2.4
twisted/trial/unittest.py:        # in __call__, whereas 2.4 defines the code in run.
twisted/trial/util.py:        else: # use hotshot, profile is broken in 2.4
twisted/trial/util.py:                    # Python 2.4 has fixed this.  Yay!
twisted/web/client.py:    # Python 2.4 doesn't have these symbolic constants
twisted/web/test/test_xmlrpc.py:    Tests for the C{useDateTime} flag on Python 2.4.
twisted/web/xmlrpc.py:# On Python 2.4 and earlier, DateTime.decode returns unicode.

Attachments (3)

ticket-5387.diff (8.8 KB) - added by Bartek 5 years ago.
Initial work for review. Partially done deprecation
5387.patch (13.0 KB) - added by Bartek 5 years ago.
Added complete changes for ticket and news files. A few items referenced in the original ticket were left unchanged as they seemed rellevant to the codebase past Python 2.4
5387.2.patch (12.5 KB) - added by Bartek 5 years ago.
Updated patch with newsfile typo fixes and additional doc changes as requested.

Download all attachments as: .zip

Change History (41)

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

Keywords: easy added

comment:2 Changed 6 years ago by Jean-Paul Calderone

Also "Programming Language :: Python :: 2.4" in setup.py

comment:3 Changed 5 years ago by Bartek

Owner: set to Bartek
Status: newassigned

Changed 5 years ago by Bartek

Attachment: ticket-5387.diff added

Initial work for review. Partially done deprecation

comment:4 Changed 5 years ago by Bartek

Initial work, still in progress, but would be happy if someone reviewed it to make sure it's appropriately done. Here's a summary of changes done so far:

twisted/web/test/test_xmlrpc.py -- Removed Python 2.4 specific test and flags.

twisted/web/xmlrpc.py -- Removed < Python 2.5 check in setattr -- Removed hack for DateTime.decode

twisted/web/client.py -- Adjusted getattr(os, 'SEEK_SET', 0) to os.SEEK_SET, kept private vclass variable though. Necessary?

twisted/trial/util.py -- Removed 2.4 hack under profiled -- DIDNOTFIX: Python 2.4 has fixed this, line 286. del sys.modules try/catch l

twisted/trial/unittest.py -- Removed overrides to call and run. Python 2.5/6 handle this as implemented.

twisted/trial/test/mockdoctest.py -- Removed reference to python 2.4 in doc, but test seems valid.

twisted/trial/runner.py -- Removed deprecated DocTestCase.

twisted/test/test_defgen.py -- Removed string exec hack for InlineCallbacksTests

comment:5 Changed 5 years ago by jesstess

Author: bartekci
Cc: Bartek added
Keywords: review added
Owner: Bartek deleted
Status: assignednew

Thanks for this initial work, bartekci! If you want your work reviewed, please add the 'review' keyword and assign the ticket to <blank>. This will make it show up on http://twistedmatrix.com/trac/report/11 and in the review reminders on IRC. Otherwise it's likely to get lost in the shuffle. :) I'll go ahead and set it now.

Changed 5 years ago by Bartek

Attachment: 5387.patch added

Added complete changes for ticket and news files. A few items referenced in the original ticket were left unchanged as they seemed rellevant to the codebase past Python 2.4

comment:6 Changed 5 years ago by Glyph

Description: modified (diff)

(Just updating markup, please ignore.)

comment:7 Changed 5 years ago by Corbin Simpson

Author: bartekcibartekci, MostAwesomeDude
Branch: branches/remove-py24-refs-5387

(In [33806]) Branching to 'remove-py24-refs-5387'

comment:8 Changed 5 years ago by Corbin Simpson

(In [33807]) Applying patch from #5387.

Refs #5387

comment:9 Changed 5 years ago by Corbin Simpson

(In [33808]) t.p.util: Fix trailing whitespace.

Refs #5387

comment:10 Changed 5 years ago by Corbin Simpson

Keywords: easy, revieweasy review

Looks like I've added myself to the authors list. Whatever.

This largely looks decent. The topfiles are not okay. I don't know what "Tornadio" is, but I'm pretty sure you meant something else. There are also spelling mistakes ("byres") which should be corrected.

I'm going to kick off a build for this branch; you can follow along at home at http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-py24-refs-5387

I know I'm forfeiting points, but I'm going to leave on the review keyword to attract other eyes. I don't see any code problems, but this is a very wide-reaching patch.

comment:11 Changed 5 years ago by Jeremy Thurgood

Keywords: easy review removed

I only have some minor stylistic comments.

  • In python/dist.py removing the Python 2.4 comment in the docstring leaves a partial line. I'd prefer the paragraph be reflowed to look tidier.
  • In internet/defer.py all references to Python 2.4 have been removed, but the reference to "Python 2.5 generators" is still there. Since we no longer support any version of Python /without/ generators, I'd drop the version from that sentence.
  • In trial/test/mockdoctest.py there is a reference to a __test__ attribute that doesn't seem to exist. This isn't strictly part of this ticket, but I think it can be cleaned up as well.

The code changes look okay. There's only one test failure on my system (some XPath thing in twisted.words), and that looks unrelated to the changes in this branch. (The same test fails in trunk.)

Ran 7295 tests in 121.716s
FAILED (skips=1468, expectedFailures=11, failures=1, successes=5815)

comment:12 Changed 5 years ago by Jeremy Thurgood

Owner: set to Corbin Simpson

comment:13 Changed 5 years ago by ralphm

There is also some version-specific code (for before Python 2.3.2) in twisted.words.protocols.jabber.xmpp_stringprep.

Also, I think your failing test is in test_sip, not xish.

Changed 5 years ago by Bartek

Attachment: 5387.2.patch added

Updated patch with newsfile typo fixes and additional doc changes as requested.

comment:14 Changed 5 years ago by Bartek

Keywords: review added

comment:15 Changed 5 years ago by rockstar

Owner: changed from Corbin Simpson to rockstar

comment:16 Changed 5 years ago by Corbin Simpson

(In [33851]) Update from 5387.2.patch.

Refs #5387

comment:17 Changed 5 years ago by Corbin Simpson

Updated the branch to the latest stuff from the patch. Please review.

bartekci, thank you for your hard work on this issue.

comment:18 Changed 5 years ago by rockstar

Keywords: review removed
Owner: rockstar deleted

Hi bartekci, thanks for this patch.

Since you've removed DocTestCase here, you should probably also assign #5554 to yourself and close it when this ticket makes it to trunk.

After talking with dreid, I think it's probably better to ask that we hold off on completion of this ticket until after we close #5385 and #5386. A large reason is that there are many things like in t.internet.unix._UNIXPort that do things like "if sys.version_info > (2, 5)", and so we should probably remove that support in this ticket, *but* we should make sure that everything older than 2.4 is also taken care of.

comment:19 Changed 5 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)

comment:20 Changed 4 years ago by Thijs Triemstra

Owner: set to Thijs Triemstra
Status: newassigned

comment:21 Changed 4 years ago by Thijs Triemstra

Branch: branches/remove-py24-refs-5387branches/remove-py24-refs-5387-2

comment:22 Changed 4 years ago by Thijs Triemstra

(In [37918]) remove more python2.4 references, add news files. refs #5387

comment:23 Changed 4 years ago by Thijs Triemstra

(In [37919]) update setup files, refs #5387

comment:24 Changed 4 years ago by Thijs Triemstra

Author: bartekci, MostAwesomeDudebartekci, MostAwesomeDude, thijs
Branch: branches/remove-py24-refs-5387-2branches/remove-py24-refs-5387-3

(In [37920]) Branching to 'remove-py24-refs-5387-3'

comment:25 Changed 4 years ago by Thijs Triemstra

(In [37921]) merge forward, refs #5387

comment:26 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Forced a build, up for review.

comment:27 Changed 4 years ago by Julian Berman

Should the reference & conditional rockstar mentioned in t.i.unix._UNIXPort now be removed?

comment:28 Changed 4 years ago by Tom Prince

Owner: set to Tom Prince
Status: newassigned

comment:29 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Thijs Triemstra
Status: assignednew
  1. twisted/conch/topfiles/setup.py: Do we even need the conditional anymore? If the only versions we support accept classifiers, we might as well always provide them.
    • This is in fact true of every setup.py.
  2. twisted.internet.defer: Don't delete the example.
  3. twisted.python.filepath
    1. Why are you removing the reference to randomBytes? If it is for compatibility, it should be deprecated in a separate ticket.
    2. The code in children can be simplified (but perhaps this should entirely be a separate ticket:
      • .winerror is always present now.
      • The comment suggests that it might be possible to simply use the posix error code on windows as well.
      • Both of those changes probably require test changes.
  4. twisted/python/test/test_dist.py:
    • I think the comment about being tests for a 2.4-like platform are still accurate.
    • I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.
  5. twisted/trial/runner.py:
    • The comment you are deleting suggests that we could simplify the code now. If this is the case, we should leave some evidence of this (new ticket?).
  6. twisted/web/client.py:
    • Since these constans are always present, we can simply use them directly, rather than the aliases you are touching.
  7. twisted/web/xmlrpc.py:
    1. The __setattr__ is now superfluous.
    2. (minor) L{xmlrpclib} instead of C{xmlrpclib}.
  8. There appears to be an error on the py-select-gc builder.
  9. I haven't looked at coverage of the changed code, but this makes me thing that at least some of it isn't.

comment:30 Changed 4 years ago by Thijs Triemstra

(In [38650]) address review comments, refs #5387

comment:31 Changed 4 years ago by Thijs Triemstra

Branch: branches/remove-py24-refs-5387-3branches/remove-py24-refs-5387-4

(In [38652]) Branching to 'remove-py24-refs-5387-4'

comment:32 Changed 4 years ago by Thijs Triemstra

(In [38653]) merge forward, resolve conflicts. refs #5387

comment:33 in reply to:  29 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for your review!

Replying to tom.prince:

  1. twisted/conch/topfiles/setup.py: Do we even need the conditional anymore? If the only versions we support accept classifiers, we might as well always provide them.
    • This is in fact true of every setup.py.

Removed the conditionals in setup.py's.

  1. twisted.internet.defer: Don't delete the example.

Fixed.

  1. twisted.python.filepath
    1. Why are you removing the reference to randomBytes? If it is for compatibility, it should be deprecated in a separate ticket.

This was part of a patch supplied by someone else; restored the reference again.

  1. The code in children can be simplified (but perhaps this should entirely be a separate ticket:
    • .winerror is always present now.
    • The comment suggests that it might be possible to simply use the posix error code on windows as well.
    • Both of those changes probably require test changes.

I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc.

  1. twisted/python/test/test_dist.py:
    • I think the comment about being tests for a 2.4-like platform are still accurate.

Restored the comment.

  • I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.

What do you suggest, should we rename _checkCPythonWithEmptyPlatform to _checkPythonWithEmptyPlatform? And/or remove the test_cpython testcase?

  1. twisted/trial/runner.py:
    • The comment you are deleting suggests that we could simplify the code now. If this is the case, we should leave some evidence of this (new ticket?).

Yes, I simplified it and it still passes the test. I think opening a new ticket will only create more work since its already covered by a test.. thoughts?

  1. twisted/web/client.py:
    • Since these constans are always present, we can simply use them directly, rather than the aliases you are touching.

Fixed.

  1. twisted/web/xmlrpc.py:
    1. The __setattr__ is now superfluous.
    2. (minor) L{xmlrpclib} instead of C{xmlrpclib}.

Fixed.

  1. There appears to be an error on the py-select-gc builder.

Forced a new build

  1. I haven't looked at coverage of the changed code, but this makes me thing that at least some of it isn't.

The coverage report i generated shows (the changes made to) Proxy is covered.

comment:34 in reply to:  27 Changed 4 years ago by Thijs Triemstra

Replying to Julian:

Should the reference & conditional rockstar mentioned in t.i.unix._UNIXPort now be removed?

I think that should be handled in #5715.

comment:35 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. twisted/trial/runner.py: "%s" % (repr(x),) is the same as "%r" % (x,)

I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.

What do you suggest, should we rename _checkCPythonWithEmptyPlatform to _checkPythonWithEmptyPlatform? And/or remove the test_cpython testcase?

I'm suggesting that _checkCPython should return False in the fallthrough, instead of True, since now that we don't support <=2.4, that case can't be cpython. This could be done in a seperate ticket.

I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc

Sure.

Please merge, after after addressing 1, and filing tickets for 2+3.

comment:36 Changed 4 years ago by Thijs Triemstra

(In [39569]) address review comments, refs #5387

comment:37 Changed 4 years ago by Thijs Triemstra

Status: newassigned

Replying to tom.prince:

  1. twisted/trial/runner.py: "%s" % (repr(x),) is the same as "%r" % (x,)

Fixed.

I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc

Sure.

Looks somebody opened a ticket for this already (#6627) that includes a patch that seems to have a better fix as well.

Forced a [buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-py24-refs-5387-4 build] and will merge when it's green (after I opened a ticket for 2).

comment:38 Changed 4 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [39703]) Merge remove-py24-refs-5387-4: Get rid of references and code specific to Python 2.4.

Author: thijs, bartekci Reviewer: jesstess, MostAwesomeDude, jerith, ralphm, rockstar, tom.prince Fixes: #5387

Note: See TracTickets for help on using tickets.