Setup.py (install) partly fails on OpenBSD due to unresolved compilation issues (sendmsg.c)

[epdittmer]

The installation (setup.py install) of Twisted-12.1.0 partly fails do to an unresolved compilation issue in sendmsg.c.

At least on OpenBSD (version 5.1) the iovec struct is undefined. Including sys/uio.h resolves the issue.

Compilation output:

cc -pthread -fno-strict-aliasing -O2 -pipe -DNDEBUG -O2 -pipe \
    -DTHREAD_STACK_SIZE=0x20000 -fPIC -fPIC -I/usr/local/include/python2.7 \
    -c twisted/python/sendmsg.c \
    -o build/temp.openbsd-5.1-amd64-2.7/twisted/python/sendmsg.o
twisted/python/sendmsg.c: In function 'sendmsg_sendmsg':
twisted/python/sendmsg.c:174: error: array type has incomplete element type
twisted/python/sendmsg.c: In function 'sendmsg_recvmsg':
twisted/python/sendmsg.c:369: error: array type has incomplete element type
error: command 'cc' failed with exit status 1

[thijs]

Putting this patch up for review, see ReviewProcess.

Another sendmsg.c issue: #5907

[thijs]

Replying to thijs:

Putting this patch up for review, see ReviewProcess. Another sendmsg.c issue: #5907

Should be #5936.

[exarkun]

Thanks for your contribution. However, while "it compiles" is necessary it is not sufficient. Do the unit tests pass with this change?

[teratorn]

So I just hit this bug today, and came up with the same patch to fix it. Before finding this ticket.

It's great to know this issue has be languishing here for 3 months, and missed inclusion in Twisted 12.2. Because apparently we need unit tests to tell us that including the right C header files is a good thing to do.

exarkun, what is your point exactly? and what can we do to resolve it? The unit test for a build problem is the build system itself (afaict). If the build system moves forward toward a more working state, then I consider that "test passed."

[glyph]

Replying to teratorn:

It's great to know this issue has be languishing here for 3 months, and missed inclusion in Twisted 12.2. Because apparently we need unit tests to tell us that including the right C header files is a good thing to do.

teratorn, snarky comments about the review process really aren't helpful, and I would thank you to please never do this on a ticket again. If you want to complain, come up with some constructive criticism, not sarcasm, and put it in a dedicated place for such discussion, like a new, dedicated thread on the mailing list, or a chat on the IRC channel; don't derail an existing discussion.

Better yet though, just ask a question without the pointless rudeness. If you don't know how the review process works, or what our acceptance criteria are, or why they're there, I'm always happy to explain, provide references, et cetera.

exarkun, what is your point exactly? and what can we do to resolve it? The unit test for a build problem is the build system itself (afaict). If the build system moves forward toward a more working state, then I consider that "test passed."

In the broader context, exarkun's point is that OpenBSD is not a supported platform and therefore we don't know anything about this patch. As you can see on our  full buildbot status page, we have only unsupported builders on OpenBSD, and they've been offline for hundreds of builds; we really have no idea whether this platform works.

In a much narrower sense, though, all exarkun asked for at this point in the conversation was assurance that somebody, somewhere had even run the existing unit tests with the change here. In the last 6 weeks, nobody, including yourself, thought this bug was important enough to reply.

We are never, ever, ever going to change Twisted's quality-control process to allow random people who claim to have problems that we don't have the infrastructure to reproduce to apply dubious changes with no effect. Maybe this breaks compilation on some other random, unsupported platform. Every change is a risk, especially changes to C code. This change is no different. Maybe it breaks builds on a supported platform. Maybe it's detrimental to performance. The fact is that we don't know, and there's no clear indication of a benefit; someone has to do the work necessary to demonstrate a benefit.

If you care about OpenBSD, please work with the OpenBSD buildbots' maintainer (soyt, apparently?) to get them back online, or contribute some new ones yourself, and work towards getting a full clean run of the tests on those bots, so we can put them in the "supported" category and prevent this sort of breakage in the future.

I will assume that you care enough about this bug to get it fixed now though, so I'll reassign it to you.

