On Thu, Feb 28, 2008 at 12:10:30AM +0100, Roland Mainz wrote:
> Nicolas Williams wrote:
> > > > +[[ $# -eq 3 ]] || {
> > >
> > > Ouch... both /usr/bin/ksh and /usr/bin/ksh93 support builtin math and
> > > comparing strings using the "test" builtin is a bit sub-optimal, please
> > > use
> >
> > Ouch what? [[ ... ]] uses the built-in test, and surely costs nothing
> > measurable in this case.
>
> Erm... (( )) is a math expression which uses the internal representation
[long explanation elided]
I understand, but I this makes not an iota of difference here.
> > It does.
> >
> > > -- snip --
> > > integer BITS
> > > -- snip --
> >
> > No arithmetic is being done with $BITS, it's just substituted in.
>
> Erm... yes... but if someone tries to pass a non-numberic string to the
> "BITS" variable it will result in an error and the "set -o errexit" then
> "catches" this error...
The errors will happen anyways when this:
_install D $libsqlite3 $ROOT/usr/lib/$BITS/libsqlite3.so.0 555
fails.
> > Not needed in variable assignments where there is only one assignment in
> > the line, even if any of those substitutions expand to strings
> > containing whitespace.
>
> Erm... the problem is that it's not portable (you could avoid any
> problems in this area if you put a IFS='' at the beginning of the script
> (to avoid any field splitting)) across the various POSIX-like shells
> (erm... Ok... you use an explicit #! at the beginning... ignore my
> bickering... :-) ).
Not portable to what? KSH works like this, and this is a KSH script.
No need to make this script portable to other shells.
You'll need a much stronger argument for me to accept this comment :)
> > > > +
> > > > +if [[ "$BITS" = 32 ]]
> > >
> > > Please use
> > > -- snip --
> > > if (( BITS == 32 )) ; then
> > > -- snip --
> >
> > It's only compared and then substituted into paths, so what's the point?
>
> See above (both the issue about (( )) itself and the idea to let the
> "integer" datatype catch any string garbage).
Here there's not even any type conversions involved (= vs. -eq).
> > > -- snip --
> > > install_all=":"
> > > -- snip --
> >
> > No.
>
> Ok...
> ... BTW: While reading the script a 2nd time I realise that ":" use used
> as boolean value below... using "true" as value may be more readable
> (and it's guranteed to be a ksh93 builtin command (using exactly the
> same codepath as the ":" bulitin command (and take a look at the output
> of $ builtin | egrep "true|:" # :-) ))).
Is it a KSH88 built-in? If not, then forget it :)
> > > 1. Erm... alternative way may be "bundle" the redirection, e.g.
> > > -- snip --
> > > # redirect output of "print" below to stderr
> > > {
> > > print "Error: ${1} has incorrect SONAME!"
> > > print "\tactual: $soname"
> > > print "\texpected: $esoname"
> > > } >&2
> > > -- snip --
> >
> > This is a simple install script. I don't see the point.
>
> It was just an idea... the block redirection is a bit more efficient and
> avoids lots of extra |sfflush()| calls...
Don't get me wrong, I do that very often in my scripts (I can send you
some if you like). Particularly when I'm redirecting output to a pipe
or when I'm appending to a file (less risk of missing a '>' and
clobbering something important). I've just never felt compelled to do
it for stderr output.
> > > 2. The 'print -u2 "Error: ${1} has incorrect SONAME!"' is a bit tricky
> > > if the filename contains unexpected stuff. AFAIK it's better to use
> >
> > What?! What tricky stuff can $1 contain that would mess that up?
>
> The "print" builtin interprets the string you pass via $1 as a format,
> e.g. anything which follows a '\'-character will be intepreted as a
> special escape sequence (e.g. \E or \u[20ac]) etc.
> Ok... it's unlikely that an Unix filename contains '\' but for
> portabilty it may be safer to use the $ printf "foo %s" "bar" # thing
Ah! OK, yes, this is true. Usually I use print -r when I want to avoid
this. No, it didn't occur to me do that here. I'll add -r to these
print command-lines.
Nico
--
