Remove Jython platform detection

[exarkun]

Recent versions of Jython are totally different from old versions of Jython which Twisted supported. The current code is mostly just getting in the way of things. We should drop it so that it's clear what we should do in order to support the new version of Jython.

[thijs]

Are you referring to the code in setup.py? Where can this Jython platform detection be found?

[exarkun]

I mean everything throughout Twisted which has a special case in order to support Jython. So, for example:

• twisted.spread.jelly, around line 121
• twisted.internet._javaserialport
• twisted.python.threadpool, around line 60

[ltaylor.volks]

As of changeset:12375, there is no javareactor, so twisted.internet._javaserialport won't work at all and should be able to be safely removed, along with the import in twisted.internet.serialport (line 62):

elif os.name == 'java':
    from twisted.internet._javaserialport import SerialPort

[cyli]

(In [28688]) Branching to 'jython-platformdetection-remove-3725'

[cyli]

Removed relevant platform checks from spread/jelly.py, internet/serialport.py, and python/threadpool.py (also changed test_threadpool to test deprecation of ThreadSafeList). And removed the _javaserialport.py file.

[jesstess]

Thanks for working on this, cyli. The meat of the ticket work looks great. Some feedback:

• I might reword the deprecation warning for ThreadSafeList to make it sound less like Twisted fosters dubious and broken code. :) Maybe something like "This code supported the now obsolete Jython 2.1 and is deprecated to make way for support for newer Jython versions". Maybe with less verbosity.
• 3725.removal needs a final carriage return
• test_threadpool.py and threadpool.py have trailing whitespace
• does serialport.py really not have any test cases? I ask because it doesn't have a test case name at the top, but grepping around I didn't see anything. Too bad.
• the for loop at the bottom of test_threadpool.py for setting skip uses DeprecationTestCase when the test class's name is ThreadSafeListDeprecationTestCase. This  doesn't appear to have actually broken anything, so is the surrounding interface check useless?
• pyflakes warnings: test_threadpool.py imports Version but doesn't use it. threadpool.py imports runtime but doesn't use it. serialport.py has a bunch of unused imports. jelly.py imports runtime but doesn't use it.
• grepping through the source, I see a couple of other places where something about Java is special-cased, although I didn't check if they fall under the umbrella of this ticket: setup.py, twisted/python/runtime.py, and twisted/test/test_pb.py.

[exarkun]

3725.removal needs a final carriage return

Probably a good idea, but the news generator won't actually care. It reformats the text to fit in the news file anyway.

does serialport.py really not have any test cases?

Sad and true. But #2462, #3690, #4249. :)

[cyli]

* I might reword the deprecation warning for ThreadSafeList to make it sound less like Twisted fosters dubious and broken code. :) Maybe something like "This code supported the now obsolete Jython 2.1 and is deprecated to make way for support for newer Jython versions". Maybe with less verbosity.

Ah ok, I was given to understand that it never worked in the first place, even for Jython 2.1. Have fixed.

* 3725.removal needs a final carriage return

Fixed

* test_threadpool.py and threadpool.py have trailing whitespace

Fixed

* does serialport.py really not have any test cases? I ask because it doesn't have a test case name at the top, but grepping around I didn't see anything. Too bad.

Oh well. :) Maybe I will look at those tickets some time (perhaps not near) in the future.

* the for loop at the bottom of test_threadpool.py for setting skip uses DeprecationTestCase when the test class's name is ThreadSafeListDeprecationTestCase. This  doesn't appear to have actually broken anything, so is the surrounding interface check useless?

Huh, you are correct. I had originally named it DeprecationTestCase, and then changed it to be more clear, but apparently none of that stuff mattered. So I took out the interface check, and moved the "else" imports up top.

* pyflakes warnings: test_threadpool.py imports Version but doesn't use it. threadpool.py imports runtime but doesn't use it. serialport.py has a bunch of unused imports. jelly.py imports runtime but doesn't use it.

threadpool.py also imports runtime but doesn't use it. Removed the unused imports from all mentioned files.

* grepping through the source, I see a couple of other places where something about Java is special-cased, although I didn't check if they fall under the umbrella of this ticket: setup.py, twisted/python/runtime.py, and twisted/test/test_pb.py.

Regarding setup.py - extensions are a CPython thing, and I don't think they're needed in Java? Anyway, I thought this ticket covered Twisted itself, rather than the installation/distribution/compilation, so I thought altering setup.py should be a different ticket.

The java-related stuff in twisted/python/runtime.py is regarding what platform Twisted is running on - it isn't special-casing anything exactly, it's just providing information.

Regarding twisted/test/test_pb.py: I originally interpreted this as not being covered by this ticket, because the test seemed more like the inverse of support for jython - it was explicitly not messing with jython's garbage collection, which I thought may have been intentional somehow because perhaps garbage collection would always be special in every version of jython. But checking the <a href=" http://www.jython.org/docs/library/gc.html">current jython docs regarding the garbage collector</a>, I see I'm probably wrong, so I've removed the java stuff in test_pb.py.

[cyli]

(In [28767]) Fixed whitespace/extra imports/carriage returns as per review, and removed some extra Java special case code + broken test code

refs #3725 -This line, and those below, will be ignored--

M twisted/test/test_pb.py M twisted/test/test_threadpool.py M twisted/python/threadpool.py M twisted/topfiles/3725.removal M twisted/internet/serialport.py M twisted/spread/jelly.py

[jesstess]

