Opened 10 months ago

Last modified 6 hours ago

#6831 defect new

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

Reported by: exarkun Owned by: glyph
Priority: normal Milestone:
Component: runner Keywords:
Cc: tobias.oberstein@… 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 10 months ago.
a port of twisted.runner.portmap to cffi
portmap-setup.patch (580 bytes) - added by exarkun 10 months ago.
check for Python.h before allowing portmap compilation to be attempted

Download all attachments as: .zip

Change History (29)

Changed 10 months ago by exarkun

a port of twisted.runner.portmap to cffi

Changed 10 months ago by exarkun

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

comment:1 Changed 10 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 9 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 9 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 9 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 8 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 8 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 8 months ago by Alex

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

comment:8 Changed 8 months ago by Alex

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

comment:9 Changed 8 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 5 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 5 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 5 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 5 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 5 months ago by Alex

  • Keywords review removed

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

comment:15 Changed 5 months ago by Alex

This also needs a topfile.

comment:16 Changed 5 months ago by exarkun

  • Keywords review added

Not sure this really got reviewed.

comment:17 Changed 3 weeks ago by glyph

  • Keywords review removed
  • Owner set to exarkun

The patch doesn't actually LGTM :). I guess you were right about Alex's review not really being a review :).

  1. We're trying to make Twisted have fewer dependencies; requiring cffi may be functionally the same as requiring a C compiler in many circumstances. Particularly it requires a functioning libffi installation, and that is definitely not present on all our supported operating systems by default right now. So I think this should be an optional dependency (at least for now) if it's going to serve its stated purpose, in addition to getting the stuff on the buildbots.
  2. python setup.py build_ext -i doesn't build this extension any more, which suggests that the implicit compile is the only way that this gets loaded. In the situation where users do have a C compiler, this will make things a lot worse - for example, ubuntu will build Twisted, it won't have this, and the system installed unit tests won't pass. (Not quite sure that they pass right now but moving further away from that would definitely be the wrong direction.) To land, this really ought to use cffi's setup.py integration so that the extension gets built as a normal extension. I think this is what dstufft was talking about in his comments but he did ramble a bit there :-).
  3. This serves as an interesting case study for starting to use cffi rather than C modules, but I'm inclined to say that we should go with the portmap-setup.patch approach to address the most pressing issue first, then land the @_alias decorator somewhere in twisted.python as private functionality for future cffi (and similar) experiments to use, just to keep the patches nice and small, and finally, land the CFFI rewrite once we've figured out the setup.py mess.

Thoughts?

comment:18 follow-up: Changed 3 weeks ago by exarkun

  • Owner changed from exarkun to glyph

Thanks for the review.

We're trying to make Twisted have fewer dependencies

Hmm. I wasn't trying to do that. Perhaps that effort is underway somewhere else? It seems contrary to some other ideas/discussion I've encountered, though. We seem to be accepting that some valuable code can live outside of Twisted and it's okay for Twisted to rely on it.

requiring cffi may be functionally the same as requiring a C compiler in many circumstances

As I understand it, in *all* circumstances - if you are considering both build- and run-time.

portmap.c had a hard dependency on a C compiler as well, of course. I think portmap.c was supposed to be optional but we didn't actually manage to make it so (eg we managed to skip building it on Windows but not on POSIX if you were missing Python.h). A goal of "only be available if the dependencies are satisfied" seems great to me. I think this is what we should aim for to resolve this ticket.

in addition to getting the stuff on the buildbots

This raises the question of whether we are going to try to properly test optionalness on buildbot or not. The current approach is to semi-randomly install a semi-random subset of semi-random versions of our dependencies on semi-randomly selected slaves. This isn't working so well, I think (for example, it produces certain builds which only fail *sometimes* - depending on which slave they happen to run on, the negative consequences of which I think you have noticed).

python setup.py build_ext -i doesn't build this extension any more

This is clearly bad. I've fixed it for twisted/runner/topfiles/setup.py in the branch. Unfortunately because of the way our setup.pys work, it also needs to be fixed in the top-level setup.py. I haven't done this because it looks tedious and annoying.

which suggests that the implicit compile is the only way that this gets loaded

To be clear, I agree that the "implicit compile" feature of cffi is kind of meh and relying on it as the *only* way to get the extension module built would be a terrible mistake.

I think this is what dstufft was talking about in his comments but he did ramble a bit there :-).

