Bug 242311 - Review Request: perl-Time-Duration - rounded or exact English expression of durations
Summary: Review Request: perl-Time-Duration - rounded or exact English expression of d...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
: 243733 (view as bug list)
Depends On:
Blocks: 242310
TreeView+ depends on / blocked
 
Reported: 2007-06-03 07:44 UTC by Marc Bradshaw
Modified: 2014-02-21 15:17 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-09-24 23:30:01 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Marc Bradshaw 2007-06-03 07:44:47 UTC
First package, sponsor required.

Spec URL: http://marcbradshaw.co.uk/packages/SPECS/perl-Time-Duration.spec

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

Description:

This module provides functions for expressing durations in rounded or exact
terms.

Comment 1 Chris Weyl 2007-06-04 23:06:11 UTC
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


Comment 2 Marc Bradshaw 2007-06-05 01:42:05 UTC
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

Comment 4 Jeffrey C. Ollie 2007-07-01 20:44:50 UTC
*** Bug 243733 has been marked as a duplicate of this bug. ***

Comment 5 Jeffrey C. Ollie 2007-07-01 20:52:36 UTC
Marc, I'm willing to comaintain once you get this package approved and get
sponsored (unfortunately I can't sponsor).

Comment 6 Ruben Kerkhof 2007-07-01 21:02:36 UTC
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

Comment 7 Marc Bradshaw 2007-07-02 06:54:43 UTC
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.

Comment 8 Jeffrey C. Ollie 2007-07-12 15:02:37 UTC
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.

Comment 9 Marc Bradshaw 2007-07-31 00:35:27 UTC
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

Comment 10 Hans de Goede 2007-09-18 19:39:37 UTC
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


Comment 11 Marc Bradshaw 2007-09-18 23:01:55 UTC
All fixed.

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


Comment 12 Ralf Corsepius 2007-09-19 03:21:29 UTC
Remove the OPTIMIZE from %build.

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


Comment 13 Hans de Goede 2007-09-19 12:28:38 UTC
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.

Comment 14 Marc Bradshaw 2007-09-19 23:39:56 UTC
Thanks.
I will remove the optimize before cvs.

Comment 15 Marc Bradshaw 2007-09-23 11:37:00 UTC
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

Comment 16 Kevin Fenzi 2007-09-24 16:12:37 UTC
cvs done.

Comment 17 Marc Bradshaw 2007-09-24 23:30:01 UTC
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.


Comment 18 Marc Bradshaw 2008-08-19 00:51:14 UTC
Package Change Request
======================
Package Name: perl-Time-Duration
New Branches: EL-4 EL-5

Comment 19 Toshio Kuratomi 2008-08-23 19:09:07 UTC
cvs done.

Comment 20 Marcela Mašláňová 2012-01-11 07:07:30 UTC
Package Change Request
======================
Package Name: perl-Time-Duration
InitialCC: perl-sig

Comment 21 Gwyn Ciesla 2012-01-11 13:09:42 UTC
I think I've done what you need, if not, let me know.

Comment 22 Petr Pisar 2012-01-11 16:14:34 UTC
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.

Comment 23 Gwyn Ciesla 2012-01-11 16:36:26 UTC
Fixed.

Comment 24 Lubomir Rintel 2014-02-21 14:57:30 UTC
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

Comment 25 Gwyn Ciesla 2014-02-21 15:17:35 UTC
Git done (by process-git-requests).


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