Hi Paul, Thanks for the review. I have updated the files and answered some of your queries below.
Point 1 below: Done. Point 2 below: Done. Point 3 below: all the files re copied the headers. Point 4 below: yes, install_dir is correct. Point 5 below: Added /usr/lib/pywbem to Targetdirs. Point 6 below: Correct couple of mistakes on version number and the sentence which included 'planet' in the man page. Point 7 below: Append the version and also changed the ARCH="i386" to ARCH="ISA". Point 8 below: yes, you are right, both would be required, I have pasted the GPL disclaimer also. Point 9 below: yes, it is dependent on Zip for uncompressing the package before installation Point 10 below: yes the setup.py actually extracts to the src folder and then installed to required folders from the extracted folder. Also the new webrev is posted. (http://cr.opensolaris.org/~vivekrt/6833852-pywbem/ ) Let me know if this looks good. Thanks, ~Vivek R. Titarmare -----Original Message----- From: Paul Cunningham [mailto:[email protected]] Sent: Thursday, April 30, 2009 1:53 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "pywbem" See below ... paul Vivek Titarmare wrote: > > Also I have uploaded the updated webrev in the same link. > (http://cr.opensolaris.org/~vivekrt/6833852-pywbem ) > From: Paul Cunningham [mailto:paul.cunningham at tadpole.com] > 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/ > 1. usr/src/lib/pywbem/METADATA > 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. I still think you should put in a real url on the SRC: line 2. usr/src/Targetdirs & usr/src/pkgdefs/Makefile Needs resyncing with the gate by the looks of it. > 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/"; The following are still wrong ... pywbem/Makefile.sfw pywbem/install-sfw and various others (if not all) > 4. usr/src/lib/pywbem/install-sfw Line ... 65 install_dir "*" ${ROOTPYWBEM} 755 444 is this correct ? > Should ... > /usr/lib/pywbem and /usr/share/doc/pywbem be in Targetdirs? > > [VIVEK] yes doc was missing in the Targetdirs. Now it is updated. I think /usr/lib/pywbem should be in Targetdirs as well! > 6. usr/src/lib/pywbem/sunman/pywbem.3 I'm not sure you say the following in a sun man page ... 41 Pywbem also provides a Python provider interface, and is the fastest and easiest way to write providers on the planet. ie. the bit about ... "fastest and easiest way to write providers on the planet" Do you need to refer to the version in a man page ? ... 42 The latest version of Pywbem is 0.7 ...... > 7. usr/src/pkgdefs/SUNWpywbem/pkginfo.tmpl > This doesn't look like a template file, it has ARCH="i386" > defined ? this still doesn't look right > Add version number yo end of DESC= line > > [VIVEK] version added It's not in the webrev! > 8. usr/src/pkgdefs/SUNWpywbem/copyright > Is this the correct Sun disclaimer statement for LGPL ? > > [VIVEK] LGPL Disclaimer Notice is prepend in the copyright file. Does it need both of those two disclaimer statements? > 9. usr/src/pkgdefs/SUNWpywbem/depend > Does this need to include the normal default dependencies > also ? > > [VIVEK] added default dependencies Does it need the zip dependencies ? 10. usr/src/lib/pywbem/Makefile.sfw What is this line doing for you ? ... 45 (cd $(VER); $(PYTHON) setup.py install --install-lib .. $(SRC)/lib/pywbem/$(VER)) it looks to me as though you are installing stuff back into your build dir! -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
