Opened 5 years ago

Closed 4 years ago

#3725 enhancement closed fixed (fixed)

Remove Jython platform detection

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords: jython
Cc: thijs, ltaylor.volks, jessica.mckellar@… Branch: branches/jython-platformdetection-remove-3725-2
(diff, github, buildbot, log)
Author: cyli Launchpad Bug:

Description

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.

Change History (31)

comment:1 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords jython added

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

comment:2 Changed 5 years ago by 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

comment:3 Changed 4 years ago by ltaylor.volks

  • Cc ltaylor.volks added

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

comment:4 Changed 4 years ago by cyli

  • Author set to cyli
  • Branch set to branches/jython-platformdetection-remove-3725

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

comment:5 Changed 4 years ago by cyli

  • Keywords review added
  • Owner glyph deleted

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.

comment:6 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:7 follow-up: Changed 4 years ago by jesstess

  • Cc jessica.mckellar@… added
  • Keywords review removed
  • Owner changed from jesstess to cyli

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.

comment:8 Changed 4 years ago by 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. :)

comment:9 in reply to: ↑ 7 Changed 4 years ago by 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.

comment:10 Changed 4 years ago by 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

comment:11 Changed 4 years ago by cyli

  • Keywords review added

comment:12 Changed 4 years ago by cyli

  • Owner cyli deleted

comment:13 follow-up: Changed 4 years ago by jesstess

  • Keywords review removed
  • Owner set to cyli

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.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by 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! :)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by 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.

comment:16 Changed 4 years ago by 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

comment:17 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by cyli

  • Keywords review added
  • Owner cyli deleted

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?

comment:18 in reply to: ↑ 17 Changed 4 years ago by glyph

  • Keywords review removed
  • Owner set to cyli

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.

comment:19 Changed 4 years ago by cyli

  • Keywords review added

comment:20 Changed 4 years ago by cyli

  • Owner cyli deleted

comment:21 Changed 4 years ago by glyph

  • Keywords review removed
  • Owner set to cyli

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!

comment:22 Changed 4 years ago by cyli

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

(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

comment:23 Changed 4 years ago by 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.

comment:24 Changed 4 years ago by cyli

  • Resolution fixed deleted
  • Status changed from closed to reopened

(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

comment:25 Changed 4 years ago by cyli

  • Branch changed from branches/jython-platformdetection-remove-3725 to branches/jython-platformdetection-remove-3725-2

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

comment:26 Changed 4 years ago by cyli

  • Keywords review added
  • Owner cyli deleted
  • Status changed from reopened to new

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.

comment:27 Changed 4 years ago by cyli

  • Status changed from new to reopened

(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

comment:28 Changed 4 years ago by 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

comment:29 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to cyli
  • Status changed from reopened to new

Looks good, please merge, thanks!

comment:30 Changed 4 years ago by cyli

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

(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

comment:31 Changed 3 years ago by <automation>

  • Owner cyli deleted
Note: See TracTickets for help on using tickets.