| Message ID | b8ef63602bd75ed7fe6b8d87d48e5d36680f3c91.1469505637.git.baruch@tkos.co.il |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, On Tue, 26 Jul 2016 07:00:37 +0300, Baruch Siach wrote: > Fixes: > http://autobuild.buildroot.net/results/6cf/6cf75e08795d9ab194ce4e882c0f4858bad979c3/ > http://autobuild.buildroot.net/results/964/964d8f694bc7d05b35411eabfbadf40bbf6337ae/ > http://autobuild.buildroot.net/results/0f2/0f2cddf89af3ad4330556acd04ab6cb080370e24/ > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > package/ipkg/Config.in | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/package/ipkg/Config.in b/package/ipkg/Config.in > index 4b2078d9cd70..d93e1d17b3dd 100644 > --- a/package/ipkg/Config.in > +++ b/package/ipkg/Config.in > @@ -1,5 +1,6 @@ > config BR2_PACKAGE_IPKG > bool "ipkg" > + depends on BR2_USE_MMU # fork() I think this needs more investigation, because: BR2_bfin=y BR2_TOOLCHAIN_EXTERNAL=y BR2_INIT_NONE=y # BR2_PACKAGE_BUSYBOX is not set BR2_PACKAGE_IPKG=y # BR2_TARGET_ROOTFS_TAR is not set which is a noMMU configuration, is able to build ipkg just fine.
Hello, Sorry, last e-mail sent mistakenly. On Tue, 26 Jul 2016 09:59:29 +0200, Thomas Petazzoni wrote: > I think this needs more investigation, because: > > BR2_bfin=y > BR2_TOOLCHAIN_EXTERNAL=y > BR2_INIT_NONE=y > # BR2_PACKAGE_BUSYBOX is not set > BR2_PACKAGE_IPKG=y > # BR2_TARGET_ROOTFS_TAR is not set > > which is a noMMU configuration, is able to build ipkg just fine. I was indeed a bit surprised to see this patch: we have been having Blackfin configurations for a long time in the autobuilders, and we never had to add a BR2_USE_MMU dependency on this package. So I believe there's something a bit more subtle going on here. Thomas
Hi Thomas, On Tue, Jul 26, 2016 at 10:01:01AM +0200, Thomas Petazzoni wrote: > On Tue, 26 Jul 2016 09:59:29 +0200, Thomas Petazzoni wrote: > > I think this needs more investigation, because: > > > > BR2_bfin=y > > BR2_TOOLCHAIN_EXTERNAL=y > > BR2_INIT_NONE=y > > # BR2_PACKAGE_BUSYBOX is not set > > BR2_PACKAGE_IPKG=y > > # BR2_TARGET_ROOTFS_TAR is not set > > > > which is a noMMU configuration, is able to build ipkg just fine. > > I was indeed a bit surprised to see this patch: we have been having > Blackfin configurations for a long time in the autobuilders, and we > never had to add a BR2_USE_MMU dependency on this package. > > So I believe there's something a bit more subtle going on here. Right. It's this code from libbb/libbb.h: /* Cope with mmu-less systems somewhat gracefully */ #if defined(__UCLIBC__) && !defined(__ARCH_HAS_MMU__) #define fork vfork #endif This breaks musl that does not define __UCLIBC__. I posted an updated patch to uses HAVE_FORK instead. Is there anyone actually using this package anyway? Even its homepage is not accessible here (handhelds.org does not resolve). Thanks for reviewing, baruch
Hello, On Tue, 26 Jul 2016 14:15:57 +0300, Baruch Siach wrote: > Right. It's this code from libbb/libbb.h: > > /* Cope with mmu-less systems somewhat gracefully */ > #if defined(__UCLIBC__) && !defined(__ARCH_HAS_MMU__) > #define fork vfork > #endif > > This breaks musl that does not define __UCLIBC__. I posted an updated patch to > uses HAVE_FORK instead. We don't use musl on any noMMU platform today, so this certainly cannot explain failures like: http://autobuild.buildroot.net/results/6cf/6cf75e08795d9ab194ce4e882c0f4858bad979c3/ (which was the first one mentioned in your commit log), since this failure happens with uClibc. Looking at http://autobuild.buildroot.net/?reason=ipkg-0.99.163, I see (looking only at the failures since the beginning of 2016) : * Numerous failures on ARM noMMU (uClibc) * Two failures on m68k noMMU (uClibc) * An old failure on i686 due to download issue i.e, none of the failures are caused by a musl-related build. Thomas
Hi Thomas, On Tue, Jul 26, 2016 at 01:48:01PM +0200, Thomas Petazzoni wrote: > On Tue, 26 Jul 2016 14:15:57 +0300, Baruch Siach wrote: > > Right. It's this code from libbb/libbb.h: > > > > /* Cope with mmu-less systems somewhat gracefully */ > > #if defined(__UCLIBC__) && !defined(__ARCH_HAS_MMU__) > > #define fork vfork > > #endif > > > > This breaks musl that does not define __UCLIBC__. I posted an updated patch to > > uses HAVE_FORK instead. > > We don't use musl on any noMMU platform today, so this certainly cannot > explain failures like: > > http://autobuild.buildroot.net/results/6cf/6cf75e08795d9ab194ce4e882c0f4858bad979c3/ > > (which was the first one mentioned in your commit log), since this > failure happens with uClibc. > > Looking at http://autobuild.buildroot.net/?reason=ipkg-0.99.163, I see > (looking only at the failures since the beginning of 2016) : > > * Numerous failures on ARM noMMU (uClibc) > * Two failures on m68k noMMU (uClibc) > * An old failure on i686 due to download issue > > i.e, none of the failures are caused by a musl-related build. You are right of course, forgive my sloppiness. To be correct for uClibc the code should test for __ARCH_USE_MMU__ instead of __ARCH_HAS_MMU__. But testing HAVE_FORK is better, I thing, since it also covers other hypothetical MMU-less C libraries. I'll resend the patch with a correct description. baruch
Hello, On Tue, 26 Jul 2016 15:14:29 +0300, Baruch Siach wrote: > You are right of course, forgive my sloppiness. To be correct for uClibc the > code should test for __ARCH_USE_MMU__ instead of __ARCH_HAS_MMU__. But testing > HAVE_FORK is better, I thing, since it also covers other hypothetical MMU-less > C libraries. Using HAVE_FORK indeed seems better. However, can you verify that it actually works for MMU-capable platforms? What worries me is that the code in libbb/ originally comes from Busybox, which doesn't use autoconf. HAVE_FORK is a #define value defined by ipkg's autoconf configure script, in config.h, and I'm not sure if the libbb/ code includes config.h. So, please make sure that HAVE_FORK is really taken into account when building on MMU-capable platforms by adding some #error in the #if defined(HAVE_FORK) test in the libbb code that you're changing. Thanks! Thomas
Hi Thomas, On Tue, Jul 26, 2016 at 02:52:48PM +0200, Thomas Petazzoni wrote: > On Tue, 26 Jul 2016 15:14:29 +0300, Baruch Siach wrote: > > You are right of course, forgive my sloppiness. To be correct for uClibc > > the code should test for __ARCH_USE_MMU__ instead of __ARCH_HAS_MMU__. But > > testing HAVE_FORK is better, I thing, since it also covers other > > hypothetical MMU-less C libraries. > > Using HAVE_FORK indeed seems better. However, can you verify that it > actually works for MMU-capable platforms? > > What worries me is that the code in libbb/ originally comes from > Busybox, which doesn't use autoconf. HAVE_FORK is a #define value > defined by ipkg's autoconf configure script, in config.h, and I'm not > sure if the libbb/ code includes config.h. > > So, please make sure that HAVE_FORK is really taken into account when > building on MMU-capable platforms by adding some #error in the #if > defined(HAVE_FORK) test in the libbb code that you're changing. I verified that config.h macros are defined in libbb.h before posting the last patch, by using another defined macros. I repeated this test as you suggested, with HAVE_FORK and the current br-arm-full.config, just to be sure. The #error triggers as expected in both tests. baruch
Hello, On Tue, 26 Jul 2016 18:08:54 +0300, Baruch Siach wrote: > I verified that config.h macros are defined in libbb.h before posting the last > patch, by using another defined macros. I repeated this test as you suggested, > with HAVE_FORK and the current br-arm-full.config, just to be sure. The #error > triggers as expected in both tests. Excellent, thanks! Thomas
> On Jul 26, 2016, at 5:14 AM, Baruch Siach <baruch@tkos.co.il> wrote: > > Hi Thomas, > > On Tue, Jul 26, 2016 at 01:48:01PM +0200, Thomas Petazzoni wrote: >> On Tue, 26 Jul 2016 14:15:57 +0300, Baruch Siach wrote: >>> Right. It's this code from libbb/libbb.h: >>> >>> /* Cope with mmu-less systems somewhat gracefully */ >>> #if defined(__UCLIBC__) && !defined(__ARCH_HAS_MMU__) >>> #define fork vfork >>> #endif >>> >>> This breaks musl that does not define __UCLIBC__. I posted an updated patch to >>> uses HAVE_FORK instead. >> >> We don't use musl on any noMMU platform today, SH2 fdpic is available in musl, so in theory you can start :) >> so this certainly cannot >> explain failures like: >> >> http://autobuild.buildroot.net/results/6cf/6cf75e08795d9ab194ce4e882c0f4858bad979c3/ >> >> (which was the first one mentioned in your commit log), since this >> failure happens with uClibc. >> >> Looking at http://autobuild.buildroot.net/?reason=ipkg-0.99.163, I see >> (looking only at the failures since the beginning of 2016) : >> >> * Numerous failures on ARM noMMU (uClibc) >> * Two failures on m68k noMMU (uClibc) >> * An old failure on i686 due to download issue >> >> i.e, none of the failures are caused by a musl-related build. > > You are right of course, forgive my sloppiness. To be correct for uClibc the > code should test for __ARCH_USE_MMU__ instead of __ARCH_HAS_MMU__. But testing > HAVE_FORK is better, I thing, since it also covers other hypothetical MMU-less > C libraries. ARCH_USE_MMU seems to be sufficient unless there is any other kind of memory management system that can redefine fork behavior. is fork the only difference when it comes to nommu, I guess not. So why create an extra check just for fork. > > I'll resend the patch with a correct description. > > baruch > > -- > http://baruch.siach.name/blog/ ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
diff --git a/package/ipkg/Config.in b/package/ipkg/Config.in index 4b2078d9cd70..d93e1d17b3dd 100644 --- a/package/ipkg/Config.in +++ b/package/ipkg/Config.in @@ -1,5 +1,6 @@ config BR2_PACKAGE_IPKG bool "ipkg" + depends on BR2_USE_MMU # fork() help The Itsy Package Installer from handhelds.org
Fixes: http://autobuild.buildroot.net/results/6cf/6cf75e08795d9ab194ce4e882c0f4858bad979c3/ http://autobuild.buildroot.net/results/964/964d8f694bc7d05b35411eabfbadf40bbf6337ae/ http://autobuild.buildroot.net/results/0f2/0f2cddf89af3ad4330556acd04ab6cb080370e24/ Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- package/ipkg/Config.in | 1 + 1 file changed, 1 insertion(+)