Thanks for the detailed info, Paul. I'll change my code accordingly. As to your concerns, please see my comments below.
cheers, Simon Paul Cunningham wrote: > 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? \ Once the source compiled, the lib version is 3.2.0. I check the 3.3.0 and compiled it. Its lib version is 4.2.0. > > 6. usr/src/pkgdefs/SUNWlibosip2/copyright > Do you need to add the Sun disclaimer at the top? It's the head included in the original C source file by the author. > > 7. usr/src/pkgdefs/SUNWlibosip2/depend > Have you run the dependency checker script on this > package to ensure you have all the required dependencies ? Yes. > > == End of Comments ====
