I will review this one. 

----------------------------------------
Review for release 2:

* RPM name is OK
* This is the latest version
* Builds fine in mock (rawhide)
* rpmlint failed
* File list looks OK
* Config files of sqlgrey looks OK

Needs work:
* Encoding should be UTF-8

Some of the changelog entries contain the name "�stein Viggen" which is tripping
this rpmlint error. 

* there must be a proper %clean section
  (wiki: Packaging/ReviewGuidelines)

You %clean section should look like:

%clean
rm -rf $RPM_BUILD_ROOT

not:

%clean
make clean

* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)

You should use "%{_initrddir}" instead of "%{_sysconfdir}/init.d/"

* No downloadable source. Please give the full URL in the Source tag.

You should use "http://download.sourceforge.net/%{name}/%{name}-%{version}.tar.gz 

* The BuildRoot must be cleaned at the beginning of %install

You need to add "rm -rf $RPM_BUILD_ROOT" before you run make install

* The package should contain the text of the license
  (wiki: Packaging/ReviewGuidelines)

There is a COPYING file in the tarball. Please add this to the %docs

rpmlint of sqlgrey:
W: sqlgrey non-conffile-in-etc /etc/sqlgrey/README

This shouldn't be there? 

E: sqlgrey init-script-without-chkconfig-postin /etc/init.d/sqlgrey
E: sqlgrey init-script-without-chkconfig-preun /etc/init.d/sqlgrey

You should atleast use chkconfig to add the init script. 

W: sqlgrey no-reload-entry /etc/init.d/sqlgrey
W: sqlgrey service-default-enabled /etc/init.d/sqlgrey
E: sqlgrey subsys-not-used /etc/init.d/sqlgrey

The init script is not very Fedora friendly. Perhaps a new one should be created
and supplied? 

Warren, I hope you don't mind, but I updated to the latest version and made some
other changes:

  http://ftp.kspei.com/pub/steve/rpms/sqlgrey/sqlgrey.spec
  http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-1.src.rpm

AFAIK, the only major issue left is that the init script is extremely broken. 
I'm going to try to work on that right now.

Oh, and I'm inclined to completely drop the upstream changelog.  Thoughts?

Thanks Steven, I haven't been able to prioritize working on this.  Would you
like to take over ownership?

(In reply to comment #3)
> Thanks Steven, I haven't been able to prioritize working on this.  Would you
> like to take over ownership?

Sure.

One more question...  There doesn't appear to be an explicit dependency on
postfix in this package.  Should there be?

Oops, yes. =)

I think this is pretty close:

  http://ftp.kspei.com/pub/steve/rpms/sqlgrey/sqlgrey.spec
  http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-2.src.rpm

There's the pesky little detail that it doesn't seem to actually *work*, but the
package seems clean to me.  :-)

Sorry. Due to my stepping out for a while, I am unable to complete this review. 

I'd be happy to take over this review... 

OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
b84931d638c3527e2dabc26ad6754bc0  sqlgrey-1.7.4.tar.bz2
b84931d638c3527e2dabc26ad6754bc0  sqlgrey-1.7.4.tar.bz2.1
OK - Package compiles and builds on at least one arch.
OK - BuildRequires correct
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package doesn't own any directories other packages own.
See below - No rpmlint output.

SHOULD Items:

See below - Should include License or ask upstream to include it.
OK - Should build in mock.
Builds ok on fc6 i386|x86_64 and fc5 i386|x86_64.
OK - Should have sane scriptlets.

Issues:

1. Might include the gpl COPYING file?

2. Might include the Changelog and CONTRIB as %doc files?

3. rpmlint says:

W: sqlgrey conffile-without-noreplace-flag /etc/sqlgrey/README

Does this need to be in /etc/sqlgrey? Or can it be in doc?

E: sqlgrey non-standard-uid /var/lib/sqlgrey sqlgrey
E: sqlgrey non-standard-gid /var/lib/sqlgrey sqlgrey
E: sqlgrey non-standard-dir-perm /var/lib/sqlgrey 0750

All those can be ignored I think.

4. Have you been able to determine the issue from comment #6 about it not 
working? 

