Bug 242311 - Review Request: perl-Time-Duration - rounded or exact English expression of durations
Review Request: perl-Time-Duration - rounded or exact English expression of d...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
: 243733 (view as bug list)
Depends On:
Blocks: 242310
  Show dependency treegraph
 
Reported: 2007-06-03 03:44 EDT by Marc Bradshaw
Modified: 2014-02-21 10:17 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-24 19:30:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marc Bradshaw 2007-06-03 03:44:47 EDT
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 19:06:11 EDT
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-04 21:42:05 EDT
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 16:44:50 EDT
*** Bug 243733 has been marked as a duplicate of this bug. ***
Comment 5 Jeffrey C. Ollie 2007-07-01 16:52:36 EDT
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 17:02:36 EDT
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 02:54:43 EDT
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 11:02:37 EDT
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-30 20:35:27 EDT
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 15:39:37 EDT
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 19:01:55 EDT
All fixed.

http://marcbradshaw.co.uk/packages/perl-Time-Duration-1.06-1.src.rpm
Comment 12 Ralf Corsepius 2007-09-18 23:21:29 EDT
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 08:28:38 EDT
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 19:39:56 EDT
Thanks.
I will remove the optimize before cvs.
Comment 15 Marc Bradshaw 2007-09-23 07:37:00 EDT
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 12:12:37 EDT
cvs done.
Comment 17 Marc Bradshaw 2007-09-24 19:30:01 EDT
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-18 20:51:14 EDT
Package Change Request
======================
Package Name: perl-Time-Duration
New Branches: EL-4 EL-5
Comment 19 Toshio Kuratomi 2008-08-23 15:09:07 EDT
cvs done.
Comment 20 Marcela Mašláňová 2012-01-11 02:07:30 EST
Package Change Request
======================
Package Name: perl-Time-Duration
InitialCC: perl-sig
Comment 21 Jon Ciesla 2012-01-11 08:09:42 EST
I think I've done what you need, if not, let me know.
Comment 22 Petr Pisar 2012-01-11 11:14:34 EST
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 Jon Ciesla 2012-01-11 11:36:26 EST
Fixed.
Comment 24 Lubomir Rintel 2014-02-21 09:57:30 EST
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 Jon Ciesla 2014-02-21 10:17:35 EST
Git done (by process-git-requests).

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