Bug 890491 - Review Request: perl-Mail-Procmail - Procmail-like facility for creating easy mail filters
Summary: Review Request: perl-Mail-Procmail - Procmail-like facility for creating easy...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Pisar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-12-27 09:14 UTC by Steven Roberts
Modified: 2013-03-12 17:42 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-02-21 05:42:47 UTC
Type: ---
Embargoed:
ppisar: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Steven Roberts 2012-12-27 09:14:24 UTC
Spec URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail.spec
SRPM URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail-1.08-1.el6.src.rpm
Description: procmail is a great mail filter program, but it has weird recipe format. It's pattern matching capabilities are basic and often insufficient. I wanted something flexible whereby I could filter my mail using the power of Perl.
Fedora Account System Username: strobert

Koji scratch build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=4818076

Comment 1 Trond H. Amundsen 2012-12-27 12:31:09 UTC
Not an official review, I just have some comments on the spec file:

* These are only needed if rpmbuild doesn't find those dependencies automatically:

  Requires:       perl(LockFile::Simple)
  Requires:       perl(Mail::Internet)
  Requires:       perl(Test::More)

* Unless EPEL5 is one of the intended targets you can (and should) omit the BuildRoot and the whole %clean section

* The description is weird... "I wanted something etc." should be rephrased

* You don't need a macro for %{__perl} (I think). I'm not 100% sure of this one.

* Spec files don't belong in %doc, at least not spec files for other packages

* You should use %perl_default_filter, see URL: https://fedoraproject.org/wiki/Perl_default_filter

-trond

Comment 2 Steven Roberts 2012-12-27 21:15:15 UTC
What I get for trying to squeeze this in before I went to bed last night :).  Obviously was a bit too tired.  The wrong version got sync'd up which addresses some of the items...  I was fighting my local fedora dev env last night (mock stopped working after latest updates) so was shuffling files around.

- I'll drop the requires.  Thought I had.

- EPEL5 is a planned target.  we have been using a local generated rpm for a bit know, but thinking getting it upstream would be a good thing.

- that description is straight from the perl module's author when describing his module.  I'll reqord it a bit to make a bit more sense as a package description.

- %{__perl} is what cpanspec spits out and what the MODEUL_COMPAT define listed uses at:
https://fedoraproject.org/wiki/Packaging:Perl?rd=Packaging/Perl#Versioned_MODULE_COMPAT_Requires
so got the impression that was normal best practice.

- I'll have to see where cpanspec grabbed the extra spec files from.  that is really weird.  I'm actually working with the cpanspec author at the moment on a new release so sounds like a bug to get fixed :)

- thank you for the pointer to the %perl_default_filter.  the link to it (https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Perl) from the perl packaging page is a little hidden.  I'll see about tweaking the wiki pages to point it out more.

Comment 3 Steven Roberts 2012-12-31 03:12:54 UTC
new spec,srpm uploaded
Spec URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail.spec
SRPM URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail-1.08-2.el6.src.rpm

Did the adjustments mentioned above.  I figured out where those spec files came from, they are actually bundled in the tarball from upstream (including the ones for the other packages).

Comment 4 Petr Pisar 2013-01-21 15:25:00 UTC
URL and Source0 are usable. Ok.
Source file is original (SHA-256: 09ff5367e83a752f1279f31d8ac05b62360222bb438dc58a71f7e39506298c70). Ok.
Summary verified from lib/Mail/Procmail.pm. Ok.
Description verified from lib/Mail/Procmail.pm. Ok.
License verified from lib/Mail/Procmail.pm. Ok.
No XS code, noarch BuildArch is Ok.

TODO: Remove BuildRoot definition. It's not needed any more.
TODO: Remove explicit build root cleaning, it's not needed any more.
TODO: Remove deleting empty directories in %install section. Modern ExtUtils::MakeMaker does not leave empty directories.
TODO: Remove explicit %clean, it's not needed any more.
TODO: Remove explicit defattr in %files section. It's not needed any more.

TODO: Build-require `perl(constant)' (lib/Mail/Procmail.pm:155).
TODO: Build-require `perl(Exporter)' (lib/Mail/Procmail.pm:162).

