Nicolas Williams wrote:
> On Mon, Feb 25, 2008 at 02:14:54PM -0800, Mike Sullivan wrote:
> > Nicolas Williams wrote:
> >
> > >Hmmm, I don't quite buy it.  SFW components are a lot more discrete than
> > >ON components.  And many will install library and executable content
> > >both.
> >
> > Like I said, that can happen in ON too - it's just that it's easier to
> > make people split their code between lib and cmd in such cases, and it's
> > not easy or friendly to have folks in sfw split that code up. But
> > it still can be a useful split, or at least useful enough it's not worth
> > changing to me.
> 
> I guess it can happen in ON, but I think it's unlikely -- we've taken
> major pains to avoid this for, e.g., krb5, ssh, and other things in ON
> of external procedence.
> 
> But I've moved it to $SRC/lib/sqlite3, and I've generated a new webrev:
> 
> http://cr.opensolaris.org/~nico/webrev-sqlite3-sfw-2nd/

The usual 5min race throught
http://cr.opensolaris.org/~nico/webrev-sqlite3-sfw-2nd/sfwnv-sqlite3.patch
follows (patch code is quoted with "> "):
-- 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. =:-) ))

> +#
> +# CDDL HEADER START
> +#
> +# The contents of this file are subject to the terms of the
> +# Common Development and Distribution License (the "License").
> +# You may not use this file except in compliance with the License.
> +#
> +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
> +# or http://www.opensolaris.org/os/licensing.
> +# See the License for the specific language governing permissions
> +# and limitations under the License.
> +#
> +# When distributing Covered Code, include this CDDL HEADER in each
> +# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
> +# If applicable, add the following below this CDDL HEADER, with the
> +# fields enclosed by brackets "[]" replaced with your own identifying
> +# information: Portions Copyright [yyyy] [name of copyright owner]
> +#
> +# CDDL HEADER END
> +#
> +# Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
> +# Use is subject to license terms.
> +#
> +# ident        "@(#)install-sfw        1.17    08/02/26 SMI"
>
> +
> +[[ $# -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
-- snip --
(( $# == 3 )) || {
-- snip --
, please (and see
http://opensolaris.org/os/project/shell/shellstyle/#use_posix_arithmetric_expressions)
...

> +       print -u2 "Usage: $0 <sqlite3-build-dir> <sqlite3-destdir> 32|64"
> +       print -u2 "\twhere <sqlite3-destdir> is the DESTDIR where"
> +       print -u2 "\tSQLite3 was installed."
> +       exit 1
> +}
> +
> +TOP=$(/bin/pwd)
> +SQLITE3SRCDIR=$1
> +SQLITE3DESTDIR=$2
> +BITS=$3

If "BITS" contain integer numbers it may be usefull to use
-- snip --
integer BITS
-- snip --
(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))

> +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"

> +
> +if [[ "$BITS" = 32 ]]

Please use
-- snip --
if (( BITS == 32 )) ; then
-- snip --

> +then
> +       install_all=:

Please change this to...
-- snip --
install_all=":"
-- snip --

> +else
> +       install_all=false
> +fi
> +
> +. ${SRC}/tools/install.subr

Please use ...
-- snip --
source "${SRC}/tools/install.subr"
-- snip --
(see
http://opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot)
 
> +
> +# 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 --
(see
http://opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax
; there is a big WARNING why this is neccesary)

> +       typeset soname esoname
> +
> +       esoname=${1##*/}
> +       esoname=${esoname%.so*}.so.${2:-0}
> +       [[ -f "$1" ]] || exit 100
> +       elfedit -r -o simple -e "dyn:value SONAME" "$1" 2>/dev/null|read 
> soname
> +       if [[ -z "$soname" ]]
> +       then
> +               print -u2 "Error: $1 has no SONAME!"
> +               return 1
> +       fi
> +       [[ "$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 --

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
"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 --

> +       return 1
> +}
> +
> +fix_elf_runpath () {

Please use "function fix_elf_runpath" (see WARNING above) ...

> +       typeset runpath
> +
> +       [[ -f "$1" ]] || return 1
> +       elfedit -r -o simple -e "dyn:value RUNPATH" "$1" 2>/dev/null| \
> +               read runpath
> +       [[ "$runpath" = "${2:-/usr/lib}" ]] && return 0
> +       if elfedit -e "dyn:runpath ${2:-/usr/lib}" "$1" "${1}-fixed"
> +       then
> +               [[ -f "${1}-orig" ]] || mv "$1" "${1}-orig"
> +               mv "${1}-fixed" "$1"
> +               return 0
> +       fi
> +
> +       print -u2 "Error: Couldn't fix RUNPATH of ${1}!"
> +       return 1
> +}
> +
> +check_elf_soname $libsqlite3
> +fix_elf_runpath $libsqlite3
> +check_elf_soname $libtclsqlite3
> +fix_elf_runpath $libtclsqlite3
> +fix_elf_runpath $sqlite3
> +
> +[[ -h $ROOT/usr/lib/32 ]] || _install L . $ROOT/usr/lib/32
> +[[ -h $ROOT/usr/lib/64 ]] || _install L . $ROOT/usr/lib/$MACH64
> +_install D $libsqlite3 $ROOT/usr/lib/$BITS/libsqlite3.so.0 555
> +_install L libsqlite3.so.0 $ROOT/usr/lib/$BITS/libsqlite3.so
> +_install D $libtclsqlite3 
> $ROOT/usr/lib/tcl8.4/sqlite3/$BITS/libtclsqlite3.so 555
> +
> +$install_all || exit 0

Erm... what's the logic behind this ?

> +_install E $sqlite3 $ROOT/usr/bin/sqlite3 555
> +_install N $sqlite3_pc $ROOT/usr/lib/pkgconfig/sqlite3.pc 444
> +_install N $pkgIndex_tcl $ROOT/usr/lib/tcl8.4/sqlite3/pkgIndex.tcl 444
> +_install N $sqlite3_h $ROOT/usr/include/sqlite3.h 444
> +_install N $sqlite3ext_h $ROOT/usr/include/sqlite3ext.h 444
> +_install M $sqlite3_1 $ROOT/usr/share/man/man1/sqlite3.1 444
> +
> +exit 0
-- snip --

AFAIK the rest of the script looks Ok (on demand I can send you a copy
of the script with the changes listed above) ...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 7950090
 (;O/ \/ \O;)

Reply via email to