Thanks for interpreting. I didn't manage to understand anything from his comment.

I'm inclined to say that we should go with the portmap-setup.patch approach to address the most pressing issue first

I disagree with this but if you or someone else wants to take this ticket in that direction, please feel free (I'd appreciate it if you file a new ticket for cffi'ing portmap and recovering the changes from this branch which do that into a new branch for that ticket so that the work isn't lost - because I'm pretty sure that cffi is definitely the way to go in the long run for code like this).

then land the @_alias decorator somewhere in twisted.python as private functionality for future cffi (and similar) experiments to use

This doesn't make much sense to me either. I'd much prefer to see _alias (or something else) successfully used for something real before putting it in Twisted. I don't see the benefit to putting unproven private infrastructure into the codebase - it won't do any more good there than it will in a branch.

just to keep the patches nice and small

Small patches are the best. The diff for this branch is still well under 300 lines though. I don't think the branch is yet in danger of becoming unreviewably large (how many thousand-plus line changes have been landed elsewhere in Twisted recently?).

I probably won't pursue the work here in the near future. I started it mainly to try to do something nice for Alex but it seems like he never really got very interested. I don't have much other motivation to work on this particular area of Twisted and anything involving distutils has an automatic 10 point demotivator attached.

comment:19 in reply to: ↑ 18 Changed 3 weeks ago by glyph

Replying to exarkun:

Thanks for the review.

We're trying to make Twisted have fewer dependencies

Hmm. I wasn't trying to do that. Perhaps that effort is underway somewhere else? It seems contrary to some other ideas/discussion I've encountered, though. We seem to be accepting that some valuable code can live outside of Twisted and it's okay for Twisted to rely on it.

Sorry, that was super unclear.

What I meant was - the purpose of this ticket is to make Twisted be minimally functionally correct in the face of a missing dependency, to wit, a C compiler. Therefore, in the service of this particular problem it would be bad to add additional required dependencies, especially when we don't have any infrastructure to automatically note that dependency.

requiring cffi may be functionally the same as requiring a C compiler in many circumstances

As I understand it, in *all* circumstances - if you are considering both build- and run-time.

portmap.c had a hard dependency on a C compiler as well, of course. I think portmap.c was supposed to be optional but we didn't actually manage to make it so (eg we managed to skip building it on Windows but not on POSIX if you were missing Python.h). A goal of "only be available if the dependencies are satisfied" seems great to me. I think this is what we should aim for to resolve this ticket.

+1.

in addition to getting the stuff on the buildbots

This raises the question of whether we are going to try to properly test optionalness on buildbot or not. The current approach is to semi-randomly install a semi-random subset of semi-random versions of our dependencies on semi-randomly selected slaves. This isn't working so well, I think (for example, it produces certain builds which only fail *sometimes* - depending on which slave they happen to run on, the negative consequences of which I think you have noticed).

The py-without-modules buildbot seems like a reasonable compromise between "easy to set up" and "tests requirements in a useful way", although a more disciplined approach to the build matrix would be better. Random variances in builders which are supposed to be "the same", like the two different configurations of py3 builders, are just bad and we should eliminate them.

Ideally, what I'd like is: a buildbot on each platform without a C compiler installed (especially Windows) that gets its dependencies from PyPI, or at least a local wheelhouse.

python setup.py build_ext -i doesn't build this extension any more

This is clearly bad. I've fixed it for twisted/runner/topfiles/setup.py in the branch. Unfortunately because of the way our setup.pys work, it also needs to be fixed in the top-level setup.py.

I thought that the top-level setup.py... like... aggregated stuff? (Why doesn't it?)

I haven't done this because it looks tedious and annoying.

Understandable :-\.

which suggests that the implicit compile is the only way that this gets loaded

To be clear, I agree that the "implicit compile" feature of cffi is kind of meh and relying on it as the *only* way to get the extension module built would be a terrible mistake.

Great. I wasn't sure if this was unintentional, or partially implemented, or what. I didn't get the impression that it was fully on purpose based on the previous discussion.

I think this is what dstufft was talking about in his comments but he did ramble a bit there :-).

Thanks for interpreting. I didn't manage to understand anything from his comment.

When glyph tells you you're rambling, it's time to tighten up that prose. Seriously.

I'm inclined to say that we should go with the portmap-setup.patch approach to address the most pressing issue first