FIX: Build-require `perl(Carp)' (lib/Mail/Procmail.pm:189).

IO::File is not needed on current perl. Ok.

All tests pass. Ok.

$ rpmlint perl-Mail-Procmail.spec ../SRPMS/perl-Mail-Procmail-1.08-2.fc19.src.rpm ../RPMS/noarch/perl-Mail-Procmail-1.08-2.fc19.noarch.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/noarch/perl-Mail-Procmail-1.08-2.fc19.noarch.rpm                                                                  drwxr-xr-x    2 root    root                        0 Jan 21 16:03 /usr/share/doc/perl-Mail-Procmail-1.08
-rw-r--r--    1 root    root                     2339 Sep 19  2004 /usr/share/doc/perl-Mail-Procmail-1.08/CHANGES
-rw-r--r--    1 root    root                     3729 Sep 19  2004 /usr/share/doc/perl-Mail-Procmail-1.08/README
-rw-r--r--    1 root    root                     6956 Jan 21 16:03 /usr/share/man/man3/Mail::Procmail.3pm.gz
drwxr-xr-x    2 root    root                        0 Jan 21 16:03 /usr/share/perl5/vendor_perl/Mail
-rw-r--r--    1 root    root                    22214 Sep 19  2004 /usr/share/perl5/vendor_perl/Mail/Procmail.pm
File permissions and layout are Ok.

$ rpm -q --requires -p ../RPMS/noarch/perl-Mail-Procmail-1.08-2.fc19.noarch.rpm |sort |uniq -c
      1 perl >= 0:5.005
      1 perl(Carp)
      1 perl(constant)
      1 perl(Exporter)
      1 perl(Fcntl)
      1 perl(LockFile::Simple)
      1 perl(Mail::Internet)
      1 perl(:MODULE_COMPAT_5.16.2)
      1 perl(strict)
      1 perl(Sys::Hostname)
      1 perl(vars)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/noarch/perl-Mail-Procmail-1.08-2.fc19.noarch.rpm |sort |uniq -c
      1 perl(Mail::Procmail) = 1.08
      1 perl-Mail-Procmail = 1.08-2.fc19
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/noarch/perl-Mail-Procmail-1.08-2.fc19.noarch.rpm 
Binary dependencies resolvable. Ok.

Package builds in F19 (http://koji.fedoraproject.org/koji/taskinfo?taskID=4889603). ???

Otherwise package is in line with Fedora and Perl packaging guidelines.

Please correct all `FIX' issues, consider fixing `TODO' items, and provide new spec file.
Resolution: NOT approved.

Comment 5 Steven Roberts 2013-01-24 04:01:27 UTC
thank you for the review.  what are you using? it looks different than at last the version of fedora-review I have been running.  I would like to prescreen future packages to save folks some time.

EPEL5 is a target so the following are still required:
TODO: Remove BuildRoot definition. It's not needed any more.
TODO: Remove explicit build root cleaning, it's not needed any more.
TODO: Remove deleting empty directories in %install section. Modern ExtUtils::MakeMaker does not leave empty directories.
TODO: Remove explicit %clean, it's not needed any more.
TODO: Remove explicit defattr in %files section. It's not needed any more.

Fixed these:
TODO: Build-require `perl(constant)' (lib/Mail/Procmail.pm:155).
TODO: Build-require `perl(Exporter)' (lib/Mail/Procmail.pm:162).
FIX: Build-require `perl(Carp)' (lib/Mail/Procmail.pm:189).

I'm working with the spanspec author on a new version, I'll see about why those weren't caught by it.

new spec,srpm uploaded
Spec URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail.spec
SRPM URL: http://rpm.ysl.org/fedora-review/perl-Mail-Procmail-1.08-3.src.rpm

Comment 6 Petr Pisar 2013-01-24 08:38:25 UTC
Spec file changes:
--- perl-Mail-Procmail.spec.old 2012-12-31 04:08:04.000000000 +0100
+++ perl-Mail-Procmail.spec     2013-01-24 04:54:40.000000000 +0100
@@ -1,6 +1,6 @@
 Name:           perl-Mail-Procmail
 Version:        1.08
-Release:        2%{?dist}
+Release:        3%{?dist}
 Summary:        Procmail-like facility for creating easy mail filters
 License:        GPL+ or Artistic
 Group:          Development/Libraries
@@ -12,6 +12,9 @@
 BuildRequires:  perl(LockFile::Simple)
 BuildRequires:  perl(Mail::Internet)
 BuildRequires:  perl(Test::More)
+BuildRequires:  perl(Carp)
+BuildRequires:  perl(constant)
+BuildRequires:  perl(Exporter)
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

 %{?perl_default_filter}
@@ -52,6 +55,9 @@
 %{_mandir}/man3/*

 %changelog
+* Wed Jan 23 2013 Steven D. Roberts <strobert> 1.08-3
+- added some missing buildreqs
+
 * Sun Dec 30 2012 Steven D. Roberts <strobert> 1.08-2
 - spec cleanup.


> TODO: Remove BuildRoot definition. It's not needed any more.
> TODO: Remove explicit build root cleaning, it's not needed any more.
> TODO: Remove deleting empty directories in %install section. Modern ExtUtils::MakeMaker does not leave empty directories.
> TODO: Remove explicit %clean, it's not needed any more.
> TODO: Remove explicit defattr in %files section. It's not needed any more.
Not addressed.

> TODO: Build-require `perl(constant)' (lib/Mail/Procmail.pm:155).
+BuildRequires:  perl(constant)
Ok.

