Dan, Here are a few more comments, see below ..
Paul On Mon, 2008-06-09 at 18:51, Dan Hain wrote: > > Thank you for your feedback! > > Please see the updated webrev at http://cr.opensolaris.org/~dhain/fping1/ > > Other comments inline below. === More Comments ========== 1. usr/src/cmd/Makefile & usr/src/pkgdefs/Makefile Looks as though it needs resysncing with gate, otherwise it looks as though your are trying to delete stuff. 2. usr/src/cmd/fping/Makefile.sfw You are still hard coding '--prefix=', you might want to use the predefined value in CONFIGURE_OPTIONS, see http://cr.opensolaris.org/~rayx/erlang/webrev/usr/src/cmd/erlang/Makefile.sfw.html Could you put the following all on the same line, so its less confusing .. 40 $(MAKE) \ 41 "LIBS=-lnsl -lsocket" \ 42 "MAKE=$(MAKE) -e" 3. usr/src/cmd/fping/metadata This file name is normally uppercase METADATA 4. sunman-stability Still can't see this in your webrev (or am I going blind). 5. usr/src/cmd/fping/install-sfw & usr/src/cmd/fping/install-sfw-64 Could use ${BINDIR} instead of ${PREFIX}/bin === End of More Comments === > Paul Cunningham wrote: > > > > ==== Start of Comments ==== > > > > 1. usr/src/cmd/fping/Makefile.sfw > > Could you use the '--prefix=..' value predefined in Makefile.master? > > see example in .. > > http://cr.opensolaris.org/~rayx/erlang/webrev/usr/src/cmd/erlang/Makefile.sfw.html > > > > > > Also is there a predefined MAKE, if so could you use that? > > > > Do you need the 'real-all:' rule, couldn't it just be > > 'all: all32 all64' instead? > > > > Do you need to do the 'cp ../fping.1m fping.1m' in > > '$(VER64)/configure:' as you only install in from $(VER) ? > > Corrected > > > > 2. METADATA > > You don't seem to have a METADATA file. > > Corrected. > > > > 3. usr/src/cmd/fping/install-sfw > > Do you need to define LOCALEDIR= & INFODIR=, I don't think they > > are used? > > > > Why use 'i=...' when its only used once? > > Removed. > > > > You don't seem to have a sunman-stability file (in the webrev). > > Added. > > > > Copyright year is wrong. > > Updated. > > > > 4. usr/src/cmd/fping/install-sfw-64 > > Copyright year is wrong. > > > > What happens on a sparc build - amd64/fping ? > > No, it should go to sparcv9/fping. That and the copyright have been > corrected. > > > > 5. usr/src/common/rbac/exec_attr > > Copyright year needs changing. > > > > 6. usr/src/pkgdefs/Makefile > > You haven't added 'SUNWfpingr' > > Corrected. > > > > 7. usr/src/pkgdefs/SUNWfping/Makefile > > & usr/src/pkgdefs/SUNWfping/pkginfo.tmpl > > & usr/src/pkgdefs/SUNWfping/prototype_sparc > > Copyright year is wrong. > > Corrected. > > The sccs ident stuff looks wrong. > > It passes our internal checks, so I'm not sure what looks wrong. Please > clarify. > > > > 8. usr/src/pkgdefs/SUNWfping/copyright > > Should this include some copyright years? > > I agree this seems strange, but this is the copyright the distribution > requires be included, it does not contain any years. According to the > distributions ChangeLog file, the original copyright with dates in the > RCS header is no longer required. No dates appear in the fping.c file > copyright notice, only in the ChangeLog file. > > > > Why is it not the same as SUNWfpingr/copyright? > > Copy error, this has been corrected. > > > > 9. usr/src/pkgdefs/SUNWfping/prototype_sparc > > Some there be sparc 64 bit stuff? > > Yes. Target should have been /usr/bin/sparcv9/fping. I've corrected that. > > > > 10. usr/src/pkgdefs/SUNWfpingr/prototype_com > > SUNWmkcdr - name comment wrong. > > Corrected. > > > > ==== End of Comments ====== > > > > Pradhap Devarajan wrote: > > Hi Dan, > > Can you add metadata file as part of the putback > > http://cr.opensolaris.org/~pd155743/librsync/usr/src/lib/librsync/METADATA.html > > .It would be helpful for future reference. > > The metadata file has been added. > > > Roland Mainz wrote: > > 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...) > > > > Added, using the macros from Makefile.master where possible. (as > requested by Keith.Wesolowski at Sun.COM) > > > >> + > >> +clean: > >> + -rm -rf $(VER) > >> + -rm -rf $(VER64) > >> > > > > Please combine these to: > > -- snip -- > > -rm -rf \ > > "$(VER)" \ > > "$(VER64)" > > -- snip -- > > > > Done. > > > >> --- /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) > > Done. > > > > > > > >> + > >> + > >> +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) > > ... > > > > <snip> > >> + > >> +manpage=fping.1m > >> +i=${PKGVERS}/${manpage} > >> > > > > Double-quotes, please > > > > > >> +_install M ${i} ${MAN1MDIR}/${manpage} 444 > >> > > > > Double-quotes, please > > > > Done > > > >> +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... > > > > > > Done. > > > Thank you all again! > > > Dan -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
