Ticket #4652 enhancement closed fixed

Opened 4 years ago

Last modified 3 years ago

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

Reported by: glyph Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/python-cfreactor-4652
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description

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.

Change History

1

  Changed 4 years ago by glyph

  • branch set to branches/clang-4652
  • branch_author set to glyph

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

2

  Changed 4 years ago by glyph

  • keywords review added
  • owner glyph deleted

 Build results here, when they're ready.

3

  Changed 4 years ago by exarkun

  • keywords review removed
  • owner set to glyph

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?

4

  Changed 4 years ago by 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 :).

5

  Changed 4 years ago by glyph

  • owner glyph deleted
  • keywords review added

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.

6

follow-up: ↓ 7   Changed 4 years ago by 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.

7

in reply to: ↑ 6   Changed 4 years ago by 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.

8

follow-up: ↓ 9   Changed 4 years ago by 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.

9

in reply to: ↑ 8   Changed 4 years ago by 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.

10

  Changed 4 years ago by exarkun

  • owner set to glyph
  • keywords review removed

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.

11

  Changed 3 years ago by glyph

  • branch changed from branches/clang-4652 to branches/python-cfreactor-4652

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

12

  Changed 3 years ago by glyph

  • keywords review added
  • owner glyph deleted

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.

13

  Changed 3 years ago by glyph

14

  Changed 3 years ago by glyph

15

  Changed 3 years ago by exarkun

  • owner set to glyph
  • keywords review removed
  • 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 :)

16

  Changed 3 years ago by glyph

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

17

  Changed 3 years ago by glyph

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

18

  Changed 3 years ago by glyph

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

19

  Changed 3 years ago by glyph

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

20

  Changed 3 years ago by glyph

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

21

  Changed 3 years ago by glyph

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

22

  Changed 3 years ago by glyph

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

23

  Changed 3 years ago by glyph

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

24

  Changed 3 years ago by glyph

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

25

  Changed 3 years ago by glyph

  • keywords review added
  • owner glyph deleted

 Build Results, one more time.

26

  Changed 3 years ago by exarkun

  • keywords review removed
  • owner set to glyph

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.

27

  Changed 3 years ago by glyph

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

(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.)

28

  Changed 3 years ago by <automation>

  • owner glyph deleted
Note: See TracTickets for help on using tickets.