Hi Paul, Pl. see my comments INLINE.
Also I have uploaded the updated webrev in the same link. (http://cr.opensolaris.org/~vivekrt/6833852-pywbem ) Thanks, ~Vivek R. Titarmare -----Original Message----- From: Paul Cunningham [mailto:[email protected]] Sent: Wednesday, April 29, 2009 2:23 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "pywbem" Vivek, See comments below .... Paul Vivek Titarmare wrote: > I have posted a webrev for package "pywbem" which I am porting to > OpenSolaris and would like to request a code review. Please see below link > http://cr.opensolaris.org/~vivekrt/6833852-pywbem/ === Start of Comments === 1. usr/src/lib/pywbem/METADATA & usr/src/lib/pywbem/pywbem-0.7.tar.gz Line ... 6 SRC: pywbem-0.7.0.tar.gz from http://pywbem.sf.net/ says version 0.7.0 (and 4 VERSION:) but your tarball is 0.7. Why ? SRC: line, why no specify the direct link ? ... "http://kent.dl.sourceforge.net/sourceforge/pywbem/pywbem-0.7.0.tar.gz"; this uncompresses into pywbem-0.7.0. [VIVEK] Yes, you are right. The downloaded file had 0.7.0 version. Now the correct gz file should be seen in the webrev. 2. usr/src/lib/pywbem/Makefile.sfw Lines .. 28 #VER= $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) 29 VER=pywbem-0.7 why have you hardcoded this ? when the tarball uncompresses to dir pywbem-0.7.0 Define 'TARBALL=$(VER).tar.gz' and then use $(TARBALL) instead of '$(VER).tar.gz' throughout [VIVEK] Yes, the problem was due to the version info in METADATA. Now it is changed. 3. Top-of-files (various) Cosmetic: Change top-of-file so it conforms to (missing line-space, etc) ... "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; [VIVEK] verified and corrected 4. usr/src/lib/pywbem/install-sfw Line 1, change to /usr/bin/ksh93 [VIVEK] done. Line .. 102 cd ${BASEDIR} is this putting you in the correct place for the following stuff ? [VIVEK] line removed. Line .. 103 install_dir "pywbem.txt" ${ROOTPYWBEM_DOC} 755 444 is "pywbem.txt" correct for a directory ? [VIVEK] line removed. Should ... /usr/lib/pywbem and /usr/share/doc/pywbem be in Targetdirs? [VIVEK] yes doc was missing in the Targetdirs. Now it is updated. 5. usr/src/lib/pywbem/cim_schema_2.20.0Experimental-MOFs.zip Where is this used? [VIVEK] This is a test suit which was required for testing. I have remove this. 6. usr/src/lib/pywbem/sunman/pywbem.3 Line ... 68 Availability SUNWpyWBEM elsewhere its SUNWpywbem [VIVEK] Done. 7. usr/src/pkgdefs/SUNWpywbem/pkginfo.tmpl This doesn't look like a template file, it has ARCH="i386" defined ? Add version number yo end of DESC= line [VIVEK] version added 8. usr/src/pkgdefs/SUNWpywbem/pkginfo.tmpl Is this the correct Sun disclaimer statement for LGPL ? [VIVEK] LGPL Disclaimer Notice is prepend in the copyright file. 9. usr/src/pkgdefs/SUNWpywbem/depend Does this need to include the normal default dependencies also ? [VIVEK] added default dependencies 10. usr/src/pkgdefs/SUNWpywbem/prototype_i386 & usr/src/pkgdefs/SUNWpywbem/prototype_sparc Cosmetic: add package name in comment for consistency as in .. "http://cr.opensolaris.org/~vivekrt/6816355-xstream/usr/src/pkgdefs/SUNWxstr eam/prototype_sparc.html" [VIVEK] done. === End of Comments ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
