Bug 189188 - Review Request: sqlgrey - postfix grey-listing policy service
Review Request: sqlgrey - postfix grey-listing policy service
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-17 22:27 EDT by Warren Togami
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-22 19:54:41 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Warren Togami 2006-04-17 22:27:33 EDT
Spec URL: http://people.redhat.com/wtogami/temp/sqlgrey.spec
SRPM URL: http://people.redhat.com/wtogami/temp/sqlgrey-1.7.3-2.src.rpm
Description: 
Very easy to setup and use greylisting policy service for postfix MTA.

This package isn't done, it requires some fixing:
- convert spec file into UTF-8
- fix up the initrd script
Comment 1 Michael J Knox 2006-07-26 22:19:49 EDT
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? 
Comment 2 Steven Pritchard 2006-08-29 15:20:14 EDT
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?
Comment 3 Warren Togami 2006-08-29 15:26:10 EDT
Thanks Steven, I haven't been able to prioritize working on this.  Would you
like to take over ownership?
Comment 4 Steven Pritchard 2006-08-29 19:19:38 EDT
(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?
Comment 5 Warren Togami 2006-08-29 19:46:23 EDT
Oops, yes. =)
Comment 6 Steven Pritchard 2006-08-31 12:57:32 EDT
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.  :-)
Comment 7 Michael J Knox 2006-09-16 16:27:06 EDT
Sorry. Due to my stepping out for a while, I am unable to complete this review. 
Comment 8 Kevin Fenzi 2006-09-17 20:03:12 EDT
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? 
Comment 9 Steven Pritchard 2006-09-18 17:15:33 EDT
(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.
Comment 10 Chris Grau 2006-09-28 20:17:49 EDT
(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?
Comment 11 Kevin Fenzi 2006-10-07 23:37:30 EDT
Ping Steven. Any chance to look at this again? 
Comment 12 Steven Pritchard 2006-10-08 05:47:55 EDT
I'm planning to work on it again on Tuesday (10/10).
Comment 13 Kevin Fenzi 2006-11-11 20:35:56 EST
Hey Steven. Any news on this package? 
Comment 14 Steven Pritchard 2006-11-11 21:11:48 EST
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.  :-)
Comment 15 Steven Pritchard 2006-11-17 13:08:40 EST
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).
Comment 16 Kevin Fenzi 2006-11-17 15:37:47 EST
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. 

Comment 17 Kevin Fenzi 2006-12-03 18:01:07 EST
Hey Steven. 

Any news on an updated version to review here?
Comment 18 Steven Pritchard 2006-12-12 17:00:56 EST
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.  :-)
Comment 19 Kevin Fenzi 2006-12-13 00:56:21 EST
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. 
Comment 20 Steven Pritchard 2006-12-13 11:05:15 EST
(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
Comment 21 Steven Pritchard 2006-12-14 14:26:41 EST
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.
Comment 22 Steven Pritchard 2006-12-14 15:06:01 EST
That was easy...

http://ftp.kspei.com/pub/steve/rpms/sqlgrey-1.7.4-5.src.rpm
Comment 23 Kevin Fenzi 2006-12-14 22:27:31 EST
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. 
Comment 24 Steven Pritchard 2006-12-22 19:54:41 EST
Imported, branches (finally) created, and builds requested.

Note You need to log in before you can comment on or make changes to this bug.