Ticket #5158 defect closed fixed

Opened 3 years ago

Last modified 2 years ago

Installing on Pypy fails because of CPython-specific extension modules

Reported by: lvh Owned by: lvh
Priority: normal Milestone:
Component: core Keywords: pypy
Cc: Branch: branches/pypy-5158
(diff, github, buildbot, log)
Author: lvh Launchpad Bug:

Description (last modified by exarkun) (diff)

Some of the extension modules (specifically the Cython extensions) are CPython specific and break on Pypy. This makes pip install -e some_twisted_checkout not work. Eventually it blows up with an error like:

/home/lvh/tmp/pypy/include/pypy_decl.h:129:29: note: expected ‘struct PyThreadState *’ but argument is of type ‘int’

There is a complete pip log attached to this ticket.

Armin Rigo has assured me this is a consequence of Cython. Cython support in Pypy is a work in progress. There are a number of potential eventual resolutions for this, but the correct thing to do right now is to disable compilation of the module on not-CPython. The right thing to do *eventually* might be re-enable it, or just use `select.epoll` as available on recent Pythons.

The affected modules are twisted.test.raiser, twisted.internet.iocpreactor.iocpsupport.iocpsupport and twisted.python.epoll`.

Attachments

pip.log Download (8.5 KB) - added by lvh 3 years ago.
5158.diff Download (2.6 KB) - added by lvh 3 years ago.
5158.2.diff Download (10.3 KB) - added by lvh 3 years ago.
twisted_std_epoll.patch Download (4.6 KB) - added by jell 2 years ago.
twisted_std_epoll.2.patch Download (5.1 KB) - added by jell 2 years ago.
fixed patch with some missing parts

Change History

Changed 3 years ago by lvh

1

Changed 3 years ago by lvh

  • keywords review added

Here's a patch. It's missing tests and docstrings because I'm not sure how to test this without shoving fake modules into sys.modules and other less-than-savory things like del sys.subversion.

If that's what has to happen, that's what I'll do, but...

Changed 3 years ago by lvh

2

Changed 3 years ago by exarkun

  • description modified (diff)

3

Changed 3 years ago by exarkun

  • owner set to lvh
  • keywords review removed

Thanks for working on this important yet often neglected part of Twisted, lvh.

  1. Yea, tests. Fortunately, I don't think this will be very hard or gross to test. Whenever the only way to proceed appears to be to jam some fakes into a global like sys.modules, the solution is usually the same - add some more parameters to something. _isCPython should accept some arguments; perhaps the platform module and the sys module would suffice? However, consider the next point first.
  2. I think you should drop the IronPython and Jython fixes from this patch. They make the change much more complicated and we presently have no chance of testing them, as we lack Jython and IronPython builders. _isCPython is a lot simpler if it doesn't know about them.
  3. _isCPython and hasEpoll should have docstrings, of course.
  4. To return to testing, I suggest moving _isCPython and hasEpoll into twisted.python._dist where they can be imported properly (and so unit tested properly). While you're doing that, you should discover that hasEpoll has a bug.
  5. It might be cool to have a CPythonExtension subclass of Extension which takes care of adding in the _isCPython check, on top of whatever other conditional it is given. On the other hand, trying to create cool distutils-based code is sort of like trying to air condition hell. Regardless, the approach you've already taken is sufficient for this ticket, so don't worry about it for now, but file another ticket for refactoring this code if you like.
  6. I haven't tried this on PyPy yet, so I'm assuming that you did and it worked. :) I notice that twisted/runner/ still builds its rpc extension, even if not on CPython. I guess that extension works on PyPy?

Please re-submit for review once you've addressed these points. Also, perhaps talk to me then about adding a build_ext step to our PyPy builder so we can see what happens there.

Thanks again!

4

Changed 3 years ago by exarkun

Hm. One other thought. twisted.python.runtime might actually be the best place for this logic. We can always move a private implementation of this detection from twisted.python.dist to a public interface in twisted.python.runtime later, though.

5

Changed 3 years ago by lvh

  • keywords review added
  1. Done
  2. Done
  3. Done
  4. Done & fixed
  5. Yeah, did the easy thing now, different ticket
  6. Yes

All new stuff is private because of your second comment and point 5.

Changed 3 years ago by lvh

6

Changed 3 years ago by lvh

  • owner lvh deleted

7

Changed 3 years ago by exarkun

  • owner set to lvh
  • keywords review removed

Probably looks good. I'd like to see the results on buildbot before suggesting to merge, though.

8

Changed 3 years ago by lvh

  • branch set to branches/pypy-5158
  • branch_author set to lvh

(In [32360]) Branching to 'pypy-5158'

9

Changed 3 years ago by lvh

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/pypy-5158

Looks like only some unrelated Windows troubles to me. So, do we do the build_ext thing now?

10

Changed 3 years ago by exarkun

11

Changed 3 years ago by lvh

Okay, recap of IRC discussion:

Problem: this fails because I have a PyPy with cpyext. Only some of the C extensions fail to build on PyPy. This one (portmap) doesn't, iff you can compile C extensions at all (it's mostly just the Cython cexts that don't work).

Solution: either we only support cpyext enabled PyPys (this is similar to how CPython works now, you need Python.h and a C compiler available as well), or we check if we're on cpyext-enabled PyPy and only compile then. I think the latter is preferable but more work. Input welcome.

12

Changed 3 years ago by lvh

Just to clarify: This fails on the buildslave but not on my machine because I have a cpyext-enabled Pypy, and the buildslave doesn't.

13

Changed 3 years ago by exarkun

The buildslave now has PyPy with cpyext. The branch succeeded a "build_ext" step on that builder:  http://buildbot.twistedmatrix.com/builders/lucid32-pypy/builds/401

14

Changed 3 years ago by exarkun

(In [32636]) docstring adjustments

banish "should" and "correctly" from your vocabulary

refs #5158

15

Changed 3 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [32639]) Merge pypy-5158

Author: lvh Reviewer: exarkun Fixes: #5158

Disable CPython extension modules which fail to build on PyPy, when building on PyPy.

16

Changed 2 years ago by jell

  • status changed from closed to reopened
  • resolution fixed deleted

Maybe that would be better solution for this bug.
With attached patch twisted cython extension mimics cpython >= 2.5 api, and twisted/internet/epollreactor is adapted to this api.
But it's needed only under cpython < 2.5

Is python 2.4 still supported by twisted? If not - this cython extension should now be deprecated or even removed.

Changed 2 years ago by jell

17

Changed 2 years ago by exarkun

  • status changed from reopened to closed
  • resolution set to fixed

Thanks for your interest in this. However, please don't re-open tickets (see ReviewProcess for details of how we use the issue tracker).

The attached patch looks like it duplicates a lot of the work in #3114.

Changed 2 years ago by jell

fixed patch with some missing parts

Note: See TracTickets for help on using tickets.