test_pb.py needs a copyright bump, and test_threadpool.py could use a module-level docstring. Other than that, this branch looks great and ready to merge! Thanks for working on this, cyli.

[cyli]

Replying to jesstess:

test_pb.py needs a copyright bump, and test_threadpool.py could use a module-level docstring. Other than that, this branch looks great and ready to merge! Thanks for working on this, cyli.

Does that mean I should make the changes and kick it back into review, or change and merge? Thanks! :)

[thijs]

Replying to cyli:

Replying to jesstess:

test_pb.py needs a copyright bump, and test_threadpool.py could use a module-level docstring. Other than that, this branch looks great and ready to merge! Thanks for working on this, cyli.

Does that mean I should make the changes and kick it back into review, or change and merge? Thanks! :)

Can you put it back up for review and also:

• replace the 'Twisted 10.1.0' reference with 'Twisted 10.1'
• you mention that in 3725.removal that 'all internal Jython platform detection removed' but there's still a check in setup.py (that needs to stay) which skips building extensions on jython, so technically not all platform detection has been removed imo. Can you rephrase that and do something like 'in Twisted core' or 'across all projects' instead?

Thanks.

[cyli]

(In [28859]) Change deprecation version to 10.1 instead of 10.0, copyright bump, and other minor branch description fixes as per review.

refs #3725

[cyli]

Apologies for taking so long for such minor changes - have been both extremely busy and lazy. Made the following modifications:

test_pb.py needs a copyright bump test_threadpool.py could use a module-level docstring - replace the 'Twisted 10.1.0' reference with 'Twisted 10.1' - you mention that in 3725.removal that 'all internal Jython platform detection removed' but there's still a check in setup.py (that needs to stay) which skips building extensions on jython, so technically not all platform detection has been removed imo. Can you rephrase that and do something like 'in Twisted core' or 'across all projects' instead?

[glyph]

Replying to cyli:

- replace the 'Twisted 10.1.0' reference with 'Twisted 10.1'

I don't understand why thijs asked for this; the standard is currently to have all of major/minor/micro versions listed in the deprecation warning so that the version is precisely identified. In any case, Version requires a micro-version argument to its constructor, so lots of tests now fail in this branch with this traceback:

  File "twisted/python/threadpool.py", line 289, in ThreadSafeList
    Version("Twisted", 10, 1),
exceptions.TypeError: __init__() takes at least 5 arguments (4 given)

So please put the '.0' back.

[glyph]

Unfortunately the Jython buildbot is offline right now, so I can't see if this branch makes any difference, but the other  build results are encouraging, so please go ahead and merge!

[cyli]

(In [28981]) Merge jython-platformdetection-remove-3725: jython platform detection removal

Author: cyli Reviewer: jesstess, thijs, glyph Fixes: #3725

twisted.python.threadpool.ThreadSafeList is deprecated and Jython platform detection in Twisted core removed

[exarkun]

twisted.internet.serialport used to export these names:

PARITY_NONE, PARITY_EVEN, PARITY_ODD, STOPBITS_ONE, STOPBITS_TWO, FIVEBITS, SIXBITS, SEVENBITS, EIGHTBITS

Abstractly, the removal is an incompatible change.

It appears as though at least one project is using them (fizzjik). So it's also a real-world problem. I think this should be reverted and the incompatible removals dropped.

[cyli]

(In [28984]) Revert r28981: Revert jython platform detection removal

twisted.internet.serialport used to export these names:

PARITY_NONE, PARITY_EVEN, PARITY_ODD, STOPBITS_ONE, STOPBITS_TWO, FIVEBITS, SIXBITS, SEVENBITS, EIGHTBITS

Abstractly, the removal is an incompatible change.

It appears as though at least one project is using them (fizzjik). So it's also a real-world problem. I think this should be reverted and the incompatible removals dropped.

Reopens: #3725

[cyli]

(In [28986]) Branching to 'jython-platformdetection-remove-3725-2'

[cyli]

Reverted. I've also added an __all__ to explicitly export these variables so that the next person who runs pyflakes doesn't remove the imports.

[cyli]

(In [28984]) Revert r28981: Revert jython platform detection removal

twisted.internet.serialport used to export these names:

PARITY_NONE, PARITY_EVEN, PARITY_ODD, STOPBITS_ONE, STOPBITS_TWO, FIVEBITS, SIXBITS, SEVENBITS, EIGHTBITS

Abstractly, the removal is an incompatible change.

It appears as though at least one project is using them (fizzjik). So it's also a real-world problem. I think this should be reverted and the incompatible removals dropped.

Reopens: #3725

[cyli]

(In [28984]) Revert r28981: Revert jython platform detection removal

twisted.internet.serialport used to export these names:

PARITY_NONE, PARITY_EVEN, PARITY_ODD, STOPBITS_ONE, STOPBITS_TWO, FIVEBITS, SIXBITS, SEVENBITS, EIGHTBITS

Abstractly, the removal is an incompatible change.

It appears as though at least one project is using them (fizzjik). So it's also a real-world problem. I think this should be reverted and the incompatible removals dropped.

Reopens: #3725

[exarkun]

Looks good, please merge, thanks!

[cyli]

(In [28995]) Merge jython-platformdetection-remove-3725-2: removing jython platform detection

Author: cyli Reviewer: jesstess, thijs, glyph, exarkun Fixes: #3725

twisted.python.threadpool.ThreadSafeList is deprecated and Jython platform detection in Twisted core removed