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 ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
