Opened 6 years ago

Closed 5 years ago

#3099 defect closed fixed (fixed)

signal module shouldn't be imported unless it's used

Reported by: nriley Owned by:
Priority: normal Milestone:
Component: core Keywords: jython
Cc: thijs Branch: branches/signal-module-3099
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

Description

I'm working on getting Twisted on Jython. Twisted already makes this conditional on runtime.platformType == 'posix'. Jython doesn't yet have a signal module, so this patch makes the import conditional as well.

Attachments (1)

signal.patch (872 bytes) - added by nriley 6 years ago.
patch

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by nriley

patch

comment:1 follow-up: Changed 6 years ago by glyph

In terms of a long-term solution, wouldn't wrapping sun.misc.SignalHandler for a simple 'signal' module be almost as easy?

For real inclusion, this change would need some kind of tests (or at least a Jython buildbot that we could start testing bootstrap / import issues like this on).

comment:2 in reply to: ↑ 1 ; follow-up: Changed 6 years ago by nriley

Replying to glyph:

In terms of a long-term solution, wouldn't wrapping sun.misc.SignalHandler for a simple 'signal' module be almost as easy?

sun.misc.* is not portable; however we're working on wrapping "jna-posix", a library used by JRuby, to implement the bits of Python os, signal, subprocess, etc. that we don't currently. (This is a potential SoC project for this year).

For real inclusion, this change would need some kind of tests (or at least a Jython buildbot that we could start testing bootstrap / import issues like this on).

Uhh. I'm moving an import from one place to another in a file where the usage is conditional anyway; I'm not sure what there is to test.

That said, we are working on getting a machine to use for Jython testing, on which we could likely host a Twisted buildbot. We'll let you know when it's up.

comment:3 in reply to: ↑ 2 Changed 6 years ago by glyph

Replying to nriley:

Replying to glyph:
sun.misc.* is not portable; however we're working on wrapping "jna-posix", a library used by JRuby, to implement the bits of Python os, signal, subprocess, etc. that we don't currently. (This is a potential SoC project for this year).

Given that jython could dynamically import sun.misc, it seems like a reasonable temporary workaround, but I knew it was

For real inclusion, this change would need some kind of tests (or at least a Jython buildbot that we could start testing bootstrap / import issues like this on).

Uhh. I'm moving an import from one place to another in a file where the usage is conditional anyway; I'm not sure what there is to test.

Normally I'd say any change needs to have unit tests; the reason I specifically included "a buildbot" as an alternative in this case is because issues like this that completely prevent the unit tests from bootstrapping on a particular platform can obviously be counted as a failure, assuming that people know about them ;).

Without that kind of facility available to our core developers, the chances of some future change breaking this fix are very high. (Way, way higher than you'd normally think in a library like Twisted with all these fiddly platform-specific bits - and likely to get higher now that both Jython and IronPython hackers are interested in stretching our portability to even more ridiculous heights).

That said, we are working on getting a machine to use for Jython testing, on which we could likely host a Twisted buildbot. We'll let you know when it's up.

This is really great news. For best results, drop a message to the mailing list when this is available so that one of our buildbot admins finds out about it and give you the appropriate info to get it connected to our buildmaster.

comment:4 follow-up: Changed 6 years ago by thijs

  • Cc thijs added
  • Owner changed from glyph to thijs
  • Status changed from new to assigned

There's now a Jython buildbot available at http://buildbot.twistedmatrix.com/builders/ubuntu64-jython2.5-select

Unfortunately it's not doing much at the moment because it's blocked by this jython ticket. I'll check the patch into a branch when that issue(s) is resolved.

comment:5 Changed 6 years ago by thijs

  • Keywords jython added

comment:6 Changed 6 years ago by thijs

  • author set to thijs
  • Branch set to branches/signal-module-3099

(In [24689]) Branching to 'signal-module-3099'

comment:7 in reply to: ↑ 4 Changed 6 years ago by thijs

  • Priority changed from low to normal

Replying to thijs:

Unfortunately it's not doing much at the moment because it's blocked by this jython ticket. I'll check the patch into a branch when that issue(s) is resolved.

That ticket was fixed in r5290 of Jython so the buildbot is now working. I applied the patch in r24688, and adjusted setup.py in r24688, but the build fails:

Traceback (most recent call last):
  File "./bin/trial", line 23, in <module>
    from twisted.scripts.trial import run
  File "/home/buildbot/slaves/twisted-jython/jython2.5-thijs-ubuntu/Twisted/twisted/scripts/trial.py", line 9, in <module>
    from twisted.internet import defer
  File "/home/buildbot/slaves/twisted-jython/jython2.5-thijs-ubuntu/Twisted/twisted/internet/defer.py", line 17, in <module>
    from twisted.python import log, failure, lockfile
  File "/home/buildbot/slaves/twisted-jython/jython2.5-thijs-ubuntu/Twisted/twisted/python/lockfile.py", line 28, in <module>
    from win32api import OpenProcess
ImportError: No module named win32api

comment:8 follow-up: Changed 6 years ago by exarkun

There's a ton more signal imports throughout the codebase. Maybe more than one thing should change in this branch?

On the other hand, it's pretty hard to see what effect any particular fix is having until the test suite can run. Maybe there should be a ticket for all the fixes required to make trial twisted actually produce a summary?

comment:9 in reply to: ↑ 8 Changed 6 years ago by thijs

Replying to exarkun:

Maybe there should be a ticket for all the fixes required to make trial twisted actually produce a summary?

Looks like #3413 was opened for that.

comment:10 Changed 5 years ago by thijs

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

Closing this ticket now that there's initial support for signal in Jython. The buildslave doesn't fail anymore with the win32api ImportError we saw before.

comment:11 Changed 3 years ago by <automation>

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