I disagree with this but if you or someone else wants to take this ticket in that direction, please feel free (I'd appreciate it if you file a new ticket for cffi'ing portmap and recovering the changes from this branch which do that into a new branch for that ticket so that the work isn't lost - because I'm pretty sure that cffi is definitely the way to go in the long run for code like this).

I do think that cffi'ing portmap is generally a more worthwhile investment of effort, but it looks like it's going to be a lot more effort. Nevertheless, addressing the issues that are blocking this could un-block a lot of useful stuff, like getting rid of pywin32 and IOCP cython junk and calling fun platform APIs like initgroups and so on.

then land the @_alias decorator somewhere in twisted.python as private functionality for future cffi (and similar) experiments to use

This doesn't make much sense to me either. I'd much prefer to see _alias (or something else) successfully used for something real before putting it in Twisted. I don't see the benefit to putting unproven private infrastructure into the codebase - it won't do any more good there than it will in a branch.

Smaller changes, easier to independently review. We can review the private functionality, and then future branches can use it without including big changes.

I would not mind landing this as private infrastructure by itself in anticipation of its use, but if you'd rather keep it off of trunk until we need it, we can just have a ticket with the relevant code in a branch, and then land it in advance of whatever concrete branch wants to start using it.

just to keep the patches nice and small

Small patches are the best. The diff for this branch is still well under 300 lines though. I don't think the branch is yet in danger of becoming unreviewably large

It's not that I think it's unreviewably large, it's that there are other cffi things we might want to land

(how many thousand-plus line changes have been landed elsewhere in Twisted recently?).

Two wrongs doesn't make a right :-).

I probably won't pursue the work here in the near future. I started it mainly to try to do something nice for Alex but it seems like he never really got very interested. I don't have much other motivation to work on this particular area of Twisted and anything involving distutils has an automatic 10 point demotivator attached.

OK. Hopefully someone else (donald?) can be convinced to help address the build and integration issues. If we could deal with that, the cffi code itself looks fine and I would like to land it.

comment:20 Changed 3 days ago by oberstet

  • Cc tobias.oberstein@… added

comment:21 Changed 3 days ago by oberstet

Moving to CFFI - in general: +1. In this case, I doesn't help.

At least it wouldn't help me: installing Twisted on a tiny MIPS embedded system (Arduino Yun). Requires manual patching of the setup.py to turn off building stuff that needs a C compiler. Building CFFI requires a C compiler as well. Catch 22.

What would help: having a command line option to turn off the portmap and any other C stuff.

Until then I need to ask users to patch setup.py manually. Which is kinda stretch and pain for that kind of user audience (Yun / makers scene).

Ah, and the Twisted pkg blob avail for that platform (not only MIPS Lineo, but also OpenWRT) is ages old. No option either.

comment:22 Changed 2 days ago by glyph

tobias, any chance you would be interested in taking a crack at testing the setup.py change or sending a buildbot configuration PR that tests installation without a C compiler?

comment:23 Changed 23 hours ago by oberstet

