[Sugar-devel] [PATCH 1/2] Allow to build outside the source directory

[Sascha Silbe]

Excerpts from Marco Pesenti Gritti's message of 2011-12-07 18:01:05 +0100:
> 
> Signed-off-by: Marco Pesenti Gritti <marco at marcopg.org>

Thanks for the patch! It contains a few issues that prevent me for
pushing it as-is. It would have been easy to fix them up prior to
pushing, but I'd like to use this opportunity to point out some mistakes
that happen rather often. Shell scripts are quite tricky to get right.


[autogen.sh]
> @@ -1,4 +1,13 @@
>  #!/bin/sh
> +

> +test -n "$srcdir" || srcdir=`dirname "$0"`

You miss one layer of quoting. Special characters in the directory name
can cause you to do unexpected things, ranging from merely not working
up to destroying user data (unlikely, but possible).

Better:

test -n "${srcdir}" || srcdir="$(dirname "$0")"

Both $() and ${} are defined by POSIX [1,2]. $var works and is
equivalent to ${var} in simple cases like the ones we're talking about.
$(command) and `command` differ w.r.t. escaping and nesting. The
differences shouldn't matter in this particular case, but it's a good
habit to adopt. ${} and $() improve clarity and are significantly less
tricky to use in more complex cases.


> +test -n "$srcdir" || srcdir=.

Shouldn't we assign "$(pwd)" in this case?

> +
> +olddir=`pwd`

Again, we need to quote the result of the command:

olddir="$(pwd)"

And maybe builddir is a better name for olddir?


An updated patch would be appreciated, but I can post it as well. Just
tell me what you prefer.

Sascha

[1] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02
[2] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_03
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120109/74cd8e57/attachment.pgp>