Now sure if this got sent first time ..
On Tue, 2008-06-10 at 13:54, 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.
>
> 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
