Opened 3 years ago

Closed 13 months ago

#5387 enhancement closed fixed (fixed)

Get rid of references and code specific to Python 2.4

Reported by: jesstess Owned by: thijs
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, bartekci, thijs Branch: branches/remove-py24-refs-5387-4
(diff, github, buildbot, log)
Author: bartekci, MostAwesomeDude, thijs Launchpad Bug:

Description (last modified by thijs)

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 bartekci 3 years ago.
Initial work for review. Partially done deprecation
5387.patch (13.0 KB) - added by bartekci 3 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 bartekci 3 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 3 years ago by exarkun

  • Keywords easy added

comment:2 Changed 3 years ago by exarkun

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

comment:3 Changed 3 years ago by bartekci

  • Owner set to bartekci
  • Status changed from new to assigned

Changed 3 years ago by bartekci

Initial work for review. Partially done deprecation

comment:4 Changed 3 years ago by bartekci

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 3 years ago by jesstess

  • Author set to bartekci
  • Cc bartekci added
  • Keywords review added
  • Owner bartekci deleted
  • Status changed from assigned to new

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 3 years ago by bartekci

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 3 years ago by glyph

  • Description modified (diff)

(Just updating markup, please ignore.)

comment:7 Changed 3 years ago by MostAwesomeDude

  • Author changed from bartekci to bartekci, MostAwesomeDude
  • Branch set to branches/remove-py24-refs-5387

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

comment:8 Changed 3 years ago by MostAwesomeDude

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

Refs #5387

comment:9 Changed 3 years ago by MostAwesomeDude

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

Refs #5387

comment:10 Changed 3 years ago by MostAwesomeDude

  • Keywords changed from easy, review to easy 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 3 years ago by jerith

  • 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 3 years ago by jerith

  • Owner set to MostAwesomeDude

comment:13 Changed 3 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 3 years ago by bartekci

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

comment:14 Changed 3 years ago by bartekci

  • Keywords review added

comment:15 Changed 3 years ago by rockstar

  • Owner changed from MostAwesomeDude to rockstar

comment:16 Changed 3 years ago by MostAwesomeDude

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

Refs #5387

comment:17 Changed 3 years ago by MostAwesomeDude

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 3 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 2 years ago by thijs

  • Cc thijs added
  • Description modified (diff)

comment:20 Changed 19 months ago by thijs

  • Owner set to thijs
  • Status changed from new to assigned

comment:21 Changed 18 months ago by thijs

  • Branch changed from branches/remove-py24-refs-5387 to branches/remove-py24-refs-5387-2

comment:22 Changed 18 months ago by thijs

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

comment:23 Changed 18 months ago by thijs

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

comment:24 Changed 18 months ago by thijs

  • Author changed from bartekci, MostAwesomeDude to bartekci, MostAwesomeDude, thijs
  • Branch changed from branches/remove-py24-refs-5387-2 to branches/remove-py24-refs-5387-3

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

comment:25 Changed 18 months ago by thijs

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

comment:26 Changed 18 months ago by thijs

  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

Forced a build, up for review.

comment:27 follow-up: Changed 17 months ago by Julian

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

comment:28 Changed 16 months ago by tom.prince

  • Owner set to tom.prince
  • Status changed from new to assigned

comment:29 follow-up: Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to thijs
  • Status changed from assigned to new
  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 16 months ago by thijs

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

comment:31 Changed 16 months ago by thijs

  • Branch changed from branches/remove-py24-refs-5387-3 to branches/remove-py24-refs-5387-4

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

comment:32 Changed 16 months ago by thijs

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

comment:33 in reply to: ↑ 29 Changed 16 months ago by thijs

  • Keywords review added
  • Owner thijs 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 16 months ago by thijs

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 15 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs
  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 13 months ago by thijs

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

comment:37 Changed 13 months ago by thijs

  • Status changed from new to assigned

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 13 months ago by thijs

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.