| Message ID | 56063c394c473bf4292658511bae0dd99583be07.1479356115.git.sam.bobroff@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
Am Thu, 17 Nov 2016 15:15:23 +1100 schrieb Sam Bobroff: > Because affected configure files can be discovered automatically, this > patch introduces a hook (enabled only when building for powerpc64 and > powerpc64le) that uses a script to scan and fix each package. Hi, good work! The minidlna package is fixed by your patch, but the libsidplay2 package needs some fixing: >>> libsidplay2 2.1.1 Checking configure (powerpc64/powerpc64le) support/scripts/fix-configure-powerpc64.sh /home/bernd/buildroot/br2/ output/build/libsidplay2-2.1.1 patching file /home/bernd/buildroot/br2/output/build/libsidplay2-2.1.1/./ libsidplay/configure Hunk #1 succeeded at 4176 with fuzz 1 (offset 2874 lines). patching file /home/bernd/buildroot/br2/output/build/libsidplay2-2.1.1/./ resid/configure Hunk #1 succeeded at 4978 with fuzz 1 (offset 3676 lines). patching file /home/bernd/buildroot/br2/output/build/libsidplay2-2.1.1/./ builders/resid-builder/configure Hunk #1 succeeded at 4187 with fuzz 1 (offset 2885 lines). patching file /home/bernd/buildroot/br2/output/build/libsidplay2-2.1.1/./ builders/hardsid-builder/configure Hunk #1 succeeded at 4181 with fuzz 1 (offset 2879 lines). patching file /home/bernd/buildroot/br2/output/build/libsidplay2-2.1.1/./ libsidutils/configure Hunk #1 succeeded at 4180 with fuzz 1 (offset 2878 lines). [...] checking whether the /home/bernd/buildroot/br2/output/host/usr/bin/ powerpc64le-linux-gcc linker (/home/bernd/buildroot/br2/output/host/opt/ ext-toolchain/powerpc64le-buildroot-linux-gnu/bin/ld -m elf64ppc) supports shared libraries... no checking dynamic linker characteristics... GNU/Linux ld.so checking how to hardcode library paths into programs... immediate checking whether stripping libraries is possible... yes checking if libtool supports shared libraries... no checking whether to build shared libraries... no checking whether to build static libraries... yes which leads to /home/bernd/buildroot/br2/output/host/opt/ext-toolchain/bin/../lib/gcc/ powerpc64le-buildroot-linux-gnu/5.4.0/../../../../powerpc64le-buildroot- linux-gnu/bin/ld: /home/bernd/buildroot/br2/output/build/ libsidplay2-2.1.1/libsidplay/src/.libs/libsidplay2.a(player.o): In function `.LTHUNK4': player.cpp:(.text._ZN12__sidplay2__6Player12interruptIRQEb [_ZN12__sidplay2__6Player12interruptIRQEb]+0x58): call to `__sidplay2__::Player::fakeIRQ()' lacks nop, can't restore toc; recompile with -fPIC /home/bernd/buildroot/br2/output/host/opt/ext-toolchain/bin/../lib/gcc/ powerpc64le-buildroot-linux-gnu/5.4.0/../../../../powerpc64le-buildroot- linux-gnu/bin/ld: final link failed: Bad value because configure is re-created during the build phase. The quote from configure comes from the second run of it. I used this defconfig: http://autobuild.buildroot.net/ results/5d6/5d60bccbe024db240630845e15a3f15e8ddf0f01/defconfig Regards, Bernd
Hello, On Thu, 17 Nov 2016 07:43:35 +0100, Bernd Kuhls wrote: > good work! The minidlna package is fixed by your patch, but the How can minidlna be fixed by Sam's patch? I don't see the offending hunk in minidlna's configure script. Am I missing something: $ grep powerpc minidlna-1.1.5/configure $ > because configure is re-created during the build phase. The quote from > configure comes from the second run of it. It's not good to have configure re-created during the build phase. We shouldn't find out why, and fix it. Thomas
Hello, On Thu, 17 Nov 2016 15:15:23 +1100, Sam Bobroff wrote: > Many (100+) packages supported by buildroot contain old configure > scripts (or build them from old versions of autotools) that are unable > to determine how to link shared libraries on powerpc64 and > powerpc64le. This causes that test to erroneously fail on toolchains > that are not "bi-endian" (which is the case for toolchains built by > buildroot), which causes configure to build static libraries instead > of dynamic ones. Although these builds succeed, they tend to cause > linker failures in binaries later linked against them. > > Because affected configure files can be discovered automatically, this > patch introduces a hook (enabled only when building for powerpc64 and > powerpc64le) that uses a script to scan and fix each package. > > This applies only to packages built for the target. > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> I think I like this approach. Yes, it looks a bit hackish, but we already patch libtool files in a fairly similar way, and this is actually fixing the real problem. > +set -e > + > +if [ $# -ne 1 ]; then > + echo "Usage: $0 <package build directory>" > + exit 2 > +fi > + > +pkg="$1" It's not really pkg (which we normally use for the package name), but the package source directory. Maybe just: srcdir="$1" > +files=$(cd "$pkg" && find . -name configure -exec grep -qF 'Generated by GNU Autoconf' {} \; -exec grep -qF 'ppc*-*linux*|powerpc*-*linux*)' {} \; -print) This line is a bit too long, can you split it? Also, why do you cd into the source directory, you can just as well do find ${srcdir}, right? > +suffix=.powerpc64.orig Do we really want to keep the original files? > +*** a/configure 2016-11-07 14:04:47.444117880 +1100 > +--- b/configure 2016-11-07 14:05:03.652181547 +1100 > +*************** > +*** 1302,1308 **** > + x86_64-*linux*) > + LD="${LD-ld} -m elf_x86_64" > + ;; > +! ppc*-*linux*|powerpc*-*linux*) > + LD="${LD-ld} -m elf64ppc" > + ;; > + s390*-*linux*|s390*-*tpf*) > +--- 1302,1311 ---- > + x86_64-*linux*) > + LD="${LD-ld} -m elf_x86_64" > + ;; > +! powerpcle-*linux*) > +! LD="${LD-ld} -m elf64lppc" > +! ;; > +! powerpc-*linux*) > + LD="${LD-ld} -m elf64ppc" > + ;; > + s390*-*linux*|s390*-*tpf*) > +EOF Is there any reason to use this diff format? You seem to be using it all the time. Please use unified diffs instead (diff -u). I'm hesitating between this patch approach (with significant offsets) when applying, and an approach using a awk script. Here is an awk script that does the job: /^\s*ppc\*-\*linux\*|powerpc\*-\*linux\*)$/ { infix = 1; print " powerpcle-*linux*)"; print " LD=\"${LD-ld} -m elf64lppc\""; print " ;;"; print " powerpc-*linux*)"; print " LD=\"${LD-ld} -m elf64ppc\""; print " ;;"; } /^\s*;;$/ { if (infix) { infix = 0; next; } } // { if (infix) next; print; } I'm not sure which of the two solutions is the most appropriate. The awk solution can probably be applied without doing the grep for the ppc*-*linux line, because the awk script only does the replacement if this line is found anyway. Thomas
On 17-11-16 11:05, Thomas Petazzoni wrote: > Hello, > [snip] > I'm hesitating between this patch approach (with significant offsets) > when applying, and an approach using a awk script. Here is an awk > script that does the job: > > /^\s*ppc\*-\*linux\*|powerpc\*-\*linux\*)$/ { How sure are you that no configure script will contain this line somewhere else than in the shared library test? You *have* to match the context as well - in particular the LD="${LD-ld} -m elf64ppc" part. This is possible with awk, but the patch approach is a lot simpler IMHO, and anyway easier to read. Regards, Arnout > infix = 1; > print " powerpcle-*linux*)"; > print " LD=\"${LD-ld} -m elf64lppc\""; > print " ;;"; > print " powerpc-*linux*)"; > print " LD=\"${LD-ld} -m elf64ppc\""; > print " ;;"; > } > > /^\s*;;$/ { > if (infix) { > infix = 0; > next; > } > } > > // { > if (infix) > next; > print; > } > > I'm not sure which of the two solutions is the most appropriate. The > awk solution can probably be applied without doing the grep for the > ppc*-*linux line, because the awk script only does the replacement if > this line is found anyway. > > Thomas >
Hello, On Thu, 17 Nov 2016 12:06:42 +0100, Arnout Vandecappelle wrote: > > /^\s*ppc\*-\*linux\*|powerpc\*-\*linux\*)$/ { > > How sure are you that no configure script will contain this line somewhere else > than in the shared library test? You *have* to match the context as well - in > particular the LD="${LD-ld} -m elf64ppc" part. This is possible with awk, but > the patch approach is a lot simpler IMHO, and anyway easier to read. Fine with me. I was also unsure the awk idea was better. Thomas
On Thu, Nov 17, 2016 at 09:55:45AM +0100, Thomas Petazzoni wrote: > Hello, > > On Thu, 17 Nov 2016 07:43:35 +0100, Bernd Kuhls wrote: > > > good work! The minidlna package is fixed by your patch, but the > > How can minidlna be fixed by Sam's patch? I don't see the offending > hunk in minidlna's configure script. Am I missing something: > > $ grep powerpc minidlna-1.1.5/configure Yes ;-) Almost all the breakages due to this problem are of this type: old configure scripts cause libraries to be built statically, but successfully, and then packages that use them fail to link due to undefined symbols. In this case it's probally libid3tag or libexif, both of which are affected by the patch. > > because configure is re-created during the build phase. The quote from > > configure comes from the second run of it. > > It's not good to have configure re-created during the build phase. We > shouldn't find out why, and fix it. > > Thomas Some packages seem to do it based on dependency rules: I found that if I patched the libtool input files, "make" would trigger a re-autoconfigure but I haven't really investigated it. Cheers, Sam.
On Thu, Nov 17, 2016 at 11:05:29AM +0100, Thomas Petazzoni wrote: > Hello, > > On Thu, 17 Nov 2016 15:15:23 +1100, Sam Bobroff wrote: > > Many (100+) packages supported by buildroot contain old configure > > scripts (or build them from old versions of autotools) that are unable > > to determine how to link shared libraries on powerpc64 and > > powerpc64le. This causes that test to erroneously fail on toolchains > > that are not "bi-endian" (which is the case for toolchains built by > > buildroot), which causes configure to build static libraries instead > > of dynamic ones. Although these builds succeed, they tend to cause > > linker failures in binaries later linked against them. > > > > Because affected configure files can be discovered automatically, this > > patch introduces a hook (enabled only when building for powerpc64 and > > powerpc64le) that uses a script to scan and fix each package. > > > > This applies only to packages built for the target. > > > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> > > I think I like this approach. Yes, it looks a bit hackish, but we > already patch libtool files in a fairly similar way, and this is > actually fixing the real problem. > > > +set -e > > + > > +if [ $# -ne 1 ]; then > > + echo "Usage: $0 <package build directory>" > > + exit 2 > > +fi > > + > > +pkg="$1" > > It's not really pkg (which we normally use for the package name), but > the package source directory. Maybe just: > > srcdir="$1" Good idea, will do. > > +files=$(cd "$pkg" && find . -name configure -exec grep -qF 'Generated by GNU Autoconf' {} \; -exec grep -qF 'ppc*-*linux*|powerpc*-*linux*)' {} \; -print) > > This line is a bit too long, can you split it? Also, why do you cd into > the source directory, you can just as well do find ${srcdir}, right? I'll split it. Doing the cd first means that the output in the build log is more readable, since it's then relative to the srcdir (without it you get the full absolute path). Which do you prefer? > > > +suffix=.powerpc64.orig > > Do we really want to keep the original files? Well, no, but now that you mention it, what about this idea: Use the patch backup file as a way to see if the patch has been applied already and if it has been, skip patching. At the moment, if you interrupt the configure process (which is quite long) and then re-run the build, it will fail because the patching fails. > > +*** a/configure 2016-11-07 14:04:47.444117880 +1100 > > +--- b/configure 2016-11-07 14:05:03.652181547 +1100 > > +*************** > > +*** 1302,1308 **** > > + x86_64-*linux*) > > + LD="${LD-ld} -m elf_x86_64" > > + ;; > > +! ppc*-*linux*|powerpc*-*linux*) > > + LD="${LD-ld} -m elf64ppc" > > + ;; > > + s390*-*linux*|s390*-*tpf*) > > +--- 1302,1311 ---- > > + x86_64-*linux*) > > + LD="${LD-ld} -m elf_x86_64" > > + ;; > > +! powerpcle-*linux*) > > +! LD="${LD-ld} -m elf64lppc" > > +! ;; > > +! powerpc-*linux*) > > + LD="${LD-ld} -m elf64ppc" > > + ;; > > + s390*-*linux*|s390*-*tpf*) > > +EOF > > Is there any reason to use this diff format? You seem to be using it > all the time. Please use unified diffs instead (diff -u). Oops, will fix! > I'm hesitating between this patch approach (with significant offsets) > when applying, and an approach using a awk script. Here is an awk > script that does the job: > > /^\s*ppc\*-\*linux\*|powerpc\*-\*linux\*)$/ { > infix = 1; > print " powerpcle-*linux*)"; > print " LD=\"${LD-ld} -m elf64lppc\""; > print " ;;"; > print " powerpc-*linux*)"; > print " LD=\"${LD-ld} -m elf64ppc\""; > print " ;;"; > } > > /^\s*;;$/ { > if (infix) { > infix = 0; > next; > } > } > > // { > if (infix) > next; > print; > } > > I'm not sure which of the two solutions is the most appropriate. The > awk solution can probably be applied without doing the grep for the > ppc*-*linux line, because the awk script only does the replacement if > this line is found anyway. > > Thomas From the other thread, it seems like you're happy to keep using patch. I'll post a v2. Cheers, Sam.
[snip] > > > +suffix=.powerpc64.orig > > > > Do we really want to keep the original files? > > Well, no, but now that you mention it, what about this idea: > > Use the patch backup file as a way to see if the patch has been applied > already and if it has been, skip patching. > > At the moment, if you interrupt the configure process (which is quite > long) and then re-run the build, it will fail because the patching > fails. ^--- I've realised that this is wrong, and I should just remove the backup file. If configure is already patched, it isn't detected as needing a patch so this problem doesn't happen! Cheers, Sam.
diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk index d1cdb89..9e6a061 100644 --- a/package/pkg-autotools.mk +++ b/package/pkg-autotools.mk @@ -79,6 +79,15 @@ define LIBTOOL_PATCH_HOOK endef # +# Hook to patch common issue with configure on powerpc64{,le} failing +# to detect shared library support: +# +define CONFIGURE_FIX_POWERPC64_HOOK + @$(call MESSAGE,"Checking configure (powerpc64/powerpc64le)") + support/scripts/fix-configure-powerpc64.sh $($(PKG)_DIR) +endef + +# # Hook to gettextize the package if needed # define GETTEXTIZE_HOOK @@ -255,6 +264,14 @@ endif endif +# Must be added after other pre-configure hooks that might regenerate the +# configure script and overwrite the changes made here. +ifeq ($(4),target) +ifeq ($(BR2_powerpc64)$(BR2_powerpc64le),y) +$(2)_PRE_CONFIGURE_HOOKS += CONFIGURE_FIX_POWERPC64_HOOK +endif +endif + # # Build step. Only define it if not already defined by the package .mk # file. diff --git a/support/scripts/fix-configure-powerpc64.sh b/support/scripts/fix-configure-powerpc64.sh new file mode 100755 index 0000000..e4d676e --- /dev/null +++ b/support/scripts/fix-configure-powerpc64.sh @@ -0,0 +1,53 @@ +#!/bin/bash + +# This is a script to find, and correct, a problem with old versions of +# configure that affect powerpc64 and powerpc64le. + +# The issue causes configure to incorrectly determine that shared library +# support is not present in the linker. This causes the package to build a +# static library rather than a dynamic one and although the build will succeed, +# it may cause packages that link with the static library it to fail due to +# undefined symbols. + +# This script searches for files named 'configure' that appear to have this +# issue (by searching for a known bad pattern) and patching them. + +set -e + +if [ $# -ne 1 ]; then + echo "Usage: $0 <package build directory>" + exit 2 +fi + +pkg="$1" +files=$(cd "$pkg" && find . -name configure -exec grep -qF 'Generated by GNU Autoconf' {} \; -exec grep -qF 'ppc*-*linux*|powerpc*-*linux*)' {} \; -print) +suffix=.powerpc64.orig + +# --ignore-whitespace is needed because some packages have included +# copies of configure scripts where tabs have been replaced with spaces. +for c in $files; do + patch --ignore-whitespace --suffix=$suffix "$pkg"/"$c" <<'EOF' +*** a/configure 2016-11-07 14:04:47.444117880 +1100 +--- b/configure 2016-11-07 14:05:03.652181547 +1100 +*************** +*** 1302,1308 **** + x86_64-*linux*) + LD="${LD-ld} -m elf_x86_64" + ;; +! ppc*-*linux*|powerpc*-*linux*) + LD="${LD-ld} -m elf64ppc" + ;; + s390*-*linux*|s390*-*tpf*) +--- 1302,1311 ---- + x86_64-*linux*) + LD="${LD-ld} -m elf_x86_64" + ;; +! powerpcle-*linux*) +! LD="${LD-ld} -m elf64lppc" +! ;; +! powerpc-*linux*) + LD="${LD-ld} -m elf64ppc" + ;; + s390*-*linux*|s390*-*tpf*) +EOF +done
Many (100+) packages supported by buildroot contain old configure scripts (or build them from old versions of autotools) that are unable to determine how to link shared libraries on powerpc64 and powerpc64le. This causes that test to erroneously fail on toolchains that are not "bi-endian" (which is the case for toolchains built by buildroot), which causes configure to build static libraries instead of dynamic ones. Although these builds succeed, they tend to cause linker failures in binaries later linked against them. Because affected configure files can be discovered automatically, this patch introduces a hook (enabled only when building for powerpc64 and powerpc64le) that uses a script to scan and fix each package. This applies only to packages built for the target. Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> --- Hi Buildroot, See the patch commit message for a description of the issue I'm trying to fix. This is an RFC and here are some things to consider: I took the approach of automatically patching packages. This presumably slows the build down a little bit, but only on the affacted architectures (the code is not enabled otherwise) and has the advantage of automatically picking up new packages as they are added. I've added a minimum of code to pkg-autotools.mk to add a PRE_CONFIGURE hook to the end of the existing hooks. It's important that it be after the AUTO_RECONFIGURE handling, which may regenerate configure. The actual patching work is done in a support script. The script is fairly simple: affected configure scripts all contain a unique pattern, so files are selected based on that, and "patch" is used to correct them. The location of the patch moves around a lot but there are lots of constant lines of context around them so patch always succeeds. The input files used to generate configure are not patched, because doing so triggers some packages to regenerate configure, and that step fails for some packages, breaking their build. It does not handle host packages. I'm not sure how to detect the host arch correctly, and although libraries may be being built incorrectly there at the moment, it's not causing any link failures so it seems unnecessary to fix them at the moment. Does this seem a reasonable solution? Would it be better to generalize it in some way? Could we solve some other problems at the same time? Should the code go somewhere else? Would a flag per package be better than checking every package? (It's not hard to do.) Cheers, Sam. package/pkg-autotools.mk | 17 ++++++++++ support/scripts/fix-configure-powerpc64.sh | 53 ++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100755 support/scripts/fix-configure-powerpc64.sh