I have tested the setup.py patch (https://twistedmatrix.com/trac/attachment/ticket/6831/portmap-setup.patch). It's not enough. Still fails.

Here is a log of what I did.

The system is

root@Arduino:~# uname -a
Linux Arduino 3.8.3 #8 Mon Aug 19 16:22:39 CEST 2013 mips GNU/Linux

root@Arduino:~# cat /proc/cpuinfo
system type             : Atheros AR9330 rev 1
machine                 : Arduino Yun
processor               : 0
cpu model               : MIPS 24Kc V7.4
BogoMIPS                : 265.42
...

Here is what happens with Twisted 14.0.0 vanilla:

root@Arduino:~/Twisted-14.0.0# python setup.py install
...
copying twisted/runner/portmap.c -> build/lib.linux-mips-2.7/twisted/runner
running build_ext
mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c conftest.c -o conftest.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
building 'twisted.test.raiser' extension
creating build/temp.linux-mips-2.7
creating build/temp.linux-mips-2.7/twisted
creating build/temp.linux-mips-2.7/twisted/test
mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c twisted/test/raiser.c -o build/temp.linux-mips-2.7/twisted/test/raiser.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
error: command 'mips-openwrt-linux-uclibc-gcc' failed with exit status 1

After applying the setup.py patch, I get this

creating build/lib.linux-mips-2.7/twisted/internet/iocpreactor/iocpsupport
copying twisted/internet/iocpreactor/iocpsupport/iocpsupport.c -> build/lib.linux-mips-2.7/twisted/internet/iocpreactor/iocpsupport
copying twisted/internet/iocpreactor/iocpsupport/winsock_pointers.c -> build/lib.linux-mips-2.7/twisted/internet/iocpreactor/iocpsupport
copying twisted/test/raiser.c -> build/lib.linux-mips-2.7/twisted/test
copying twisted/python/sendmsg.c -> build/lib.linux-mips-2.7/twisted/python
copying twisted/runner/portmap.c -> build/lib.linux-mips-2.7/twisted/runner
running build_ext
mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c conftest.c -o conftest.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
building 'twisted.test.raiser' extension
creating build/temp.linux-mips-2.7
creating build/temp.linux-mips-2.7/twisted
creating build/temp.linux-mips-2.7/twisted/test
mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c twisted/test/raiser.c -o build/temp.linux-mips-2.7/twisted/test/raiser.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
error: command 'mips-openwrt-linux-uclibc-gcc' failed with exit status 1

I now patched twisted/internet/iocpreactor/setup.py to

setup(name='iocpsupport',
      ext_modules=[Extension('iocpsupport',
                   ['iocpsupport/iocpsupport.pyx',
                    'iocpsupport/winsock_pointers.c'],
                   condition=lambda builder: builder._check_header("Python.h"),
                   libraries = ['ws2_32'],
                   )
                  ],
      cmdclass = {'build_ext': build_ext},
      )

Now I get this

mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c conftest.c -o conftest.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
building 'twisted.test.raiser' extension
mips-openwrt-linux-uclibc-gcc -fno-strict-aliasing -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -DNDEBUG -Os -pipe -mips32r2 -mtune=mips32r2 -fno-caller-saves -mno-branch-likely -fhonour-copts -Wno-error=unused-but-set-variable -msoft-float -fPIC -I/usr/include/python2.7 -c twisted/test/raiser.c -o build/temp.linux-mips-2.7/twisted/test/raiser.o
unable to execute mips-openwrt-linux-uclibc-gcc: No such file or directory
error: command 'mips-openwrt-linux-uclibc-gcc' failed with exit status 1

It fails building that raiser.c thing. Gave up.

===

This is what works: comment out the following line in the top level setup.py

#        conditionalExtensions=getExtensions(),

comment:24 Changed 23 hours ago by oberstet

Ideally, Twisted setup (IMO) would do

  • check if Python.h is there _and_ if a C compiler is there
  • if so, build compiled stuff
  • if no, build _none_ of the compiled stuff

Or have a (manual) command line option that skips all compiled stuff. Which is even better - pls see below.

Or: kick these compiled things altogether;)

portmap: exarkun told me some guys might still need this. but really, come on. portmap? it's 2014. there is no way I personally would run such a security hole / attack surface in production.

iocp: why does it even _try_ to build this when this is a Linux box? mmh. but this is a module that would profit from CFFI - IMO.

raiser: I have no clue what this is. I've skimmed the C source. I don't understand it, but it seems cpyext extreme magic. Mmh.

Then there is

./twisted/python/_initgroups.c
./twisted/python/sendmsg.c

The latter is a Linux sendmsg cpyext wrapper. Does that actually work? I mean, can we use sendmsg in Twisted apps?

The initgroups.c thing wraps a Unix group management function. Code is from Zope anno 2002. Huh.

To me, the innocent and naive, most of above (everything but IOCP) appears quite exotic ..

====

There is one more aspect: people might want to build Twisted without any compiled stuff even when there is a C compiler. Because of _security_ reasons.

Running a system on a stack of pure Python modules makes security reasoning easier. Some admins might even _require_ a pure Python module stack for this reason.

comment:25 Changed 18 hours ago by exarkun

Hi oberstet, thanks for your input here.

portmap: exarkun told me some guys might still need this. but really, come on. portmap? it's 2014. there is no way I personally would run such a security hole / attack surface in production.

Did I? I don't recall. Regardless, Twisted has a compatibility policy. Feel free to suggest deprecating this module. However, as you note, this doesn't solve the larger issue since Twisted includes other extension modules.

iocp: why does it even _try_ to build this when this is a Linux box? mmh. but this is a module that would profit from CFFI - IMO.

Because we use distutils. The default behavior in distutils is to break. Surely you've noticed this by now? Please don't expand the scope of this ticket beyond portmap, though. It's obviously difficult enough to fix the behavior of one specific trivial extension module. Let's just tackle one thing at a time, please?

