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.
> -- 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.
> > +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.
> (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.
> > +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.
> > +
> > +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?
> > +then
> > + install_all=:
>
> Please change this to...
> -- snip --
> install_all=":"
> -- snip --
No.
> > +else
> > + install_all=false
> > +fi
> > +
> > +. ${SRC}/tools/install.subr
>
> Please use ...
> -- snip --
> source "${SRC}/tools/install.subr"
> -- snip --
Yes.
> > +
> > +# 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.
> 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?
> "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.
> > +$install_all || exit 0
>
> Erm... what's the logic behind this ?
Please elaborate on your question. I don't follow it.
> 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.
Nico
--
