Opened 9 months ago

Last modified 3 months ago

#6831 defect new

twisted/runner/portmap.c extension module unnecessarily complicates build/install process

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: runner Keywords: review
Cc: Branch: branches/runner-cffi-6831
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Any extension module must be compiled in order to install Twisted from source. We have some distutils hacks to try to skip these when compilation will fail but the hacks are incomplete (the main known shortcoming is that they don't detect lack of Python.h - though they detect lack of other header files).

portmap.c is probably our simplest extension module (excepting perhaps raiser.c but this cannot easily be changed because its extensionness is the reason it exists). Fixing this problem in it won't completely resolve this installation speedbump but it will reduce the number of occurrences of the problem and perhaps prove a particular strategy viable.

Attachments (2)

runner-cffi.patch (3.5 KB) - added by exarkun 9 months ago.
a port of twisted.runner.portmap to cffi
portmap-setup.patch (580 bytes) - added by exarkun 9 months ago.
check for Python.h before allowing portmap compilation to be attempted

Download all attachments as: .zip

Change History (18)

Changed 9 months ago by exarkun

a port of twisted.runner.portmap to cffi

Changed 9 months ago by exarkun

check for Python.h before allowing portmap compilation to be attempted

comment:1 Changed 9 months ago by exarkun

Attached are two alternative solutions to this problem.

One replaces the C implementation of twisted.runner.portmap with a pure Python implementation based on CFFI. This adds a new CFFI dependency but moves the compilation responsibility into CFFI. A compiler is still necessary to build the module! But it is not a part of the distutils "build_ext" step. This is a mere proof of concept and requires a bit more work to build a proper distribution (details for those steps are available in the cffi documentation).

The other adds a check for "Python.h" before allowing the attempt to build the C extension module to proceed (using our already-in-place tools for this kind of check).

Of course, a third approach to solving this (which I am not supplying a patch for) is to simply delete the whole module and not try to retain the functionality.

comment:2 Changed 8 months ago by exarkun

Fixing this problem in it won't completely resolve this installation speedbump but it will reduce the number of occurrences of the problem and perhaps prove a particular strategy viable.

By this I mean that I hope that whatever fix we work out for portmap.c we can apply to the rest of the extension modules in Twisted and thereby *completely* resolve the installation speedbump.

comment:3 Changed 8 months ago by exarkun

  • Keywords review added

Since cffi is cool, I suggest we switch portmap (and eventually the other extension modules) to cffi. So, please review the cffi patch.

comment:4 Changed 8 months ago by glyph

  • Keywords review removed
  • Owner set to exarkun

This definitely looks like an improvement, but, I am compelled to note that:

  1. it has no test coverage,
  2. it changes behavior (portmap.set and portmap.unset actually return their values now!)
  3. What's the deal with the #include + source thing? The CFFI docs say it should just be the include, so this deserves at least a comment.

A bit of a cheat so that you can write a "def" but don't actually have to write unit tests for your platform's 'portmap' implementation would be to make an alias decorator, and then use it something like:

def _alias(x):
    return lambda y: x
@alias(_lib.pmap_set)
def set(program, version, protocol, port): 
    "stuff"

That way there's not actually any code in the functions that are just there for the purpose of holding a docstring.

Then the only thing that you need to do is to write something that causes it to get imported.

comment:5 Changed 7 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/runner-cffi-6831

(In [41087]) Branching to 'runner-cffi-6831'

comment:6 Changed 7 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Thanks! I gave it a try, results in the branch. Forcing a build but I suspect there will be at least two widespread problems (missing skip logic on Windows and missing cffi installation on slaves).

comment:7 Changed 7 months ago by Alex

Should setuptool's isntall_requires feature be used, when available, to simplify installation?

comment:8 Changed 7 months ago by Alex

As you projected, several of the builders failed because they could not import cffi.

comment:9 Changed 7 months ago by exarkun

Should setuptool's isntall_requires feature be used, when available, to simplify installation?

If and only if there's a way to automatically test this.

As you projected, several of the builders failed because they could not import cffi.

https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1265400

comment:10 Changed 4 months ago by dstufft

For the record, the runner-cff.patch is going to make downstream hate y'all. The cffi implicit compile is a blight and it should be avoided at all costs. As of right now CFFI doesn't actually let you make your setup.py simpler, to do it properly it currently adds complexity. There are obviously other benefits to CFFI but setup.py simplicity isn't one of them.

Also as implemented it's going to trigger a compile step anytime you import that module if it can't import it's compiled module. You can work around that but it requires some monkeypatching and other such terrible things.

comment:11 Changed 4 months ago by dstufft

Specifically the call to cffi.FFI().verify() will trigger an implicit compile. The basic way to handle this is:

Just let the implicit compile happen, but setup your setup.py so it still builds an extension (the extension built by setup.py build and the implicit compile go to different locations). This is the bare minimum you want to do so that the standard build process for the downstream OSs still work. Basically this amounts to adding to ext_modules the results of cffi.FF().verifier.get_extension(). This is made slightly tricker by the fact you have to deal with the fact you can't import the modules right away because setup_requires hasn't been installed yet. Probably the best way to handle this is by subclassing the build and install commands as cryptography.io has done.

The problem with the above is you're going to get hit with the compile step twice anytime you run setup.py build (which setup.py install will implicitly run it). Also it makes it much more difficult to diagnose problems with your build step as on your local developer machine the implicit compile will likely happen easily even if there's a bug that's prevent setup.py build from working [1]. Unfortunately cffi doesn't currently give you a lot of great tools to prevent the implicit compile all together. You basically have to construct your own cffi.Verifier instance, and monkeypatch it remove the implicit compile step, and then you also have to move the import part of cffi to happen lazily or in a different module from where you define your ffi and verifier. There is an example of this here: https://github.com/pyca/pynacl/blob/master/src/nacl/_lib/__init__.py#L47-L86. Basically what you're looking for is the ability to delay the cffi.FFI().verifier.load_library() call to happen at runtime and not when you import things in your setup.py.

Now both of these things are made more annoying if your library has other dependencies to make import itself work. I'm not off the cuff familar with Twisted but for instance if you require zope.interface to even import up to the point where your Verifier object is then you'll need to add that to setup_requires as well.Depending on the complexity of things you could possible get away with using exec to exec the python file that holds the verifier if that file itself doesn't have any dependencies itself.

comment:12 Changed 4 months ago by dstufft

Oh, forgot to add. I actually discovered a CFFI bug where the extension built by setup.py build wasn't being found by the cffi.FFI().verifier.load_library() and it took me several hours to diagnose it because I couldn't make the build failure occur locally because the implicit compile kept happening. After I monkeypatched away implicit compile I found the problem relatively quickly.

comment:13 Changed 4 months ago by dstufft

Oh, and you'll also want to adopt the modulename shenanigans that cryptography.io has done. By default CFFI makes the generated file name a hash of several things, one of which is the *exact* version of CFFI that was used to compile the module. This will cause lots of problems when you upgrade CFFI without recompiling everything that was built with that CFFI because they'll all try to implicit compile the next time they run (which may or may not succeed depending on permissions, whats installed, and if you left implicit compiled enable). This will also make it very difficult to install two binary wheels of things that are written using CFFI because for instance a Twisted wheel might be built using cffi X.Y.Z, but a cryptography wheel built using cffi X.Y.Y and you can't install them both at the same time.

Link to shenanigans:

https://github.com/pyca/cryptography/blob/master/cryptography/hazmat/bindings/utils.py#L75

https://github.com/pyca/cryptography/blob/master/cryptography/hazmat/bindings/utils.py#L91-L105

comment:14 Changed 3 months ago by Alex

  • Keywords review removed

Patch LGTM, please get cffi installed on the builders, verify tests pass, and merge.

comment:15 Changed 3 months ago by Alex

This also needs a topfile.

comment:16 Changed 3 months ago by exarkun

  • Keywords review added

Not sure this really got reviewed.

Note: See TracTickets for help on using tickets.