Raymond Xiong wrote: > > Can anyone help to review my code for Erlang/OTP integration? > > http://cr.opensolaris.org/~rayx/erlang/webrev/
5min race through http://cr.opensolaris.org/~rayx/erlang/webrev/erlang.patch (patch code is quoted with "> "): General issue: SPARC always suppirt 64bit - why aren't you delivering a 64bit binary on SPARC ? > +VER = $(COMPONENT_NAME:sh)_src_$(COMPONENT_VERSION:sh) > +TARBALL = $(VER).tar.gz >+PATCHES:sh = echo Patches/*.patch 1. Please think about adding "-xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1" to CFLAGS. 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. 2. Please add "-xstrconst" to CFLAGS to put all string literals into read-only memory (which can be shared between processes) If you have any problems with these flags send me a PM, I can make patches for you... > --- /dev/null Fri Apr 25 07:19:01 2008 > +++ new/usr/src/cmd/erlang/postinstall-sfw Fri Apr 25 07:19:01 2008 [snip] AFAIK it may be nice to use /usr/bin/ksh93 or /usr/bni/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 15 new A380F planes via your Ebay-account etc. =:-) )) Please add "set -o errexit" at the top of the script to make sure it exists when a command returns a non-zero exit code (=failure). Note: The following issues are mainly based on http://www.opensolaris.org/os/project/shell/shellstyle/ ... > +. ${SRC}/tools/install.subr Please use "source" instead of "." > +VERS=R12B-1 Please make this a read-only string literal (e.g. $ typeset -r VERS="R12B-1") > +# We use INSTALL_PREFIX to install Erlang under proto root > +# for packaging. Erlang generates some files in installation > +# phase, and they may contain a ROOTDIR string, which is > +# prefixed with INSTALL_PREFIX(it is proto root in this case). > +# The code below fix ROOTDIR string by removing the prefix. > + > +(cd $ROOT; Please add double-quotes around "$ROOT" ... > + /usr/perl5/bin/perl -pi -e "s|$ROOT||g" \ > + usr/bin/erl \ > + usr/lib/erlang/bin/erl \ > + usr/lib/erlang/bin/start \ > + usr/lib/erlang/erts-5.6.1/bin/erl \ > + usr/lib/erlang/erts-5.6.1/bin/start \ > + usr/lib/erlang/releases/RELEASES > +) > + > +# Uncompress and install manpages > + > +MAN_SRC=otp_doc_man_${VERS} > +MAN_INSTALLDIR=${ROOT}/usr/share/man > +MANSCRIPT=../../../sunman-stability Are these constant strings ? If "yes" ---> "typeset -r" ... > + > +mkdir ${MAN_SRC} Please add double-quotes around "${MAN_SRC}" ... > +(cd ${MAN_SRC}; > + /usr/bin/gzip -dc ../${MAN_SRC}.tar.gz | /usr/bin/tar xpf - > + cd man; > + for subdir in `ls`; do Please use $( cmd ) , not ` cmd ` > + (cd $subdir; Double-quotes... > + case $subdir in Double-quotes... > + man1) > + # The following files are not installed: > + # - start_erl.1, werl.1, erlsrv.1 (for Windows only) > + # - start.1 (init.d script for embedded system) > + # - start_webtool (a one-line script to start webtool > app) > + suffix=1 > + files="epmd.1 erl.1 erlc.1 escript.1 erl_call.1 run_erl.1" > + dstdir=$MAN_INSTALLDIR/man1 Double-quotes... > + ;; > + man3) > + suffix=3erl > + files=`ls` Please use $( cmd ) , not ` cmd ` > + dstdir=$MAN_INSTALLDIR/man3erl Double-quotes... > + ;; > + man4) > + suffix=4erl > + files=`ls` Please use $( cmd ) , not ` cmd ` > + dstdir=$MAN_INSTALLDIR/man4erl Double-quotes... > + ;; > + man6) > + suffix=1erl > + files=`ls` Please use $( cmd ) , not ` cmd ` > + dstdir=$MAN_INSTALLDIR/man1erl Double-quotes... > + ;; > + esac > + > + for manpage in $files; do > + name=`echo $manpage | cut -d. -f1` Please use $( cmd ) , not ` cmd ` > + filename=$name.$suffix Please add double-quotes and use ${name} since $name. is disambigous - the shell is free to interpret the "dot" as part of the variable name... > + _install M $manpage $dstdir/$filename 444 > + done > + ) > + done > +) > +rm -rf ${MAN_SRC} Double-quotes... > + > +# Uncompress and install html docs > + > +DOC_SRC=otp_doc_html_${VERS} > +DOC_INSTALLDIR=${ROOT}/usr/share/doc/erlang Double-quotes... > + > +mkdir ${DOC_SRC} > +(cd ${DOC_SRC}; > + /usr/bin/gzip -dc ../${DOC_SRC}.tar.gz | /usr/bin/tar xpf - > + for subdir in `ls`; do > + [ -d $subdir ] || continue > + /usr/bin/cp -r $subdir $DOC_INSTALLDIR > + done > +) > +rm -rf ${DOC_SRC} Quotes missing... ... if you don't have time to cleanup the script drop me an email... I can do it then... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
