Hi Mark, Please find the new webrev http://cr.opensolaris.org/~spoorthy/perl-crypt-cbc1/
Thanks Spoorthy Mark Phalan wrote: > On Fri, 2009-08-21 at 11:36 +0530, Spoorthy H.S wrote: > >> Hi, >> >> I am porting the package Crypt-CBC. Please review the code and reply me >> for the changes. Webrev is at http://cr.opensolaris.org/~spoorthy/crypt-cbc/ >> > > > usr/src/Makefile > Shouldn't be in the active list as there are no changes. > > usr/src/lib/Makefile > This is a perl module. That should probably be reflected in the name. I > would suggest using perl_crypt_cbc. I have changed the name to perl-crypt-cbc and package name to SUNWperl-crypt-cbc > Will you deliver a version for both > versions of Perl (5.8, 5.10)? You may want two sub-directories in this > case. > > I am going to port the package for perl 5.10.0. As of now, i'm testing on 5.8.4. > usr/src/lib/crypt-cbc/METATDATA > * No need for PROGRAM > * Should probably mention that it is a perl module in the DESCRIPTION > * I think the package name should include "perl". You probably will need > a second package for Perl 5.10 > * BUGTRAQ line is incomplete. > > usr/src/lib/crypt-cbc/Makefile.sfw > 28,29: Why are these comments > 30,31: Don't hardcode the package name and version into the Makefile. > Use something like: > VER=$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) > TARBALL=$(VER).tar.gz > 34: Why are you adding to "CONFIGURE_OPTIONS" instead of setting it? > 37: No need for subshell. > 37,38,39,40: Why are you setting CC, CXX and LD_OPTIONS. This is purl > perl module > 42: Why GMAKE? Does this require GNU make? > 43: Why "make install"? You have a separate target for "install". SFW is > normally built without root permissions. > 37-44: Indentation looks wrong. > 51-57: Similar comments as above. > 60-61: These lines should be removed > 63: Instead of unpacking yourself you should use $(SFW_STAMP_UNPACKED) > > Made changes. I will change the VER in the next code review. > usr/src/lib/crypt-cbc/install-sfw > * Looks like you're installing into /lib/crypt-cbc. Perl modules need to > be installed into /usr/perl5/. As this module is delivered by > Solaris/Sun you need to install into vendor. This should be specified by > the ARC case. > * You shouldn't be installing tests (*.t). > * aes.pl etc: This looks wrong. What are these used for? If they are for > general use then they should be in a bin directory and not have the .pl > extension. > Done > usr/src/lib/crypt-cbc/sunman-stability > * Looks like this file hasn't been updated at all for your package > (looks like you copied it from expect). > > Thats all I've had time for today. > > -M > > >
