Hi Paul, Pradhap, Roland, Thank you for your feedback!
Please see the updated webrev at http://cr.opensolaris.org/~dhain/fping1/ Other comments inline below. Best regards, Dan 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
