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. Will you deliver a version for both
versions of Perl (5.8, 5.10)? You may want two sub-directories in this
case.
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)
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.
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