(epdittmer, if you intend to deal with this feedback soon, and want to take it back, by all means please do so.)

[teratorn]

Replying to glyph:

Replying to teratorn:

It's great to know this issue has be languishing here for 3 months, and missed inclusion in Twisted 12.2. Because apparently we need unit tests to tell us that including the right C header files is a good thing to do.

teratorn, snarky comments about the review process really aren't helpful, and I would thank you to please never do this on a ticket again. If you want to complain, come up with some constructive criticism, not sarcasm, and put it in a dedicated place for such discussion, like a new, dedicated thread on the mailing list, or a chat on the IRC channel; don't derail an existing discussion.

Im sure I wouldn't feel the need to do so again.. but I do hope something useful comes out of this discussion - I have been somewhat dissatisfied with our review process regarding such patches as this, e.g. where we lack a buildslave, where it seems the process holds up important fixes for ages. Twisted won't even install on OpenBSD right now. That is kind of embarrassing.

exarkun, what is your point exactly? and what can we do to resolve it? The unit test for a build problem is the build system itself (afaict). If the build system moves forward toward a more working state, then I consider that "test passed."

In the broader context, exarkun's point is that OpenBSD is not a supported platform and therefore we don't know anything about this patch. As you can see on our  full buildbot status page, we have only unsupported builders on OpenBSD, and they've been offline for hundreds of builds; we really have no idea whether this platform works.

Yes, that is certainly a problem.

In a much narrower sense, though, all exarkun asked for at this point in the conversation was assurance that somebody, somewhere had even run the existing unit tests with the change here. In the last 6 weeks, nobody, including yourself, thought this bug was important enough to reply.

OK Glyph - that's fair - in fact I attempted to run Twisted's full set of unit tests on the system I had available at the time. Unfortunately after an hour of CPU time it failed with MemoryError (2GB system).. *shrug*

I try to find a more powerful machine to run the test suite on, with patch applied.

We are never, ever, ever going to change Twisted's quality-control process to allow random people who claim to have problems that we don't have the infrastructure to reproduce to apply dubious changes with no effect.

Sorry, I think that is a huge mis-characterization of this patch. Sorry.

Maybe this breaks compilation on some other random, unsupported platform. Every change is a risk, especially changes to C code. This change is no different. Maybe it breaks builds on a supported platform. Maybe it's detrimental to performance. The fact is that we don't know, and there's no clear indication of a benefit; someone has to do the work necessary to demonstrate a benefit.

I wasn't trying to suggest we would ever accept a patch, like this (or any other), without positive test suite runs at least somewhere, by someone, but anyway, I think the reporter has demonstrated the benefit well. And in my original post above, I concurred with the efficacy of the patch on the platform in question.

If you care about OpenBSD, please work with the OpenBSD buildbots' maintainer (soyt, apparently?) to get them back online, or contribute some new ones yourself, and work towards getting a full clean run of the tests on those bots, so we can put them in the "supported" category and prevent this sort of breakage in the future.

Yes, I think I'll take your suggestion to heart, and see about getting some decent hardware to run an OpenBSD buildslave or two.

I will assume that you care enough about this bug to get it fixed now though, so I'll reassign it to you.

Sure, thanks. and thanks for caring! ;)

[teratorn]

(In [36540]) Branching to 'bsd-include-header-5907'

[teratorn]

buildbot run of this branch:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/bsd-include-header-5907

I've attached test run errors from openbsd 5.2/i386. It appears these errors/failures are not related to the patch in question.

There *is* one sendmsg-related test failure, but it doesn't appear to be too critical, and may possibly be just a platform quirk (I'm not sure).

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/teratorn/code/TX/lib/python2.7/site-packages/twisted/internet/defer.py", line 138, in maybeDeferred
    result = f(*args, **kw)
  File "/home/teratorn/code/TX/lib/python2.7/site-packages/twisted/internet/_utilspy3.py", line 41, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/home/teratorn/code/TX/lib/python2.7/site-packages/twisted/internet/_utilspy3.py", line 37, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/teratorn/code/TX/lib/python2.7/site-packages/twisted/python/test/test_sendmsg.py", line 541, in test_unix
    self.assertEqual(AF_UNIX, getsockfam(self._socket(AF_UNIX)))
  File "/home/teratorn/code/TX/lib/python2.7/site-packages/twisted/trial/_synctest.py", line 356, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = 1
