Paul, thanks for your detailed comments, I've modified most of them, one was left for the reason of uncertainty. In comment 5, I don't know where to attach the CDDL stuff, I didn't see any example to attach a license in a manual page. Could you refer me one if you have? Another question is, do I just need to attach the sunman stuff right after the manual page what I wrote? When I do that, I found it is not readable using "man libexosip2" command.
Below is the comment: 5. usr/src/lib/libexosip2/exosip.1 You should probably add the CDDL and Copyright stuff. You should add the sunman stability stuff. Is it normal to include the AUTHOR stuff on a Sun man page ? The attachment is the output of command "man libexosip2". Thanks, Hao Paul Cunningham ??: > Hao, > > See my comments below ... > > Paul > > Hao Wang wrote: >> The webrev page: >> http://cr.opensolaris.org/~snowsky/webrev/ > >>>> >>>> The package of libeXosip2 is on the cr.opensolaris.org for the >>> purpose of review. >>>> There is one thing to remind that according to Jim's suggestion, >>> this package is extension of libosip2, >>>> it is convenient for you reviewing it together with libosip2 package. > > === Start of Comments ==== > > 1. usr/src/lib/libexosip2/METADATA > Change so it conforms to that in ... > "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines"; > ie. add missing fields, etc. > > 2. CDDL and top of file (various files) > Cosmetic: Change so they conform to the prototypes in .. > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > > ie. missing > # > <line-space> > after # CDDL HEADER END > > 3. Copyright year (various files) > Change to this year. > > 4. usr/src/lib/libexosip2/Makefile.sfw > Extract the VER= info from the METADATA - there are numerous > examples in the gate now. > > Add TARBALL= and then replace '$(VER).tar.gz' with $(TARBALL) > throughout. > > Pass VER into install-sfw so that it can be used in > there (rather than hard-coding it). > > Line ... > 32 CONFIGURE_OPTIONS = prefix=/usr > is that the correct format for 'prefix=' option of the pkg's > 'configure' (its normally --prefix=) ? > > Line ... > 31 SHELL = /usr/bin/bash > is there a reason why you are redefining SHELL rather > than using the predefines value from Makefile.master ? > > > 5. usr/src/lib/libexosip2/exosip.1 > You should probably add the CDDL and Copyright stuff. > > You should add the sunman stability stuff. > > Is it normal to include the AUTHOR stuff on a Sun man page ? > > 6. usr/src/lib/libexosip2/install-sfw > Pass in VERS= info from the Makefile.sfw - there are numerous > examples in the gate now. > > You should probably add this dir to Targetdirs ... > 41 mkdir -p ${INCDIR}/eXosip2 > > Change as follows ... > Roland Mainz wrote: > > use /usr/bin/ksh93 for install-sfw* and add a > > $ set -o errexit # at the beginning and > > replace ". ${SRC}/tools/install.subr" with > > "source ${SRC}/tools/install.subr" (the idea is to > > catch failures in the script and abort it at that point, > > right now the script will just continue) > > Being as you wrote the exosip.1 man page you could add > the sunman-stability stuff directly in the file (see item 5), > then you don't need ... > 59 MANSCRIPT=../sunman-stability > and could change ... > 60 _install M ....... > to .. > _install N ...... > > Should you be delivering these ... > 50 _install N src/.libs/libeXosip2.a ${LIBDIR}/libeXosip2.a 555 > 51 _install N src/libeXosip2.la ${LIBDIR}/libeXosip2.la 555 > (static lib and libtool stuff) ? > > 7. usr/src/lib/libexosip2/sunman-stability > Not needed if you add it directly into exosip.1, see items > 5 & 6. > > The package name is wrong SUNWlibexosip2 and libexosip2 > > 8. usr/src/lib/Makefile > & usr/src/pkgdefs/Makefile > These changes are not in your webrev! > > 9. usr/src/pkgdefs/SUNWlibexosip2/Makefile > You have your own 'depend' file so you don't need ... > 30 DATAFILES=depend > but also see item 10 > > 10. usr/src/pkgdefs/SUNWlibexosip2/depend > This looks like the default 'depend' file. Have you > checked you have no other dependencies with the dependency > script. If there are add them here. If not, remove this > file and ignore item 9 > > Move the Copyright lines to after the 'CDDL HEADER END', > see item 2 > > 11. usr/src/pkgdefs/SUNWlibexosip2/pkginfo > Remove this file, its autogenerated from pkginfo.tmpl > > 12. usr/src/pkgdefs/SUNWlibexosip2/copyright > Add the Sun disclaimer stuff and the source pkg owner > copyright lines at the top, see example at ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > > > 13. usr/src/pkgdefs/SUNWlibexosip2/prototype_com > Lines ... > 50 f none usr/lib/eXosip2/libeXosip2.a 0644 root bin > 51 f none usr/lib/eXosip2/libeXosip2.la 0755 root bin > see item 6 (static lib, etc) > > > 14. #pragma ident (various files) > You don't need the 'pragma' on the sccs ident lines > > === End of Comments ====== -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: libexosip2 URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090505/443c1b3e/attachment.ksh>
