cfsupport.c does not build with clang (because it was generated with an old and cruddy version of pyrex)

[glyph]

on Snow Leopard:

$ CC=clang python setup.py build_ext
running build_ext
...

twisted/internet/cfsupport/cfsupport.c:128:4: error: assignment to cast is illegal, lvalue casts are not supported
  ((PyObject*)__pyx_v_socket) = Py_None; Py_INCREF(((PyObject*)__pyx_v_socket));
  ~^~~~~~~~~~~~~~~~~~~~~~~~~~ ~
twisted/internet/cfsupport/cfsupport.c:134:4: error: assignment to cast is illegal, lvalue casts are not supported
  ((PyObject *)__pyx_v_socket) = __pyx_1;
  ~^~~~~~~~~~~~~~~~~~~~~~~~~~~ ~
twisted/internet/cfsupport/cfsupport.c:829:4: error: assignment to cast is illegal, lvalue casts are not supported
  ((PyObject*)__pyx_v_obj) = Py_None; Py_INCREF(((PyObject*)__pyx_v_obj));
  ~^~~~~~~~~~~~~~~~~~~~~~~ ~
twisted/internet/cfsupport/cfsupport.c:835:4: error: assignment to cast is illegal, lvalue casts are not supported
  ((PyObject *)__pyx_v_obj) = __pyx_1;
  ~^~~~~~~~~~~~~~~~~~~~~~~~ ~
4 errors generated.

[glyph]

(In [30011]) Branching to 'clang-4652'

[glyph]

 Build results here, when they're ready.

[exarkun]

Needs a news fragment. It seems like it should be possible to have a slave running in this configuration. Do you want to install clang on d3 (or point out some docs on how to do this)? Also, how does one exercise this code?

[glyph]

clang is installed as part of the developer tools, so it should already be there.

one exercises this code by doing 'trial -r cf'. But that's not related to this ticket, which is about the build :).

[glyph]

Since the reactor itself is really experimental / unsupported ('trial -r cf twisted' terminates with a segfault in twisted.test.test_dirdbm.DirDbmTestCase.testRebuildInteraction), and this just fixes the build so that one can more easily experiment with it, I figured this deserved a 'misc' (non?) news fragment. Added that.

[exarkun]

clang is installed as part of the developer tools, so it should already be there.

It's not on $PATH, at least.

one exercises this code by doing 'trial -r cf'.

I don't have much luck with that (after building with gcc):

/Users/exarkun/Projects/Twisted/branches/clang-4652/bin/trial: The specified reactor cannot be used, failed with error: could not convert runLoop.

[glyph]

Replying to exarkun:

clang is installed as part of the developer tools, so it should already be there.

It's not on $PATH, at least.

As discussed on IRC, apparently it's in /Developer/usr/bin. Not sure why it's on my $PATH (in /usr/bin) and not yours, maybe I changed something.

one exercises this code by doing 'trial -r cf'.

I don't have much luck with that (after building with gcc): {{{ /Users/exarkun/Projects/Twisted/branches/clang-4652/bin/trial: The specified reactor cannot be used, failed with error: could not convert runLoop. }}}

This could be because you are doing this via ssh, and don't have a display up. For my part, trial -r cf twisted.internet.test completed successfully on both machines I tried it on.

[exarkun]

As discussed on IRC, apparently it's in /Developer/usr/bin. Not sure why it's on my $PATH (in /usr/bin) and not yours, maybe I changed something.

Indeed. After letting software update run, it's in /usr/bin.

trial -r cf does the same thing as before, though. :/ Running it from a context with a display (VNC) didn't change the behavior, either.

[glyph]

Replying to exarkun:

As discussed on IRC, apparently it's in /Developer/usr/bin. Not sure why it's on my $PATH (in /usr/bin) and not yours, maybe I changed something.

Indeed. After letting software update run, it's in /usr/bin.

OK, great.

trial -r cf does the same thing as before, though. :/ Running it from a context with a display (VNC) didn't change the behavior, either.

Indeed, when I tried to invoke this on my machine without any display, it still worked.

This bears further investigation, but is this behavior different with or without these changes applied, assuming you're building with GCC either way? There are at least 2 other bugs to track down here.

[exarkun]

Okay... For some reason, today things are slightly different. To summarize:

• trunk
  • builds with gcc and runs the tests up to testRebuildInteraction and then segfaults
  • does not build with clang
• the branch
  • builds with gcc and clang
  • fails with both with could not convert runLoop.

