[Pkg-xen-devel] [PATCH 03/12] debian/rules: Fix binary shuffling script for cross-building

[Elliott Mitchell]

On Thu, Sep 17, 2020 at 04:58:03PM +0100, Ian Jackson wrote:
> Elliott Mitchell writes ("[PATCH 03/12] debian/rules: Fix binary shuffling script for cross-building"):
> > `ldd` doesn't work with cross-builds, so use `file` to detect scripts
> > and `strings | grep` for identifying linked libraries.
> 
> How sad that ldd doesn't work.  I approve of the change to use file,
> although maybe that should have a change to build-depends.  (Right now
> it seems that debhelper depends on file so this is a theoretical bug
> rather than a FTBFS-on-buildd issue.)

As previously stated, I had checked.  On the flip side, yeah this does
leave a distinctly uneasy feeling, so see below.

> > Script almost conforms to POSIX shell standard, so adjust to conform and
> > switch to /bin/sh.  Simplify pipe structure, do more work in parent
> > shell.
> 
> I'm sorry to say that I don't agree with this.  What made you think
> there was anything undesirable about using bash in a build system ?

As previously stated, keeping an eye on these smaller places tends to
provide rather large gains when you move to the larger picture.

> >  set -e
> > -set -o pipefail
> 
> This, in particular, has quite adverse impacts on error handling.
> Also you have reformatted the script.  Reformatting things is usually
> to be discouraged.

This was a big deal before the changes I'm suggesting.  In particular it
had a *large* pipe structure with lots of places to fail.  Even better,
what failed was `ldd` which was early in the pipeline.

With the rework the pipeline is *much* simpler and the commands are
quite unlikely to fail.  `strings` and `grep` won't produce the failure
of `ldd`.  As stated before, a bunch of lines lost tabs, because they're
no longer part of that large pipe structure.

I got a few more bits of cleanup in this revised version.  $leaf may have
had some purpose in the past, but that is no longer useful.  Looks like
its arguments originated as copy&paste of the shuffle-boot-files script,
but shuffle-binaries no longer needs the second argument.


-----
d/shuffle-binaries: Fix binary shuffling script for cross-building

`ldd` doesn't work with cross-builds, so use `file` to detect scripts
and `strings | grep` for identifying linked libraries.  Even though
debhelper depends on file, make the requirement explicit.

Heavily simplify script inner loop.  While the core of that inner loop
was often executed only once, it is best to shrink inner loops.

With the reduced inner loop, less is being done inside pipes, so pipefail
is lower value (the early commands in pipes are unlikely to fail).  This
makes the script POSIX compliant so switch to /bin/sh.

Remove the useless extra argument passed to the script.

Signed-off-by: Elliott Mitchell <ehem+debian at m5p.com>
---
 debian/control          |  1 +
 debian/rules            |  2 +-
 debian/shuffle-binaries | 46 ++++++++++++++++++++++-------------------
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/debian/control b/debian/control
index 030214e7bd..2f47d20426 100644
--- a/debian/control
+++ b/debian/control
@@ -6,6 +6,7 @@ Section: admin
 Standards-Version: 4.5.0
 Build-Depends:
    debhelper (>= 10),
+   file,
    dh-exec,
    dpkg-dev (>= 1.16.0~),
    rdfind,
diff --git a/debian/rules b/debian/rules
index 62c87dfecb..eb4e1190de 100755
--- a/debian/rules
+++ b/debian/rules
@@ -268,7 +268,7 @@ xenstore_rm = $(addprefix debian/xen-utils-common/,		\
 		))
 
 override_dh_install:
-	debian/shuffle-binaries $(upstream_version) $(flavour)
+	debian/shuffle-binaries $(upstream_version)
 	:
 	debian/shuffle-boot-files $(upstream_version) $(flavour)
 	:
diff --git a/debian/shuffle-binaries b/debian/shuffle-binaries
index cff6de5428..7356531f1a 100755
--- a/debian/shuffle-binaries
+++ b/debian/shuffle-binaries
@@ -1,6 +1,5 @@
-#!/bin/bash
+#!/bin/sh
 set -e
-set -o pipefail
 
 version=$1; shift
 
@@ -17,34 +16,39 @@ version=$1; shift
 list=debian/libxenmiscV.install.vsn-in
 
 t=debian/tmp
-vd=/usr/lib/xen-$version/bin
+vd="/usr/lib/xen-$version/bin"
 cd=/usr/lib/xen-common/bin
 
-mkdir -p $t/$vd
+mkdir -p "$t/$vd"
 
-for binary in `find $t/usr/{bin,sbin} -type f`; do
-	reason=''
-	{ ldd "$binary" ||: ; } | { fgrep '=>' ||: ; } \
-	| (
-		while read lib dummy; do
+find "$t"/usr/*bin -type f | while read binary; do
+
+	# filter out scripts
+	file "$binary" | grep -q -eELF.\\+version.\\+interpreter || continue
+
+	# ldd doesn't work for cross-building
+	reason=$(
+		strings "$binary" | grep -e^lib.\\+\\.so\\.\[.0-9\]\\+\$ | \
+		while read lib; do
 			lib=${lib%.so.*}
-			if grep -F "usr/lib/*/$lib.so.*" $list >/dev/null; then
-				reason+=" $lib"
+			if grep -q -F "usr/lib/*/$lib.so.*" $list; then
+				printf " %s" "$lib"
 			fi
 		done
+	)
 
-		if [ "x$reason" = x ]; then
-			exit 0
-		fi
+	# if no reason, then skip
+	[ -n "$reason" ] || continue
 
-		echo "shuffling $binary $reason"
+	echo "shuffling $binary$reason"
 
-		leaf=${binary##*}
-		mv -v $binary $t/$vd/$leaf
-		ln -vs $cd/xen-utils-wrapper $binary
+	mv -v "$binary" "$t/$vd"
+	ln -vs "$cd/xen-utils-wrapper" "$binary"
 
-		touch debian/shuffle-binaries.stamp
-	)
+	touch "$0.stamp"
 done
 
-ls debian/shuffle-binaries.stamp
+if [ ! -e "$0.stamp" ]; then
+	echo "Failed to shuffle binaries!" 1>&2
+	exit 1
+fi
-----


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d-shuffle-binaries-Fix-binary-shuffling-script-for-c.patch
Type: text/x-diff
Size: 3410 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20200919/166d63ea/attachment.patch>