Hi,
| Looking at
| http://cr.opensolaris.org/~vrthra/squid/LOCAL-SQUID-GATE.patch ...
|
| 1. Why is "perl" needed here ?
| -- snip --
| + /usr/perl5/bin/perl -pi -e 's/^KERBLIBS =.*/KERBLIBS =-R
| \/usr\/lib\/gss -L\/usr\/lib\/gss -lgss \/usr\/lib\/gss\/mech_krb5.so
| -lkrb5 -lsocket /g' \
| + helpers/negotiate_auth/squid_kerb_auth/Makefile.*; \
| -- snip --
| AFAIK both "sed" and "ksh93" can do that job, too (e.g. smaller
| filters). For example this should work (untested):
| -- snip --
| for i in helpers/negotiate_auth/squid_kerb_auth/Makefile.* ; do
| c="$(< "$i")"
| print -r -- "${c//~(E)KERBLIBS =.*$'\n'/KERBLIBS =-R /usr/lib/gss
| -L/usr/lib/gss -lgss /usr/lib/gss/mech_krb5.so -lkrb5 -lsocket$'\n'}"
| >"$i"
| done
I would like to keep the perl as it is more simpler than the ksh
version, But if you can provide a simpler inplace replace library function
(like the _install that we use) with in the solaris build scripts, I will be
happy to use that instead.
| 2. Please think about using C99 by default, e.g. "-xc99=%all
| -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1". The advantage is that libc&co.
| Behave closer to the standard (and similar like glibc) which may be
| usefull (one precedent is the ksh93-integration work where these flags
| had a huge (positive) impact on performance and footprint) ... and as a
| small side-effect things like |system()|, |popen()| etc. use
| /usr/xpg4/bin/sh instead of /sbin/sh as system shell which matches the
| expectations of the GNU people for a "normal" system shell.
I think I will go with the squid default CFLAGS for now, and update it
later after testing it thoroughly.
| 3. Nit: Use "/usr/bin/ksh" (or "/usr/bin/ksh93"), not "/bin/ksh":
| -- snip --
| + MAKE=/usr/ccs/bin/make \
| + /bin/ksh ./configure \
| -- snip --
| BTW: What about using /usr/bin/ksh93 in this case ?
Will update the makefile to use ksh93 instead.
| 4. Script nits in "new/usr/src/cmd/squid/Solaris/http-squid":
| -- snip --
| > +. /lib/svc/share/smf_include.sh
| > +
| > +SQUID_HOME=/usr/squid
| > +CONF_FILE=/etc/squid/squid.conf
| > +PIDFILE=/var/squid/logs/squid.pid
| > +SQUID=${SQUID_HOME}/sbin/squid
| > +
| > +[ ! -f ${CONF_FILE} ] && exit $SMF_EXIT_ERR_CONFIG
|
| Quotes missing, e.g. please change this to
...
I would like to avoid these since the scripts are only used for build
the inputs are static and nothing is exposed to the user.
| 5. Script nits in "new/usr/src/cmd/squid/install-squid":
| General:
| - Would it be usefull to use $ set -o errexit # to stop the script when
| an error is hit, e.g. to avoid that the script continues after an error
| and then runs amok ?
will do this.
| > + cd tools
| > + ins_file cachemgr.conf ${CONFDIR} 644
| > + cd ..
|
| Alternatively you could use
| (
| cd tools
| ins_file cachemgr.conf ${CONFDIR} 644
| )
| e.g. at the end of the subshell the CWD gets restored to the original
| value.
Thanks I will update to use it.
| > +install_errtxt() {
| > + cd errors
| > + for i in `ls -p | grep '\/$'|sed 's/\/$//'`
|
| Somehow I wish you would use /usr/bin/ksh93 for this (e.g. neither "ls"
| or "sed" would be neccesary in this case) ... ;-(
As before, I think this is cleaner than going with the ksh syntax :) ,
so I will go with this now. but if you can define a library function to
extract the directory names and is made available from the build scripts
I will be happy to use that instead.
| > +# START HERE
| > +PKGVERS=`sed -ne '/VER=/s/.*=//p' Makefile.sfw`
| > +PREFIX=${ROOT}/usr/squid # Going with the apache style.
| > +BINDIR=${PREFIX}/bin
| > +SBINDIR=${PREFIX}/sbin
| > +LIBEXECDIR=${PREFIX}/libexec
| > +SHAREDIR=${PREFIX}/share
| > +CONFDIR=${ROOT}/etc/squid
| > +MAN8DIR=${PREFIX}/man/man8
|
| Erm... are these local variables which are not exported to the
| environment ? If "yes" then these variable names should be lowercase...
This seems to be the common convension followed by scripts of other
packages (eg: apache, mysql etc). so it
wont be correct if squid alone goes against it.
| AFAIK that's all what I could find in a 5min race through the code...
Thanks, I will post the newer webrevs after my nightly goes thorugh.
rahul
--
1. e4 _
----- End forwarded message -----
rahul
--
1. e4 _