So this appears to make things slightly better with clang but somewhat worse with gcc, at least in whatever configuration d3 has.

[glyph]

(In [30189]) Branching to 'python-cfreactor-4652'

[glyph]

It turns out that (on Snow Leopard at least) the apple-packaged !PyObjC wraps all the necessary APIs to create a functional "pure"-python CoreFoundation reactor, with no C extensions of our own.

I've done that, as well as add the CFReactor to the reactorbuilder tests, which it seems to pass, so it already has some test coverage even on the existing buildbots.

Doing 'trial -r cf' yields a couple of failures, but they all look like actual test bugs. Given that the execution model of CFReactor is somewhat different than the other reactors (the main loop is not under our control; Twisted runs purely as an event handler, instead of doing the one-iteration-at-a-time thing that we do with e.g. GTK) it might be a good way to tease out race conditions in tests. It might be nice to add an unsupported builder for that just to keep an eye on it.

[glyph]

 Build Results

[glyph]

Oops.  Build Results, take 2

[exarkun]

• Can twisted.internet.cfreactor have an __all__?
• posixbase._Waker is already new-style, no need to mix in object...
• But can we skip the use of super()? Aside from providing no actual benefit to _WakerPlus.doRead, it's also not any shorter: _Waker.doRead(self) is shorter, in fact. Likewise for the other uses of super in this module.
• _WakerPlus needs a class docstring
• CFReactor needs a class docstring (including docs for its attributes)
• Actually, all the methods of CFReactor need docstrings too.
• CFReactor implements IReactorFDSet but doesn't declare that it does (twisted.internet.test.test_fdset will tell you that it's skipping all the tests for cfreactor because the interface isn't provided)
• A comment about why _preserveSOError is needed (that tcp.py depends on being able to retrieve(/destroy!) SO_ERROR) would be nice. Also comments about why the if rw[READ/WRITE] checks in _socketCallback would probably be good, since clearly it is not what people expect that CF will deliver events that were apparently disabled.
• Needs a news fragment
• You could re-fix those pyflakes warnings in twisted/test/test_tcp.py if you want :)

[glyph]

(In [30214]) __all__ as per review feedback re #4652

[glyph]

(In [30215]) _Waker doesn't need object base-class per review feedback re #4652

[glyph]

(In [30216]) Explain why we need SO_ERROR a bit better, as per review feedback re #4652

[glyph]

(In [30217]) try to explain potential duplicate notifications a bit, re #4652

[glyph]

(In [30218]) re-fix test_tcp pyflakes errors, re #4652

[glyph]

(In [30219]) module docstring as per review feedback re #4652

[glyph]

(In [30221]) Docstring for installWaker, as per review. re #4652

[glyph]

(In [30222]) re #4652 implement IReactorFDSet, add lots of docstrings, per review

[glyph]

(In [30227]) As per review, for consistency with the rest of twisted.internet, don't use super(). refs #4652

[glyph]

 Build Results, one more time.

[exarkun]

1. In the docstring for install I think the line that calls install should look like this:

install(runner=AppHelper.runEventLoop)

with this I was able to run the Twistzilla example.

2. I might try to make the news fragment a little more user friendly (do people know what the reactorbuilder tests are? do they care?).

3. I think the bit.ly URL is a weird thing to do. But if you think it really makes things better, then I guess you should leave it.

Please merge when at least point one is dealt with.

[glyph]

(In [30231]) Fix cfreactor. Remove cfsupport.

Author: glyph

Reviewer: exarkun

Fixes: #1833

Fixes: #2556

Fixes: #4652

This change significantly improves cfreactor. It is a rewrite from our broken, unmaintained 'cfsupport' pyrex/C module (which has been removed) to use PyObjC's CoreFoundation and CFNetwork support. This fixes several very serious bugs, including many segfaults.

This also changes the testing status of cfreactor. Previously it received no coverage at all from the buildbots. While this change does not add any unique test code, it adds the CFReactor to all tests which use 'reactorbuilder'. These tests would previously fail (several with segfaults) and they now all pass. The result of this is that most of the direct reactor tests are invoked on our existing OS X buildbot. It's still not 100% supported, since there is still no buildbot running under the cfreactor via 'trial -r cf'. A few tests still fail intermittently in that configuration, but I believe they are all bugs in the tests, rather than in the reactor.

This change also allows Twisted to be built with the 'clang' compiler from the LLVM project. Since 'cfsupport' was the only code that had problems with that compiler, its removal fixes the issue.

(This change also includes several unrelated flymake issues.)