Opened 2 years ago

Closed 23 months ago

#5986 enhancement closed fixed (fixed)

Remove legacy code from twisted.internet._signals, as part of port to Python 3

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/signals-5986-3
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

twisted.internet._signals and its attendant C module are mostly devoted to versions of Python before 2.6. The unnecessary code can be removed, and the remaining code ported to Python 3.

Change History (13)

comment:1 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/signals-5986

(In [35678]) Branching to 'signals-5986'

comment:2 Changed 2 years ago by itamar

This depends on #5974.

comment:3 Changed 2 years ago by itamarst

  • Branch changed from branches/signals-5986 to branches/signals-5986-2

(In [35723]) Branching to 'signals-5986-2'

comment:4 Changed 2 years ago by itamar

  • Keywords review added
  • Owner set to exarkun

OK, some code removed, and it works on Python 3. I also filed #5996. I've started a buildbot run:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/signals-5986-2

comment:5 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

There's some Windows failures.

comment:6 Changed 2 years ago by itamar

I am blocked on #4286 - if that's still a valid ticket, I need to unkill the _sigchld.c version in the code.

comment:7 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

comment:8 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  1. Would be nice to unify the signal imports in twisted/internet/_signals.py now that no tricky ImportError handling is required.
  2. Considering the existence of alternate Python runtimes like PyPy and even other platforms like BSD (with kqueue support for receiving signal notifications), and the highly intricate nature of signal handling itself, it seems likely that at some point in the future we might want to re-introduce multiple approaches, with the best selected at runtime. In this case, we'll probably want to apply the unit tests to each implementation (somewhat) automatically. Preserving the mixin-based approach to defining the tests would probably make it more obvious to a future developer how to do this, and would have been less work on this branch too. Of course, the work is done now and it is only speculative that we'll want to use the now-deleted testing infrastructure in the future. So I guess this review comment should read "Oh well".
  3. The skip of test_openFileDescriptors would be better done using the skip attribute, rather than raising SkipTest. Pre-existing defect, so just file a new ticket if you'd prefer.
  4. test_systemCallUninterruptedByChildExit was not previously skipped on all non-POSIX platforms. Why is it skipped on all non-POSIX platforms now? (As a side note, whoever allowed test_systemCallUninterruptedByChildExit to be added should be severely punished.)
  5. Minor conflict in admin/_twistedpython3.py
  6. And I'm kinda conflicted about the .removal file. We already announced we're dropping Python 2.5 support. It seems unnecessary to announce each individual change where this manifests. Is someone going to read the news file and care about this?

comment:9 Changed 2 years ago by itamarst

  • Branch changed from branches/signals-5986-2 to branches/signals-5986-3

(In [35823]) Branching to 'signals-5986-3'

comment:10 Changed 2 years ago by itamarst

(In [35825]) Address review comments. Refs #5986

comment:11 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun
  1. Done.
  2. Fair point, I'll remember next time I do this sort of thing.
  3. Done.
  4. SIGCHLD is only an issue on POSIX, which is what this testing. I removed the check, though, we'll see if it's happy in buildbot.
  5. Done.
  6. True. Switched to misc.

Started another run: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/signals-5986-3

comment:12 Changed 23 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

Great. Please merge.

comment:13 Changed 23 months ago by itamarst

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

(In [35835]) Merge signals-5986-3.

Author: itamar
Review: exarkun
Fixes: #5986, #4286

Port twisted.internet._signals to Python 3, dropping unnecessary pre-2.6 code along the way.

Note: See TracTickets for help on using tickets.