autobuild - acpid does not build against musl
PATCH 1/1 autobuild - acpid does not build against musl
Fixes http://autobuild.buildroot.net/results/33e/33ef25e4707a5b81083204e0f064570c11d88af6/
--- 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
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +76 -0
@@ -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 <[email protected]> + +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;
package/acpid/0004-fix-warnings.patch new file +38 -0
@@ -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> +
Signature · 1 line
2.4.3
PATCH 1/1 autobuild - acpid does not build against musl
Hi Brendan,
Quoted 1 line · Brendan Heading
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.
Quoted 13 lines
--- 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.patchpackage/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +0
-0
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch line 4
Quoted 4 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +4
-0@@ -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?
Quoted 2 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +2
-0+ +Signed-off-by : Brendan Heading <[email protected]>
[snip]
package/acpid/0004-fix-warnings.patch
Quoted 5 lines
package/acpid/0004-fix-warnings.patch new file +0
-0
Does this patch fix build failure or correctness? Buildroot generally doesn't carry warning fix only patches.
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 1/1 autobuild - acpid does not build against musl
Baruch,
Quoted 1 line
Please spare a few words on what this patch is doing.
Sure.
Quoted 4 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch line 4
Quoted 2 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +2
-0+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 ..
Quoted 2 lines
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
PATCH 1/1 autobuild - acpid does not build against musl
In reply to Brendan HeadingDear Brendan Heading,
The commit title should be:
acpid: fix build against musl
Quoted 1 line · Brendan Heading
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.
Quoted 13 lines
--- 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.patchpackage/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +0
-0
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch line 4
Quoted 4 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +4
-0@@ -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".
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch line 6
Quoted 2 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +2
-0+ +Signed-off-by : Brendan Heading <[email protected]>
no space between Signed-off-by and colon.
Quoted 56 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +56
-0+ +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'
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch line 72
Quoted 10 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +10
-0++ 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?
package/acpid/0004-fix-warnings.patch
Quoted 9 lines
package/acpid/0003-fix-TEMP_FAILURE_RETRY.patch new file +4
-0++ + struct rtnl_handle + { + int fd;package/acpid/0004-fix-warnings.patch new file +0
-0
As Baruch said, not needed, except if you submit the patch upstream directly.
Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 autobuild - acpid does not build against musl
Quoted 5 lines · Thomas Petazzoni
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
PATCH 1/1 autobuild - acpid does not build against musl
Brendan,
Quoted 3 lines · Brendan Heading
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.
Quoted 4 lines
The absence of
TEMP_FAILURE_RETRYwill 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 autobuild - acpid does not build against musl
Quoted 9 lines
Quoted 4 lines
The absence of
TEMP_FAILURE_RETRYwill 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