Hi Paul, Thanks for reviewing the webrev.
Please see responses in line. Paul Cunningham said the following on Monday 24 November 2008 02:17 PM: > Srirama, > > See comments below from my quick skip through ... > > Paul > > Srirama Sharma wrote: > >> Requesting a code review for SimpleWBEM. SimpleWBEM provides command >> line tools for building CIM (Common Information Model) providers that >> are compatible with several CIM server implementations like OpenPegasus. >> >> Webrev: http://cr.opensolaris.org/~srirama/simplewbem/ >> Bug: http://bugs.opensolaris.org/view_bug.do?bug_id=6772743 >> >> Please note that this is targeted to get integrated in to snv_105 build. >> > > === Start of Comments ==== > > 1. usr/src/cmd/simplewbem/METADATA > You might want to add the 'NAME:' field see ... > http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines > Done. > 2. usr/src/cmd/simplewbem/*.1 > Did you write these man pages, if so you might need > to add the Sun copyright and CDDL stuff. > Yes. I wrote these manpages. Have now added the Sun Copyright & CDDL headers and have removed the sunman-stability section. > 3. usr/src/cmd/simplewbem/Makefile.sfw > You could extract the VER= stuff from the METADATA info, > see recent sfwnv package integrations. > Done. > Change 'env ' to 'env - ' throughout > Done. > It's not normal to have specific $(VER)_*/config.status > machine arch lines - so why have you got them? > In case of simplewbem (cimple), the "--host" configure option needs to be specified based on which the corresponding make file routines/targets are called during build. Here the configure information supplied through "--host" option is different for sparc/x86 and 32/64 bit. Hence we need to have 4 combinations of $(VER)_*/config.status > Do you need separate 'clean32:' & 'clean64:' rules? You > could just have ... > clean: > -rm -rf $(VER) $(VER64) > My intention was to provide a way so that one can retain the 32 bit build dir and redo 64 bit build or vice versa if required. Have now modified the Makefile.sfw to have a single 'clean' rule as per your comment. > 4. various files - #pragma ident ... > You don't need the 'pragma', I thing that's normally > only required for c, etc, code files - see previous > Jim Walker comments. > Done. > 5. usr/src/cmd/simplewbem/install-sfw > & usr/src/cmd/simplewbem/install-sfw-64 > You could pass in the VERS= stuff as an environment > variable from the Makefile.sfw > > Roland Mainz wrote: > > 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) > Done. > Do you need the 'write permission' set on the library > files ? if not remove it. > Done. Have removed the write permission. The libraries are now installed with 555 permission. > 6. usr/src/pkgdefs/SUNWsimplewbem/Makefile > Remove the null DATAFILES= line > Done. > 7. usr/src/pkgdefs/SUNWsimplewbem/depend > Move the Copyright lines down to after the > 'CDDL HEADER END' header. > And correct copyright year. > Done. Had missed to correct the copyright year after copying the 'depend' file from "usr/src/pkgdefs/common_files" dir. Guess that 'depend' file needs modification too ? > 8. usr/src/pkgdefs/SUNWsimplewbem/pkginfo.tmpl > On the 'DESC= line move the version to the end of the > line, eg. > DESC=" .......... (1.2.4)" > Done. > 9. usr/src/pkgdefs/SUNWsimplewbem/prototype_com > & usr/src/pkgdefs/SUNWsimplewbem/prototype_i386 > & usr/src/pkgdefs/SUNWsimplewbem/prototype_sparc > Do you need the 'write permission' set on the library > files ? if not remove it. > > === End of Comments ===== Done. The libraries are now packaged with 555 permission. Have updated the webrev at http://cr.opensolaris.org/~srirama/simplewbem/ with all the above changes. Please review. Thanks, Srirama -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20081125/a5757c89/attachment.html>
