Nicolas Williams wrote:
> On Wed, Feb 27, 2008 at 09:25:07PM +0100, Roland Mainz wrote:
> > Nicolas Williams wrote:
> > -- snip --
> > >
> > > --- /dev/null Tue Feb 26 18:25:53 2008
> > > +++ new/usr/src/lib/sqlite3/install-sfw Tue Feb 26 18:25:53 2008
> > > @@ -0,0 +1,120 @@
> > > +#!/bin/ksh -p
> >
> > AFAIK it may be nice to use /usr/bin/ksh93 (or "bash" if it supports the
> > "errexit" flag) for this script and then do a $ set -o errexit # at the
> > beginning that the script - the "errexit" flag causes the shell to
> > immediately abort the script when it hits a command which returns a
> > failure instead of continuing operation (and maybe running amok (and
> > ordering 666 new A350 planes via your Ebay-account etc. =:-) ))
>
> Yes, I was thinking of this. I'll make it so. install.subr should do
> it too, but I'm leaving that for another CR. (I actually mention this
> in the install.subr CR.)
>
> > > +[[ $# -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
of the values for comparisation (e.g. ksh93 stores values in their
native formats, e.g. a float is stored as |long double| internally and
comparisations and/or calculations using (( expr )) will use these
variables directly instead of strings) and not strings. If you use [[ ]]
the following sequence will happen:
1. All arguments passed to [[ ]] are expanded, e.g. $# will be converted
from |long long| to a string (honoring LC_NUMERIC&co.)
2. The expanded list is stuffed in an argv array
3. The builtin command "test" is called with this argument array
4. The test builtin will parse arguments and do another expansion pass
5. The test builtin will figure out the logic (e.g. "-eq" which should
be used)
6. The test builtin will convert the string arguments to the matching
datatypes (float) as needed for "-eq"
7. The test builtin executes the "equal" operation and returns the
matching return code
Using (( )) will just do one single expansion pass and then runs the
builtin logic for comparing numbers (the matching codepath size ~~1/8
compared to the codepath for the whole convert madness needed for [[
]]).
As a side-note: If you use [[ ]] for floating-point numbers you'll
end-up with rounding errors caused by the base16--->base10 and
base10--->base16 conversion. There are two ways to avoid this kind of
problems:
1. Use ksh/ksh93 builtin math operators and _avoid_ the string
conversion, e.g. use $ (( foo=bar+baz )) instead of (( foo=$bar+$baz ))
2. Convert floating-point values to the C99 "hexfloat" format using $
printf "%a" foo # to make sure the values are in a lossless
(based16-based) string format
(yes, yes, I know... we have integers in this case but IMO it would be a
very good idea to handle numbers always using (( )) instead of [[ ]] to
avoid that the mess later crawls around in scripts elsewhere)
> > -- snip --
> > (( $# == 3 )) || {
> > -- snip --
> > , please (and see
> > http://opensolaris.org/os/project/shell/shellstyle/#use_posix_arithmetric_expressions)
>
> Yeah, I just have to get used to doing that, but still, this is a nit.
I know... but see above why there is a difference (and why I'm a bit
picky about it).
> > > +TOP=$(/bin/pwd)
> > > +SQLITE3SRCDIR=$1
> > > +SQLITE3DESTDIR=$2
> > > +BITS=$3
> >
> > If "BITS" contain integer numbers it may be usefull to use
>
> 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...
> > (BTW: If variables are not exported to the environment it may be nice to
> > spell them using lowercase to indicate that they are not exported (see
> > http://opensolaris.org/os/project/shell/shellstyle/#naming))
>
> That's true, but here this script is picking up where make leaves off,
> so I think style-wise it's clearer to use upper-case.
Ok...
> > > +MANSCRIPT=sunman-stability
> > > +
> > > +libsqlite3=$SQLITE3DESTDIR/usr/lib/libsqlite3.so.0.8.6
> > > +libtclsqlite3=$SQLITE3DESTDIR/usr/lib/tcl8.4/sqlite3/libtclsqlite3.so
> > > +sqlite3=$SQLITE3DESTDIR/usr/bin/sqlite3
> > > +sqlite3_h=$SQLITE3DESTDIR/usr/include/sqlite3.h
> > > +sqlite3ext_h=$SQLITE3DESTDIR/usr/include/sqlite3ext.h
> > > +sqlite3_pc=$SQLITE3DESTDIR/usr/lib/pkgconfig/sqlite3.pc
> > > +sqlite3_1=$SQLITE3SRCDIR/sqlite3.1
> > > +pkgIndex_tcl=$TOP/pkgIndex.tcl
> >
> > Quotes around the values may be nice, e.g. foo=$bar/baz --->
> > foo="$bar/baz"
>
> 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... :-) ).
> > > +
> > > +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).
> > > +then
> > > + install_all=:
> >
> > Please change this to...
> > -- 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|:" # :-) ))).
> > > +else
> > > + install_all=false
> > > +fi
> > > +
> > > +. ${SRC}/tools/install.subr
> >
> > Please use ...
> > -- snip --
> > source "${SRC}/tools/install.subr"
> > -- snip --
>
> Yes.
Thanks!
> > > +
> > > +# Check that the SONAME of an object is its filename.0. If the
> > > +# libsqlite SONAME ever changes then this will catch it / have to be
> > > +# modified.
> > > +check_elf_soname () {
> >
> > Eeeks, please use...
> > -- snip --
> > function check_elf_soname {
> > -- snip --
>
> OK.
>
> > > + [[ "$soname" = "$esoname" ]] && return 0
> > > + print -u2 "Error: ${1} has incorrect SONAME!"
> > > + print -u2 "\tactual: $soname"
> > > + print -u2 "\texpected: $esoname"
> >
> > 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...
> > 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
> > "printf", e.g.
> > -- snip --
> > printf "Error: %s has incorrect SONAME!" "${1}"
> > -- snip --
> > or "print"'s "-f" option if you use ksh93:
> > -- snip --
> > print -u2 -f "Error: %s has incorrect SONAME!" "${1}"
> > -- snip --
>
> No.
See above. In the case if unexpected charcters you may end-up with a
non-zero exit code for "print" and "printf" (see also:
http://www.opensolaris.org/os/community/arc/caselog/2008/094/onepager/
which declares the behavior for unrecognized escape sequences specifiers
(the character following a backslash ('\')) as "undefined" (short: The
new printf will bite you...)).
> > > +$install_all || exit 0
> >
> > Erm... what's the logic behind this ?
>
> Please elaborate on your question. I don't follow it.
My fault... I didn't realise it's te boolean value set above (see my
comment about "true" vs. ":" above).
> > AFAIK the rest of the script looks Ok (on demand I can send you a copy
> > of the script with the changes listed above) ...
>
> Thanks, but I'll make the changes I agreed to.
Ok...
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 7950090
(;O/ \/ \O;)
