Bug 189188
Summary: | Review Request: sqlgrey - postfix grey-listing policy service | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Warren Togami <wtogami> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | chris, steve |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-12-23 00:54:41 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Warren Togami
2006-04-18 02:27:33 UTC
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. |