b = 224


twisted.python.test.test_sendmsg.GetSocketFamilyTests.test_unix
===============================================================================

All the other sendmsg tests are passing.

All this branch does is allow Twisted to build/install on OpenBSD. Allowing the sendmsg.c module to be built and enabled does not appear to cause any regressions over the pure-Python code paths.

OK for review..

[teratorn]

I should have just considered this effort a review effort from the start, since I'm not an author of the patch - going to complete the review process now :)

[teratorn]

• I've added missing news file
• The attached test errors highlight several test failures that should be filed as separate tickets (apparently unrelated to this issue)
• The need for a couple OpenBSD buildslaves is very apparent - since Twisted does have some OpenBSD users it appears. Filed as #6210
• No test regressions are apparent to me on the branch build.
• In particular the FreeBSD builder is green, and the additional #include directives don't seem to bother it (it _does_ #define BSD right?)

Thanks epdittmer, I'll merge this shortly!

[teratorn]

(In [36563]) Merge bsd-include-header-5907: sendmsg.c now building on OpenBSD

Author: epdittmer Reviewer: teratorn Fixes: #5907

[rodrigc]

It looks like the branches/bsd-include-header-5907 got merged to trunk already, but I thought I would add some comments.

Adding an #include of <sys/uio.h> is fine, since it seems to be necessary to pull in the definition of "struct iovec". Take this for what it is worth:  http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/uio.h.html

However, I think wrapping it in #ifdef BSD is perhaps not the best way to go.

Although, this *seems to just work*, it is not clear where BSD is getting defined. It looks like you are relying on the BSD macro to be set somehow by the inclusion of other header files.

Since <sys/uio.h> is on {Free,Net,Open,DragonFly}BSD and MacOS X, you need to be careful about this kind of stuff.

I would recommend a slight modification to this patch:

Index: twisted/python/sendmsg.c
===================================================================
--- twisted/python/sendmsg.c    (revision 36590)
+++ twisted/python/sendmsg.c    (working copy)
@@ -18,7 +18,7 @@

 #include <sys/param.h>

-#ifdef BSD
+#ifdef HAVE_SYS_UIO_H
 #include <sys/uio.h>
 #endif

Index: twisted/python/dist.py
===================================================================
--- twisted/python/dist.py      (revision 36590)
+++ twisted/python/dist.py      (working copy)
@@ -367,6 +367,10 @@
             self.define_macros = [("WIN32", 1)]
         else:
             self.define_macros = []
+
+        if self._check_header("sys/uio.h"):
+            self.define_macros.extend([("HAVE_SYS_UIO_H", None)])
+
         self.extensions = [x for x in self.conditionalExtensions
                            if x.condition(self)]
         for ext in self.extensions:

This makes things a bit more clear to read.

The result of this patch is that -DHAVE_SYS_UIO_H will be added to the compilation line for all C files. It is OK to do this, because on Win32, in the same function we add -DWIN32 to the compilation flags for all C files.

Alternatively, if you don't like all this, I would recommend unconditionally #include'ing <sys/uio.h> in sendmsg.c, since it is available on all the BSD's, MacOS X, *and* Linux:  http://www.kernel.org/doc/man-pages/online/pages/man2/writev.2.html

The patch would look like this:

Index: twisted/python/sendmsg.c
===================================================================
--- twisted/python/sendmsg.c    (revision 36590)
+++ twisted/python/sendmsg.c    (working copy)
@@ -17,10 +17,7 @@
 #include <signal.h>

 #include <sys/param.h>
-
-#ifdef BSD
 #include <sys/uio.h>
-#endif

 /*
  * As per

One difference between Linux and BSD, is that on Linux, often more header files will get sucked in when you include a single header file, while on BSD less header files get sucked in by including a single header file. Consequently, on BSD the client .c file is required to explicitly include more of these header files.