Dan Hain wrote: > Please review the code for fping's integration into the sfw consolidation > > The webrev is located at: http://cr.opensolaris.org/~dhain/fping/
5min race through http://cr.opensolaris.org/~dhain/fping/sfwnv.patch (patch code is quoted with "> "): > --- /dev/null Tue May 20 09:53:04 2008 > +++ new/usr/src/cmd/fping/Makefile.sfw Tue May 20 09:53:04 2008 > @@ -0,0 +1,95 @@ [snip] > + > +VER=fping-2.4b2_to > +VER64=$(VER)-64 > + > +all: real-all > + IMO it may be nice to define CFLAGS to -- snip -- -xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1 -xstrconst -xspace -- snip -- ("-xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1" enables C99/XPG6 conformance mode, "-xstrconst" folds duplicate constant strings into one (saves runtime memory, like gcc), "-xspace" tries to save space in code. If you have any problems with these flags contact me offline...) > +all32: $(VER)/config.status > + (cd $(VER); env \ > + LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \ > + CC=$(CC) CXX=$(CCC) \ > + "CFLAGS=$(CFLAGS)" \ > + MAKE=/usr/ccs/bin/make \ > + "LIBS=-lnsl -lsocket" \ > + /usr/ccs/bin/make -e) > + @find . -name core -exec rm -f {} \; > + > +all64: $(VER64)/config.status > + (cd $(VER64); env \ > + LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \ > + CC=$(CC64) CXX=$(CCC) \ > + "CFLAGS=$(CFLAGS64)" \ > + MAKE=/usr/ccs/bin/make \ > + "LIBS=-lnsl -lsocket" \ > + /usr/ccs/bin/make -e) > + @find . -name core -exec rm -f {} \; > + > +test: # there aren't any > + > +include ../Makefile.cmd > + > +install: all > + $(SH) ./install-sfw > + MACH64=$(MACH64) $(SH) ./install-sfw-64 > + > +real-all: all32 all64 > + > +$(VER)/config.status: $(VER)/configure > + (cd $(VER); env \ > + CC=$(CC) CXX=$(CCC) \ > + "CFLAGS=$(CFLAGS)" \ > + MAKE=/usr/ccs/bin/make \ > + ./configure --prefix=/usr) > + > +$(VER64)/config.status: $(VER64)/configure > + (cd $(VER64); env \ > + CC=$(CC64) CXX=$(CCC) \ > + "CFLAGS=$(CFLAGS64)" \ > + MAKE=/usr/ccs/bin/make \ > + ./configure --prefix=/usr) > + > +$(VER)/configure: $(VER).tar.gz > + mkdir -p tmp; gzip -dc $(VER).tar.gz | (cd tmp; tar xopf -) > + mv tmp/$(VER) $(VER); rmdir tmp > + (cd $(VER); gpatch -p1 fping.c ../patch-c; cp ../fping.1m fping.1m) > + touch $(VER)/configure > + > +$(VER64)/configure: $(VER).tar.gz > + mkdir -p tmp; gzip -dc $(VER).tar.gz | (cd tmp; tar xopf -) > + mv tmp/$(VER) $(VER64); rmdir tmp > + (cd $(VER64); gpatch -p1 fping.c ../patch-c; cp ../fping.1m fping.1m) > + touch $(VER64)/configure > + > +clean: > + -rm -rf $(VER) > + -rm -rf $(VER64) Please combine these to: -- snip -- -rm -rf \ "$(VER)" \ "$(VER64)" -- snip -- > > --- /dev/null Tue May 20 09:53:05 2008 > +++ new/usr/src/cmd/fping/install-sfw Tue May 20 09:53:05 2008 > @@ -0,0 +1,64 @@ > +#!/bin/sh -e AFAIK the "-e" is a nop-op since you call the script via "sh scriptname" - which causes the shell to skip the "#!" (or better: threat it as comment). IMOit may be better to use "set -o errexit" (for ksh93/bash) or "set -e" (Bourne shell) as _explicit_ statement to avoid any issues with that... [snip] > + > +VERS=2.4b2_to > +PKGVERS=fping-${VERS} > +PREFIX=${ROOT}/usr > +BINDIR=${PREFIX}/bin > +SHAREDIR=${PREFIX}/share > +INFODIR=${PREFIX}/share/info > +LIBDIR=${PREFIX}/lib > +LOCALEDIR=${LIBDIR}/locale > +MAN1MDIR=${SHAREDIR}/man/man1m > + > +. ${SRC}/tools/install.subr If you use ksh93/bash use: -- snip -- source "${SRC}/tools/install.subr" -- snip -- (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot - as a side-effect you can "trap" if the "source" command fails) > + > + > +cd ${PKGVERS} > + > +_install E fping ${PREFIX}/bin/fping 555 Please add double-quotes around argument #3 (http://www.opensolaris.org/os/project/shell/shellstyle/#quote_variables_containing_filenames_or_userinput) ... > +# The fping manpages are updated by Sun to include a comment, an > +# ATTRIBUTES section with stability classification, and a NOTES > +# section containing a pointer to the source package. We do this > +# automatically at install time. If the package is revised, it is > +# possible that additional changes may be required. This can be > +# generally be done by updating the sunman-stability file. > + > +MANSCRIPT=sunman-stability > + > +cd ../ > + > +manpage=fping.1m > +i=${PKGVERS}/${manpage} Double-quotes, please > +_install M ${i} ${MAN1MDIR}/${manpage} 444 Double-quotes, please > +exit 0 > --- /dev/null Tue May 20 09:53:05 2008 > +++ new/usr/src/cmd/fping/install-sfw-64 Tue May 20 09:53:05 2008 > @@ -0,0 +1,43 @@ > +#!/bin/sh -e Same problem as above - the "-e" is not seen by the shell in this case. Please use "set -o errexit" or "set -e" (depending on the shell). [snip] > +VERS=2.4b2_to > +PKGVERS=fping-${VERS}-64 > +PREFIX=${ROOT}/usr > +BINDIR=${PREFIX}/bin > + > +. ${SRC}/tools/install.subr If you use ksh93/bash use: -- snip -- source "${SRC}/tools/install.subr" -- snip -- (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot - as a side-effect you can "trap" if the "source" command fails) > + > + > +cd ${PKGVERS} Double-quotes, please... > + > +_install E fping ${PREFIX}/bin/amd64/fping 555 Double-quotes, please... AFAIK the remaining bits look look Ok for me... :-) ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
