Simon, This mainly looks good, see below for my comments ...
Paul > Simon Sun wrote: >> >> I'm porting libosip2, a low layer SIP implementation. >> >> Please help to code review my modification. The webrev is at: >> http://cr.opensolaris.org/~dysun/libosip2/ >> >> The comments will be changed accordingly once the subcategory gets >> approval. Thanks. == Start of Comments == 1. usr/src/lib/libosip2/Makefile.sfw Lines ... 77 -rm -rf $(VER) 78 -rm -rf $(VER64) combine into one, eg. ... -rm -rf $(VER) $(VER64) Apply this .. Roland Mainz wrote: > use either $(SHELL) or /usr/bin/bash for "configure" > calls (so we know which one is used and "configure" > doesn't pick one itself) 2. usr/src/lib/libosip2/install-sfw Line ... 43 _install M libosip2.3lib ${MANDIR}/libosip2.3lib 444 you have used "_install M" but you don't seem to have a MANSCRIPT= line or the sunman-stability sed file to add the man page stability stuff. BUT JUST noticed it is your own man-page file, so that just needs to be "_install N" I think. 3. CDDL header (various files) Cosmetic: In CDDL header replace double-space with single-space so it conforms to .. "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; 4. usr/src/lib/libosip2/install-sfw-64 Delete lines as they are not used .. 35 SHAREDIR=${PREFIX}/share 36 MANDIR=${SHAREDIR}/man/man3lib 5. usr/src/lib/libosip2/install-sfw-64 & usr/src/lib/libosip2/install-sfw Assuming 3.2.0 is a version number (or am I being stupid - if its not ignore this)? ... The version number 3.2.0 could be passed in from the Makefile.sfw (extracted from METADATA) that way you may not have to update this/these files when the src package version is updated. Why are these 3.2.0 when the src pkg version is 3.1.0? 6. usr/src/pkgdefs/SUNWlibosip2/copyright Do you need to add the Sun disclaimer at the top? 7. usr/src/pkgdefs/SUNWlibosip2/depend Have you run the dependency checker script on this package to ensure you have all the required dependencies ? == End of Comments ==== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
