Merge lp:~vorlon/linaro-image-tools/hwpack-always-from-PATH into lp:linaro-image-tools/11.11

[Steve Langasek]

Handling of the hwpack stage is wrong when the tools are on the path instead of being invoked as /path/to/linaro-media-create, because dirname returns '.' in this case. This patch fixes things up so that we can trust that the linaro-hwpack-install on the path is always the correct one.

[Peter Maydell (pmaydell)]

It seems a bit ugly to put the directory on the path just so we can say 'which linaro-hwpack-install' later. Wouldn't it be better to say at the start of the script
SCRIPTPATH="$(dirname "$(readlink -f "$0")")"

and then
LINARO_HWPACK_INSTALL="$SCRIPTPATH/linaro-hwpack-install"

?

(This should work whether linaro-media-create is on the PATH or not.)

PS: I think that "when the tools are on the path [...] because dirname returns '.' in this case" is only true when "." is on your PATH and that is where the tools were found. The use of readlink is what handles this case in my proposal. (This avoids creating an implicit requirement not to change current working directory between starting the script and setting LINARO_HWPACK_INSTALL, incidentally.)

If you do prefer your approach then this line needs quotes:
LINARO_HWPACK_INSTALL=$(which linaro-hwpack-install)

[Steve Langasek (vorlon)]

Thanks for the feedback. Using $PATH for this is partly motivated by the rsync-root-fs patch, which also involves calling an external script that should be installed in the same directory as linaro-media-create; the simplest way to address both of these cases cleanly is by using $PATH, IMHO. This just isn't obvious because the rsync-root-fs patch has now been backed out pending revision.

I've just spotted a buglet in this patch, though; if linaro-media-create is installed on the $PATH, but the user is directly calling the bzr version instead, this code will look for linaro-hwpack-install in the system path instead of in the bzr directory. I'm pushing a correction for this now.

In light of the other future uses of $PATH, do you think this is reasonable, or do you think we should still use a separate $SCRIPTPATH variable?

[lp:~vorlon/linaro-image-tools/hwpack-always-from-PATH]

140. By Steve Langasek on 2010-10-10

fix a bug in how we look up linaro-hwpack-install when l-m-c is both on the
path and in a bzr checkout

[Peter Maydell (pmaydell)]

Right, I guess that putting it on the PATH makes sense. In that case why not just say

PATH="$(dirname "$(readlink -f "$0")"):$PATH"

?

The code you have at the moment will be very fragile if the script is invoked via a relative path.

8 +if [ "$(dirname "$0")" != . ] || [ "$0" = "./$(basename "$0")" ]; then

What is the case where this if clause is not true? I can't find one...

[Steve Langasek (vorlon)]

On Mon, Oct 11, 2010 at 07:15:54AM -0000, Peter Maydell wrote:
> Right, I guess that putting it on the PATH makes sense. In that case why not just say

> PATH="$(dirname "$(readlink -f "$0")"):$PATH"

Because if $0 is invoked as 'linaro-media-create' because it's already on
the path, we don't want to put '.' at the front of the path; that's
insecure.

> The code you have at the moment will be very fragile if the script is
> invoked via a relative path.

Howso? I don't think any of these scripts ever call 'cd', so a relative
path is no worse than an absolute one for our purposes; the only benefit to
your formulation is that if someone has symlinked ~/bin/linaro-media-create
to their bzr checkout, this will look in the bzr working tree instead of
~/bin for the helper scripts. That's probably a worthwhile feature, but
the trivial implementation doesn't work correctly if linaro-media-create is
already on the path, so I would rather defer that for a separate fix instead
of blocking this fix for a proper implementation of that feature.

> 8 +if [ "$(dirname "$0")" != . ] || [ "$0" = "./$(basename "$0")" ]; then

> What is the case where this if clause is not true? I can't find one...

See above :)

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[lp:~vorlon/linaro-image-tools/hwpack-always-from-PATH]

141. By Steve Langasek on 2010-10-12

drop the convoluted dirname handling, sh always gives us the full path in $0
so dirname "$0" is always what we want on the path - thanks to Peter Maydell
for noticing!

[Peter Maydell (pmaydell)]

You still need the quotes setting LINARO_HWPACK_INSTALL:
LINARO_HWPACK_INSTALL="$(which linaro-hwpack-install)"

(For posterity: the case where [ "$(dirname "$0")" != . ] || [ "$0" = "./$(basename "$0")" ] is true is when $0 is a bareword. However the only way to get that that I'm aware of is if you type "sh scriptname".)

[Steve Langasek (vorlon)]

On Tue, Oct 12, 2010 at 03:17:39PM -0000, Peter Maydell wrote:
> You still need the quotes setting LINARO_HWPACK_INSTALL:
> LINARO_HWPACK_INSTALL="$(which linaro-hwpack-install)"

Nope:

$ LINARO_HWPACK_INSTALL=$(echo foo bar)
$ echo $LINARO_HWPACK_INSTALL
foo bar
$

:-)

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Peter Maydell (pmaydell)]

OK, that's one-all I think :-) I'm happy with this patch.

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-10-11 15:09:55 +0000
+++ linaro-media-create	2010-10-12 15:02:43 +0000
@@ -37,6 +37,8 @@
 BOOT_DISK="${DIR}/boot-disc"
 ROOT_DISK="${DIR}/root-disc"
 
+PATH="$(dirname "$(readlink -f "$0")"):$PATH"
+
 ensure_command() {
   # ensure_command foo foo-package
   which "$1" 2>/dev/null 1>/dev/null || ( echo "Installing required command $1 from package $2" && sudo apt-get install "$2" )
@@ -287,9 +289,7 @@
   # rootfs.
   trap "sudo umount ${chroot}/proc || true" EXIT
 
-  # XXX: Assume linaro-hwpack-install lives on the same directory as this
-  # script. This is far from optimal but should do for now.
-  LINARO_HWPACK_INSTALL=$(dirname $0)/linaro-hwpack-install
+  LINARO_HWPACK_INSTALL=$(which linaro-hwpack-install)
 
   sudo mv -f ${chroot}/etc/resolv.conf ${TMP_DIR}/resolv.conf.orig
   sudo cp /etc/resolv.conf ${chroot}/etc/resolv.conf