From your earlier comment, it sounds to me like the problems you ran into with this patch are that:

  1. it doesn't fix iocp
  2. it doesn't fix raiser.c

If these are the only problems then I'd say the patch is working perfectly and should be accepted now as-is. It is only a patch to fix *portmap.c* after all. If the fix works for portmap.c then we can apply the change and start thinking about how to fix those other modules.

If there are other problems with *portmap.c* that this patch doesn't address then we should certainly address them. Are there? (I think there are - there are the cffi-related issues glyph mentioned which I agree should be addressed, but I note you didn't complain about any of those.)

comment:26 Changed 17 hours ago by oberstet

Hi Jean-Paul, fixing portmap still leaves me with these problems

  • icop
  • raiser
  • sendmsg
  • _initgroups

Essentially everything *.c


Taking the risk of being stubborn (I did read what you wrote):

Wouldn't it be easier to fix the actual (larger) problem "Twisted cannot be installed from source without a C compiler" by adding a _manual option_ that skips _all_ C stuff?

I mean, I manually patch the top level setup.py for conditionalExtensions = []. If that could be made without patching, but from a command line option, that would "solve" the portmap thing _and_ all the others as well.

"solve" == skip.

By making it a manual option, there wouldn't be backward compatibility problems. Without the option explicitly set, anything stays as today.


Ok, let's get more concrete. I found http://stackoverflow.com/questions/677577/distutils-how-to-pass-a-user-defined-parameter-to-setup-py

Here is a proposal (top level setup.py):

    setup_args.update(dict(
        packages=getPackages('twisted'),
        scripts=scripts,
        data_files=getDataFiles('twisted'),
        **STATIC_PACKAGE_METADATA))

    if not "--skip-c-exts" in args:
        setup_args.update(conditionalExtensions=getExtensions())
    else:
        print("Skipping building of C extensions")
        sys.argv.remove("--skip-c-exts")

And this works for me then (tested on actual Arudino Yun device - and only there):

python setup.py install --skip-c-exts

comment:27 Changed 6 hours ago by exarkun

Wouldn't it be easier to fix the actual (larger) problem "Twisted cannot be installed from source without a C compiler" by adding a _manual option_ that skips _all_ C stuff?

Would it be easier? I don't know. In practice, this isn't an answerable question. We would need to implement both solutions completely and carefully measure the effort that goes into each. Only afterwards would we know which is easier. Since I think the point of your question is that you'd like to direct efforts towards the easier solution because total available effort is limited, this is probably counter-productive: figuring out what is easier involves doing roughly twice as much work (likely not exactly twice as much since that supposes the two solutions are just as much work to implement, but close enough).

So let's please disregard the question of what is easier (unless you are trying to direct attention in that direction for some other reason).

Should we add a --skip-c-exts option to each setup.py (remember, there is more than one of these) which entirely disables building of extensions?

That is an interesting idea. That is not an approach I considered before, perhaps for a couple reasons:

  1. It forces the user to learn about the option and then use it in certain circumstances.
  1. Documenting the option in the place a user is most likely to look for documentation - --help output - is likely to be difficult because --help is implemented inside distutils and probably not easy to extend. Not impossible, certainly - we could always catch --help ourselves and play some tricks.
  1. Somewhat related to (2) - it diverges Twisted's setup.py from everyone else's setup.py. There is some expectation of setup.py being a standard interface. This violates that.

That said, it has something pretty strongly in its favor: if we don't define any extension modules in our call to distutils, distutils will almost certainly (someone can verify this, perhaps) not try to compile anything. This should definitely solve the issue.

However, does it solve the issue better than teaching the extension definitions to fail gracefully? It's arguably a worse user experience. It also doesn't cater to more nuanced configurations - for example, it doesn't let you have sendmsg if you can't build portmap. It doesn't let us remove our existing detection code - because you can't ever have iocp and sendmsg on the same platform. Perhaps support such sophisticated usage is beyond our grasp - but I don't think so.

Hi Jean-Paul, fixing portmap still leaves me with these problems

To wrap up, please note that this ticket is not intended to make Twisted build/install on a platform without Python.h or without a C compiler or with some other defect which prevents the compilation and build of extension modules. It is only intended to make sure that portmap is not the cause of such a failure - hopefully in way which suggests a solution which can be applied to the rest of Twisted to actually solve the user-facing problem.

Note: See TracTickets for help on using tickets.