(In reply to comment #8)
> 1. Might include the gpl COPYING file?
> 
> 2. Might include the Changelog and CONTRIB as %doc files?

I'll fix that in -3.

> 3. rpmlint says:
> 
> W: sqlgrey conffile-without-noreplace-flag /etc/sqlgrey/README
> 
> Does this need to be in /etc/sqlgrey? Or can it be in doc?

Here are the contents of that file:

  README file for sqlgrey updated content
  
  Don't touch these files, they are automatically updated when
  you run update_sqlgrey_config:
  - clients_fqdn_whitelist: don't greylist these DNS names [*]
  - clients_ip_whitelist  : don't greylist these IP addresses [*]
  - dyn_fqdn.regexp       : used by new 'smart' algorithm [x]
  - smtp_server.regexp    : used by new 'smart' algortihm [x]
  
  [*]: in use starting with 1.4.0
  [x]: in use since 1.5.1, regexps looking for known fqdns patterns

So I think it seems appropriate to leave it in that directory.

> E: sqlgrey non-standard-uid /var/lib/sqlgrey sqlgrey
> E: sqlgrey non-standard-gid /var/lib/sqlgrey sqlgrey
> E: sqlgrey non-standard-dir-perm /var/lib/sqlgrey 0750
> 
> All those can be ignored I think.

Agreed.

> 4. Have you been able to determine the issue from comment #6 about it not 
> working? 

Not yet, but until today I haven't taken a moment to look at it.

Once I'm done with my last couple of pre-FC6 package upgrades, I should have
some time for this again.

(In reply to comment #9)
> > 4. Have you been able to determine the issue from comment #6 about it not 
> > working? 
> 
> Not yet, but until today I haven't taken a moment to look at it.

Jumping in here, I installed this package on my secondary MX which is running
FC-3.  It works with the SQLite option; I haven't tested with MySQL or PgSQL.  I
also installed it on an FC-5 box for testing.  On this box, it also appears to
be working, but the init script gives some warnings:

Name "DBIx::DBCluster::DEBUG" used only once: possible typo at /usr/sbin/sqlgrey
line 2413.
Name "DBIx::DBCluster::WRITE_HOSTS_NEVER_READ" used only once: possible typo at
/usr/sbin/sqlgrey line 827.
Name "DBIx::DBCluster::CLUSTERS" used only once: possible typo at
/usr/sbin/sqlgrey line 818.

I notice that the DBIx::DBCluster Perl module included in the sqlgrey
distribution isn't being installed.  In my brief review of the code, I'm not
sure including the module would help much; it doesn't appear to ever get loaded
by the program.  I don't see anything about it in the sf.net bug tracker, but
maybe we can include a note about it?

Ping Steven. Any chance to look at this again? 

I'm planning to work on it again on Tuesday (10/10).

Hey Steven. Any news on this package? 

Not yet.  I'm trying to set up some new systems to test it on, but that work 
has been held up for various reasons.

Someone is going to have my head if I don't get it working for them by Friday 
(11/17) though.  :-)

I figured out my issue.  As-is, the package doesn't depend on any DBD modules,
but it defaults to using DBD::Pg:

Nov 17 12:01:32 localhost sqlgrey: fatal: Can't locate DBD/Pg.pm in @INC (@INC
contains: /usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.6/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.8 /usr/lib/perl5/site_perl/5.8.7
/usr/lib/perl5/site_perl/5.8.6 /usr/lib/perl5/site_perl/5.8.5
/usr/lib/perl5/site_perl
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.7/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.6/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.5/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.8 /usr/lib/perl5/vendor_perl/5.8.7
/usr/lib/perl5/vendor_perl/5.8.6 /usr/lib/perl5/vendor_perl/5.8.5
/usr/lib/perl5/vendor_perl /usr/lib/perl5/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/5.8.8 .) at (eval 9) line 3. 
Nov 17 12:01:33 localhost sqlgrey: fatal: install_driver(Pg) failed: Can't
locate DBD/Pg.pm in @INC (@INC contains:
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.6/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.8 /usr/lib/perl5/site_perl/5.8.7
/usr/lib/perl5/site_perl/5.8.6 /usr/lib/perl5/site_perl/5.8.5
/usr/lib/perl5/site_perl
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.7/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.6/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.5/i386-linux-thread-multi
/usr/lib/perl5/vendor_perl/5.8.8 /usr/lib/perl5/vendor_perl/5.8.7
/usr/lib/perl5/vendor_perl/5.8.6 /usr/lib/perl5/vendor_perl/5.8.5
/usr/lib/perl5/vendor_perl /usr/lib/perl5/5.8.8/i386-linux-thread-multi
/usr/lib/perl5/5.8.8 .) at (eval 9) line 3. Perhaps the DBD::Pg perl module
hasn't been fully installed, or per

If there are no objections, I'll change the packaged config file to use SQLite
(and require DBD::SQLite).

That sounds like an excellent plan to me. 
Let me know when you have the new versions up and ready and I can do a final
checkover before approval. 

Hey Steven. 

Any news on an updated version to review here?

http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-3.src.rpm

This version addresses all obvious problems that I'm aware of.  I'm still not
completely sure that it works properly, but it does start up with no errors or
warnings.

I have it installed on a new mail server that is (hopefully) going live
tomorrow, so I'll know more then.  :-)

Looking at the package from comment #18:

>> 1. Might include the gpl COPYING file?
>> 
>> 2. Might include the Changelog and CONTRIB as %doc files?
>
>I'll fix that in -3.

Doesn't look like those are addressed from what I can see...

Otherwise it's looking pretty good. 

(In reply to comment #19)
> Looking at the package from comment #18:
> 
> >> 1. Might include the gpl COPYING file?
> >> 
> >> 2. Might include the Changelog and CONTRIB as %doc files?
> >
> >I'll fix that in -3.
> 
> Doesn't look like those are addressed from what I can see...

Oops, try -4.

http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-4.src.rpm

After using this for an entire day (finally), the only issue that I've noticed 
is the init script clobbering its own output.

I'll try to get -5 together later today with a fix for that.

That was easy...

http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-5.src.rpm

Looks all good to me. All the blockers I saw have been fixed... so this package
is APPROVED. 

Don't forget to close this package NEXTRELEASE once it's been imported and built. 

Do consider reviewing another waiting package to help spread out the reviewing
load. 

Imported, branches (finally) created, and builds requested.