Hi, (forwarding it again as my previous mail does not seem to have reached.)
| 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 _ This message posted from opensolaris.org