> TODO: Build-require `perl(Exporter)' (lib/Mail/Procmail.pm:162).
+BuildRequires:  perl(Exporter)
Ok.

> FIX: Build-require `perl(Carp)' (lib/Mail/Procmail.pm:189).
+BuildRequires:  perl(Carp)
Ok.

$ rpmlint perl-Mail-Procmail.spec ../SRPMS/perl-Mail-Procmail-1.08-3.fc19.src.rpm ../RPMS/noarch/perl-Mail-Procmail-1.08-3.fc19.noarch.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is Ok.

Package builds in F19 (http://koji.fedoraproject.org/koji/taskinfo?taskID=4898744). Ok

Package is good.
Resolution: Package APPROVED.

Comment 7 Petr Pisar 2013-01-24 08:53:43 UTC
(In reply to comment #5)
> thank you for the review.  what are you using? it looks different than at
> last the version of fedora-review I have been running.  I would like to
> prescreen future packages to save folks some time.
> 
No tool, manually. We have plan to write the test as plug-in for fedora-review, but fedora-review is broken now.

> EPEL5 is a target so the following are still required:
> > TODO: Remove explicit defattr in %files section. It's not needed any more.
This is not needed since rpm 4.4 which presents in RHEL-5 <https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions>.


> Fixed these:
> TODO: Build-require `perl(constant)' (lib/Mail/Procmail.pm:155).
> TODO: Build-require `perl(Exporter)' (lib/Mail/Procmail.pm:162).
> FIX: Build-require `perl(Carp)' (lib/Mail/Procmail.pm:189).
> 
> I'm working with the spanspec author on a new version, I'll see about why
> those weren't caught by it.

cpanspec investigates META.yml only. And even so it removes modules which are bundled with perl.

Comment 8 Steven Roberts 2013-01-24 09:15:47 UTC
ah.  unfortunate about fedora-review.  

good catch on the defattr.  one of my biggest stumbles has been finding all of the old items I used to do in the RH7.3 days (which we were on for WAY too long).  I'll drop that before I do the official builds

good to know on cpanspec.  we (the cpanspec author and a couple of us who expressed interest in it recently) have been discussing how it find buildreqs, so sounds like a good thing to look into further.

Comment 9 Steven Roberts 2013-02-02 02:54:16 UTC
New Package SCM Request
=======================
Package Name: perl-Mail-Procmail
Short Description: Procmail-like facility for creating easy mail filters
Owners: strobert
Branches: f17 f18 el5 el6
InitialCC: perl-sig

Comment 10 Gwyn Ciesla 2013-02-04 14:03:03 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2013-02-10 01:03:17 UTC
perl-Mail-Procmail-1.08-4.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/perl-Mail-Procmail-1.08-4.el5

Comment 12 Fedora Update System 2013-02-10 01:09:06 UTC
perl-Mail-Procmail-1.08-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-Mail-Procmail-1.08-4.fc17

Comment 13 Fedora Update System 2013-02-10 01:09:56 UTC
perl-Mail-Procmail-1.08-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/perl-Mail-Procmail-1.08-4.fc18

Comment 14 Fedora Update System 2013-02-10 01:13:27 UTC
perl-Mail-Procmail-1.08-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-Mail-Procmail-1.08-4.el6

Comment 15 Fedora Update System 2013-02-10 19:04:03 UTC
perl-Mail-Procmail-1.08-4.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 16 Fedora Update System 2013-02-21 05:42:49 UTC
perl-Mail-Procmail-1.08-4.fc18 has been pushed to the Fedora 18 stable repository.

Comment 17 Fedora Update System 2013-02-21 05:48:38 UTC
perl-Mail-Procmail-1.08-4.fc17 has been pushed to the Fedora 17 stable repository.

Comment 18 Fedora Update System 2013-03-12 17:42:01 UTC
perl-Mail-Procmail-1.08-4.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 19 Fedora Update System 2013-03-12 17:42:33 UTC
perl-Mail-Procmail-1.08-4.el6 has been pushed to the Fedora EPEL 6 stable repository.


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