Petr Slechta wrote: > this is the second round of code review for project "Integrate Grails > web framework (http://grails.org) into OpenSolaris 2008.11". > > I incorporated feedback from Petr Sumbera, Paul Cunningham, and Roland > Mainz. Many thanks for the feedback! > > I would like to ask my reviewers and anybody who is interested to review > the code one more time: http://cr.opensolaris.org/~pslechta/grails2/
Quick look at http://cr.opensolaris.org/~pslechta/grails2/grails.patch (patch code is quoted via "> "): > -- /dev/null Thu Aug 21 17:03:39 2008 > +++ new/usr/src/cmd/grails/Makefile.sfw Thu Aug 21 17:03:39 2008 > @@ -0,0 +1,49 @@ [snip] > + > +GRAILS_VER=1.0.3 > +GRAILS_HOME=grails-${GRAILS_VER} > +MY_SRC_DIR=${SRC}/cmd/grails > + > +include ../Makefile.cmd > + > +clean: > + rm -rf ${GRAILS_HOME} > + > +all: ${GRAILS_HOME} > + (cd ${GRAILS_HOME}; env \ Please use... -- snip -- env - \ -- snip -- ... to make sure the build system doesn't pick-up (or rely) any environment variables which shouldn't be picked up (this mainly applies to "autoconf"/"configure" but IMO it may be nice to try to be "safe" and apply this rule by default for all external build infrastructure code). > + > PATH=${MY_SRC_DIR}/${GRAILS_HOME}/bin:${MY_SRC_DIR}/${GRAILS_HOME}/ant/bin:${PATH} > \ > + JAVA_HOME=${JAVA_ROOT} \ > + ant) [snip] > --- /dev/null Thu Aug 21 17:03:49 2008 > +++ new/usr/src/cmd/grails/install-grails Thu Aug 21 17:03:49 2008 > @@ -0,0 +1,81 @@ > +#!/usr/bin/ksh93 Please test the script with $ ksh93 -n new/usr/src/cmd/grails/install-grails # and fix the warnings you'll get... > --- /dev/null Thu Aug 21 17:03:49 2008 > +++ new/usr/src/cmd/grails/src/grails Thu Aug 21 17:03:49 2008 > @@ -0,0 +1,18 @@ > +#!/bin/sh > + > +if [ -z "$GRAILS_HOME" ]; then > + echo "Please set GRAILS_HOME environment variable to location, where > Grails is installed." > + echo "(Default location is /usr/grails/[version], e.g. > /usr/grails/1.0.3)" > + exit 1 > +fi > + > +if [ ! -f "$GRAILS_HOME/bin/startGrails" ]; then > + echo "GRAILS_HOME is not set properly: $GRAILS_HOME" > + echo "It must contain path to the directory where Grails is installed!" > + exit 1 > +fi > + > +. "$GRAILS_HOME/bin/startGrails" > + > +startGrails org.codehaus.groovy.grails.cli.GrailsScriptRunner "$@" 1. Please read http://www.opensolaris.org/os/project/shell/shellstyle/, items http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_echo and use $ printf "str=%s\n" "$s" #, not $ printf "str=$s\n" # since the value of "s" may be intepreted as the format string (the same problems apply to "echo", too). 2. Error messages go to stderr, e.g. you have to replace... -- snip -- $ echo WARNING, chicken may implode" # -- snip -- ... with .. -- snip -- $ echo WARNING, chicken may implode" 1>&2 # -- snip -- 3. Please use /usr/bin/ and not /bin/ in paths since /bin/ is a softlink to /usr/bin/ (this applies to #!/bin/sh) ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
