Hi Muktha, I'm a "greener" reviewer than others on this list but i'll take a stab at this. You'll want to get a 2nd opinion (perhaps after addressing this set of comments).
Webrev: http://cr.opensolaris.org/~muktha/konkretcmpi/ 1) Your METADATA file needs to follow standard outlined on this wiki: http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines Please use CAPS for field names, add PROGRAM field, change "Package name" to PACKAGE, add SRC url, change "GPL" to "GPL v2", and change value of BUGTRAQ from your CR ID to the product/category/subcategory people should use when filing bugs against konkret. 2) Per Jim Walker, do not use "pragma ident" keywords in files other than source files. In Makefiles, install scripts and such, use "ident" (without "pragma"). 3) In Makefile.sfw, you can avoid hard-coding program name and version and instead extract this info from METADATA, something like: VER = $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) For the same effect in install-sfw script, you'd need to pass VERS as an environment variable from the Makefile. See example in webalizer Makefile.sfw and install-sfw. 4) Why do you overwrite SFW_PATH setting in Makefile.sfw? If it's necessary, please add a comment to explain what tools your build is getting off the special path. 5) In Makefile.sfw, it is advisable to use $(SHELL) instead of $(SH) to ensure predictability of which shell will get called. You may want to call configure as "$(SHELL) ./configure" for the same reason. 6) In install-sfw and install-sfw-64, it is recommended to use /usr/bin/ksh93 instead of /bin/sh. This allows you to add: # stop at first error set -o errexit And to call "source" instead of "." in this line: source ${SRC}/tools/install.subr The purpose of this is to make the install script exit on error instead of silently forging ahead. See examples in webalizer and a few other packages using ksh93. 7) I don't believe you need sunman-stability stuff because you are creating a new manpage, rather than using one from open-source distribution. From the package guidelines wiki: http://ostest.central.sun.com/wiki/index.php/Package_Delivery_Project#3._Develop_Software If you do create a new man page, the sunman-stability file is not needed. 8) Fix copyright year in the CDDL headers, it should be 2008. CDDL headers should be consistent across all your files. The easiest way is to copy the current header from here: /net/onnv.sfbay/export/onnv-tw-gate/usr/src/prototypes/prototype* 9) You are using the default 'depend' file in the package, but your build appears to pull in some headers from Pegasus. Is this only a compile-time dependency? Please verify that your binary does not have any runtime dependencies (beyond the default ones such as libc, etc). You can run "ldd" to see what it's linking against. If runtime dependency on another package exists, please create your own depend file (copy the default and add packages you explicitly depend on). Then remove this line from your package Makefile: DATAFILES=depend That's it for now, I'll take another look when you've addressed these comments, and be sure to get a second reviewer besides me. Thanks, -- daria
