Looks okay to me Paul
On Sat, 2008-06-14 at 00:20, Dan Hain wrote: > Hi Paul, > > Updates are in http://cr.opensolaris.org/~dhain/fping2/ > > Thanks for your input, oter comments inline below. > > Regards, > > Dan > > Paul Cunningham wrote: > > 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. > > > > Resync'd. > > 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" > > > > Ok. I missed the point on CONFIGURE_OPTIONS, but think I have it now. > The other lines have been merged. > > 3. usr/src/cmd/fping/metadata > > This file name is normally uppercase METADATA > > > > Renamed. > > 4. sunman-stability > > Still can't see this in your webrev (or am I > > going blind). > > > > You and me both, it's sitting there in my directory waiting to be > checked in, so webrev missed it while the install didn't. > > 5. usr/src/cmd/fping/install-sfw > > & usr/src/cmd/fping/install-sfw-64 > > Could use ${BINDIR} instead of ${PREFIX}/bin > > > > Ok. > > > === 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 > >> > > > -- > Dan Hain > Solaris Revenue Product Engineering (RPE) > -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
