| Message ID | 1437417346-18621-1-git-send-email-brendanheading@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Brendan, On Mon, Jul 20, 2015 at 07:35:46PM +0100, Brendan Heading wrote: > Fixes http://autobuild.buildroot.net/results/33e/33ef25e4707a5b81083204e0f064570c11d88af6/ Please spare a few words on what this patch is doing. Also, your sign-off is missing. > --- > package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch | 76 +++++++++++++++++++++++++ > package/acpid/0004-fix-warnings.patch | 38 +++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > create mode 100644 package/acpid/0004-fix-warnings.patch > > diff --git a/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > new file mode 100644 > index 0000000..7d81b3c > --- /dev/null > +++ b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > @@ -0,0 +1,76 @@ > +Add missing TEMP_FAILURE_RETRY. > + > +Some external toolchains using strange C libraries (eg MUSL) do not define > +this macro. What is the upstream status of this patch? > + > +Signed-off-by : Brendan Heading <brendanheading@gmail.com> [snip] > diff --git a/package/acpid/0004-fix-warnings.patch b/package/acpid/0004-fix-warnings.patch > new file mode 100644 > index 0000000..3c6a89d > --- /dev/null > +++ b/package/acpid/0004-fix-warnings.patch Does this patch fix build failure or correctness? Buildroot generally doesn't carry warning fix only patches. baruch
Baruch, > Please spare a few words on what this patch is doing. Sure. >> +Some external toolchains using strange C libraries (eg MUSL) do not define >> +this macro. > > What is the upstream status of this patch? I've not checked. Should I do so before resubmitting ? I must admit I wasn't too sure about the idea of the patch here, as it fixes only one package rather than addressing an issue that will effect any package using the same macro. But I noted a precedent for this (in the same package, there is a patch to add a macro missing from unpatched uclibc versions configured as part of an external toolchain) so I went ahead and submitted. I can certainly raise it with the upstream devs .. > Does this patch fix build failure or correctness? Buildroot generally doesn't > carry warning fix only patches. Warnings only. I will remove. Thanks for the feedback on my first ever patch :) Regards Brendan
Dear Brendan Heading, The commit title should be: acpid: fix build against musl On Mon, 20 Jul 2015 19:35:46 +0100, Brendan Heading wrote: > Fixes http://autobuild.buildroot.net/results/33e/33ef25e4707a5b81083204e0f064570c11d88af6/ Generally, we prefer: Fixes: http://autobuild.buildroot.net/results/33e/33ef25e4707a5b81083204e0f064570c11d88af6/ Please add your Signed-off-by line as well. > --- > package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch | 76 +++++++++++++++++++++++++ > package/acpid/0004-fix-warnings.patch | 38 +++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > create mode 100644 package/acpid/0004-fix-warnings.patch > > diff --git a/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > new file mode 100644 > index 0000000..7d81b3c > --- /dev/null > +++ b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch > @@ -0,0 +1,76 @@ > +Add missing TEMP_FAILURE_RETRY. > + > +Some external toolchains using strange C libraries (eg MUSL) do not define > +this macro. musl is not "strange". > + > +Signed-off-by : Brendan Heading <brendanheading@gmail.com> no space between Signed-off-by and colon. > + > +Index : b/acpid.h > +=================================================================== > +--- a/acpid.h > ++++ b/acpid.h > +@@ -39,6 +39,18 @@ > + > + #define PACKAGE "acpid" > + > ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' > ++ set to EINTR. */ > ++ > ++#ifndef TEMP_FAILURE_RETRY > ++#define TEMP_FAILURE_RETRY(expression) \ > ++ (__extension__ \ > ++ ({ long int __result; \ > ++ do __result = (long int) (expression); \ > ++ while (__result == -1L && errno == EINTR); \ > ++ __result; })) > ++#endif > ++ > + /* > + * acpid.c > + */ > +Index : b/kacpimon/libnetlink.h > +=================================================================== > +--- a/kacpimon/libnetlink.h > ++++ b/kacpimon/libnetlink.h > +@@ -11,6 +11,18 @@ > + #define MSG_CMSG_CLOEXEC 0x40000000 > + #endif > + > ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' > ++ set to EINTR. */ > ++ > ++#ifndef TEMP_FAILURE_RETRY > ++#define TEMP_FAILURE_RETRY(expression) \ > ++ (__extension__ \ > ++ ({ long int __result; \ > ++ do __result = (long int) (expression); \ > ++ while (__result == -1L && errno == EINTR); \ > ++ __result; })) > ++#endif > ++ > + struct rtnl_handle > + { > + int fd; > +Index : b/libnetlink.h > +=================================================================== > +--- a/libnetlink.h > ++++ b/libnetlink.h > +@@ -11,6 +11,18 @@ > + #define MSG_CMSG_CLOEXEC 0x40000000 > + #endif > + > ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' > ++ set to EINTR. */ > ++ > ++#ifndef TEMP_FAILURE_RETRY > ++#define TEMP_FAILURE_RETRY(expression) \ > ++ (__extension__ \ > ++ ({ long int __result; \ > ++ do __result = (long int) (expression); \ > ++ while (__result == -1L && errno == EINTR); \ > ++ __result; })) > ++#endif Is it really necessary to repeat that three times? Can we put that in some common header? > ++ > + struct rtnl_handle > + { > + int fd; > diff --git a/package/acpid/0004-fix-warnings.patch b/package/acpid/0004-fix-warnings.patch > new file mode 100644 > index 0000000..3c6a89d > --- /dev/null > +++ b/package/acpid/0004-fix-warnings.patch As Baruch said, not needed, except if you submit the patch upstream directly. Thanks! Thomas
On 20 July 2015 at 21:41, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Brendan Heading, > > The commit title should be: > > acpid: fix build against musl Hi Thomas, I think the best course with this problem is to have a chat with the upstream musl guys (first choice) or patch the buildroot musl toolchain (second choice). The absence of TEMP_FAILURE_RETRY will break more than one package so in retrospect it's probably more cleaner to try to fix it properly rather than add patches to every package where it shows up. The only downside is that acpid will be broken until it's resolved. regards Brendan
Brendan, On Tue, 21 Jul 2015 01:16:16 +0100, Brendan Heading wrote: > I think the best course with this problem is to have a chat with the > upstream musl guys (first choice) or patch the buildroot musl > toolchain (second choice). Neither of this is going to happen, I believe. TEMP_FAILURE_RETRY is a glibc-ism, and the musl developers are generally not really interested in adding glibc-isms. Patching the buildroot musl toolchain is also not acceptable, as it means we would no longer support musl external toolchains that don't carry our patches. > The absence of TEMP_FAILURE_RETRY will break more than one package so > in retrospect it's probably more cleaner to try to fix it properly > rather than add patches to every package where it shows up. The only > downside is that acpid will be broken until it's resolved. But that's still the only reasonable solution. The idea being that those patches should be contributed to the upstream acpid project, so that we can drop them from Buildroot the next time acpid makes an official release. Best regards, Thomas
>> The absence of TEMP_FAILURE_RETRY will break more than one package so >> in retrospect it's probably more cleaner to try to fix it properly >> rather than add patches to every package where it shows up. The only >> downside is that acpid will be broken until it's resolved. > > But that's still the only reasonable solution. The idea being that > those patches should be contributed to the upstream acpid project, so > that we can drop them from Buildroot the next time acpid makes an > official release. I've raised a ticket on acpid2 and asked to submit my patch there. RE some other issues you highlighted, such as duplicating the macro in several places. There is no single header file which is included by all sources; further, acpid2 has copies of the same header file in two different directories, so I followed what you had already done for a previous case (if you look in the package directory you can see your patch which solves a similar problem in the same way). I could add another header just for this macro and include it from both places but that seems untidy and not consistent with what they currently do. I can submit a new patch here or get a steer from upstream first - which is your preference ? regards Brendan
diff --git a/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file mode 100644 index 0000000..7d81b3c --- /dev/null +++ b/package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch @@ -0,0 +1,76 @@ +Add missing TEMP_FAILURE_RETRY. + +Some external toolchains using strange C libraries (eg MUSL) do not define +this macro. + +Signed-off-by : Brendan Heading <brendanheading@gmail.com> + +Index : b/acpid.h +=================================================================== +--- a/acpid.h ++++ b/acpid.h +@@ -39,6 +39,18 @@ + + #define PACKAGE "acpid" + ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' ++ set to EINTR. */ ++ ++#ifndef TEMP_FAILURE_RETRY ++#define TEMP_FAILURE_RETRY(expression) \ ++ (__extension__ \ ++ ({ long int __result; \ ++ do __result = (long int) (expression); \ ++ while (__result == -1L && errno == EINTR); \ ++ __result; })) ++#endif ++ + /* + * acpid.c + */ +Index : b/kacpimon/libnetlink.h +=================================================================== +--- a/kacpimon/libnetlink.h ++++ b/kacpimon/libnetlink.h +@@ -11,6 +11,18 @@ + #define MSG_CMSG_CLOEXEC 0x40000000 + #endif + ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' ++ set to EINTR. */ ++ ++#ifndef TEMP_FAILURE_RETRY ++#define TEMP_FAILURE_RETRY(expression) \ ++ (__extension__ \ ++ ({ long int __result; \ ++ do __result = (long int) (expression); \ ++ while (__result == -1L && errno == EINTR); \ ++ __result; })) ++#endif ++ + struct rtnl_handle + { + int fd; +Index : b/libnetlink.h +=================================================================== +--- a/libnetlink.h ++++ b/libnetlink.h +@@ -11,6 +11,18 @@ + #define MSG_CMSG_CLOEXEC 0x40000000 + #endif + ++/* Evaluate EXPRESSION, and repeat as long as it returns -1 with `errno' ++ set to EINTR. */ ++ ++#ifndef TEMP_FAILURE_RETRY ++#define TEMP_FAILURE_RETRY(expression) \ ++ (__extension__ \ ++ ({ long int __result; \ ++ do __result = (long int) (expression); \ ++ while (__result == -1L && errno == EINTR); \ ++ __result; })) ++#endif ++ + struct rtnl_handle + { + int fd; diff --git a/package/acpid/0004-fix-warnings.patch b/package/acpid/0004-fix-warnings.patch new file mode 100644 index 0000000..3c6a89d --- /dev/null +++ b/package/acpid/0004-fix-warnings.patch @@ -0,0 +1,38 @@ +diff --git a/acpi_listen.c b/acpi_listen.c +index d0bc175..cc28a43 100644 +--- a/acpi_listen.c ++++ b/acpi_listen.c +@@ -32,7 +32,7 @@ + #include <ctype.h> + #include <getopt.h> + #include <time.h> +-#include <sys/poll.h> ++#include <poll.h> + #include <grp.h> + #include <signal.h> + +diff --git a/event.c b/event.c +index 324078f..6dfa57d 100644 +--- a/event.c ++++ b/event.c +@@ -23,7 +23,7 @@ + #include <sys/types.h> + #include <sys/stat.h> + #include <sys/wait.h> +-#include <sys/poll.h> ++#include <poll.h> + #include <fcntl.h> + #include <unistd.h> + #include <stdio.h> +diff --git a/sock.c b/sock.c +index aaba8cd..eb4a3b4 100644 +--- a/sock.c ++++ b/sock.c +@@ -30,6 +30,7 @@ + #include <fcntl.h> + #include <stdio.h> + #include <stdlib.h> ++#include <string.h> + #include <errno.h> + #include <grp.h> +