Bug 242311

Heya -- a couple initial comments, not a review.

Overall the spec looks fairly clean, especially for a first submission :)  There
are some tweaks that could be made -- and some perl-specific bits.  Note you can
find the (draft!) perl packaging best practices at:  

    http://fedoraproject.org/wiki/PackagingDrafts/Perl

For perl buildrequires, it's preferable to specify the module needed rather than
the package containing the module (e.g. perl(Test::Pod) rather than perl-Test-Pod).

There's a perl-split which may be coming down the pike.  It's recommended to
buildrequire Test::More or ExtUtils::MakeMaker if either are required (and it
looks like the latter is).

As it's a noarch package, this line can be nixed safely:

    find %{buildroot} -type f -name '*.bs' -size 0 -exec rm -f {} \;

"chmod -R u+rwX,go+rX,go-w" works, but %{_fixperms} is so much easier on the eyes :)

%doc in %files is (by convention) placed at the top of the files list.

%{?dist} is usually left out of the changelog version-release.

%{perl_vendorlib}/Time/Duration.pm is redundant; the first line will include
%{perl_vendorlib}/Time and everything below it, as it's a directory:

   %{perl_vendorlib}/Time
   %{perl_vendorlib}/Time/Duration.pm

The spec is missing a %check section; I haven't checked Time-Duration itself but
if the distribution includes any tests it's important to run them.  e.g.

   %check
   make test

Chris, thanks for the comments and the link.

Spec file updated.

New SRPM URL:
http://marcbradshaw.co.uk/packages/SRPMS/perl-Time-Duration-1.04-3.fc6.src.rpm

New SRPM:
http://marcbradshaw.co.uk/packages/SRPMS/perl-Time-Duration-1.04-3.src.rpm

*** Bug 243733 has been marked as a duplicate of this bug. ***

Marc, I'm willing to comaintain once you get this package approved and get
sponsored (unfortunately I can't sponsor).

I can't sponsor either, but I had a look, and it looks good to me:

Review for release 3:
* RPM name is OK
* Source Time-Duration-1.04.tar.gz is the same as upstream
* Builds fine in mock
* rpmlint looks OK
* File list looks OK
* package contains tests and they run OK
* package has sane Requires and Provides

Just a suggestion, it may be nice to include the ChangeLog file in %doc

Ruben: Good point, ChangeLog should indeed be in %doc, it has been added.
SRPM: http://marcbradshaw.co.uk/packages/SRPMS/perl-Time-Duration-1.04-4.src.rpm

Jeffrey: I would be happy to have a co-maintainer. Thanks.

Marc, have you done any pre-reviews of other packages?  Basically you do
everything that you would normally do for a review except that you can't
actually approve the package.  Doing pre-reviews will show potential sponsors
that you have a good grasp of the packaging guidelines.

I do try and contribute where possible.

Updated URLs

http://marcbradshaw.co.uk/packages/perl-Time-Duration-1.04-4.src.rpm
http://marcbradshaw.co.uk/packages/perl-Time-Duration.spec

I've done a full review, here is the result:

Must Fix:
---------
* Change license field to: "GPLv2+ or Artistic 2.0"

Should Fix:
-----------
* Fedora's default %defattr is: "%defattr(-,root,root,-)
* There is a new upstream 1.06 release, this is a should Fix, as the changes
  seem pretty much uninteresting

All fixed.

http://marcbradshaw.co.uk/packages/perl-Time-Duration-1.06-1.src.rpm

Remove the OPTIMIZE from %build.

This is a noarch package, so passing CFLAGS doesn't make sense.

Approved, about the OPTIMIZE this is indeed unneeded, but not harmfull, you
could consider removing this before import.

Again, please wait with posting a CVS admin request until we're done with the
other 2 and I've sponsored you.

Thanks.
I will remove the optimize before cvs.

New Package CVS Request
=======================
Package Name: perl-Time-Duration
Short Description: rounded or exact English expression of durations
Owners: deebs
Branches: F-7
InitialCC: 
Cvsextras Commits: yes

cvs done.

Thanks to all for the review and thanks to Hans for the sponsorship.
Jeffrey, if you would still like to co-maintain this package let me know.

Package Change Request
======================
Package Name: perl-Time-Duration
New Branches: EL-4 EL-5

cvs done.

Package Change Request
======================
Package Name: perl-Time-Duration
InitialCC: perl-sig

I think I've done what you need, if not, let me know.

No, You have replaced the owner instead of adding perl-sig into CC role. Please revert ownership to mmaslano and add perl-sig with watch* rights only.

Fixed.

Package Change Request
======================
Package Name: perl-Time-Duration
New Branches: epel7
Owners: lkundrak

Unfortunately both Fedora and EL-6 maintainers are not willing to maintain EPEL packages as per http://fedoraproject.org/wiki/EPEL/ContributorStatusNo

Git done (by process-git-requests).