Bug 1079749 (perl-Test-Image-GD)

Summary: Review Request: perl-Test-Image-GD - A module for testing images using GD
Product: [Fedora] Fedora Reporter: Sven Nierlein <Sven.Nierlein>
Component: Package ReviewAssignee: Petr Šabata <psabata>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: package-review, psabata, rc040203
Target Milestone: ---Flags: psabata: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.03-3 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-06-05 20:29:18 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: 1069988, 1080201    

Description Sven Nierlein 2014-03-23 17:14:52 UTC
Spec URL: http://nierlein.com/fedora/perl-Test-Image-GD.spec
SRPM URL: http://nierlein.com/fedora/perl-Test-Image-GD-0.03-1.fc21.src.rpm
Description:
Test::Image::GD is a module for testing images using GD

Fedora Account System Username: sni

Successful koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6665562

This is my first review request, so i am asking hereby for a sponsor. 

This perl module is required to proceed in #1069988
Related review requests: #1079718, #1079732, #1079733, #1079745

Comment 1 Ralf Corsepius 2014-03-24 05:52:40 UTC
* These Requires: should be removed from the spec.
Requires:       perl(GD)
Requires:       perl(Scalar::Util)
Requires:       perl(Test::Builder)
Rpm's perl-deptracker generates these (and others) automatically.

* Some BR:s are missing. Please add:
BuildRequires:  perl(Exporter)
BuildRequires:  perl(Test::Builder::Tester)
BuildRequires:  perl(strict)
BuildRequires:  perl(warnings)

* Do you intend to support rhel < 6?
If no, you can get rid of many rpm-anacronisms inside of your spec, such as %clean, rm -rf $RPM_BUILD_ROOT, BuildRoot:..., %defattr etc.

NB: cpanspec is a tool aiming at assisting packagers. The spec files it generates are far from being "perfect" and usually require to be manually modified.

Comment 2 Sven Nierlein 2014-03-24 20:58:22 UTC
Updated spec and src rpm above.

* Removed %clean, BuildRoot and %defattr. Added missing BR:s.

Comment 3 Petr Šabata 2014-03-28 13:45:26 UTC
I see you've removed some of the old cruft Ralf pointed out but not all of it.  Since the answer to "Do you intend to support rhel < 6?" is obviously "no", drop the line 33, too.

There are still one more missing build-time dependencies you should add:
perl, used in the spec
perl(File::Spec::Functions), used in various tests

For the optional tests, you may also buildrequire:
perl(Test::Pod) >= 1.14
perl(Test::Pod::Coverage) >= 1.04

Consider using the description from the POD instead of just paraphrasing the Summary.

Package the README and Changes files as documentation, e.g. by adding the following to your %files section:
%doc Changes README

More tips:
Line 39 is not needed and may be safely dropped.
Sort your deps alphabetically; it's easier to maintain later.

Comment 4 Sven Nierlein 2014-03-28 14:24:05 UTC
Thanks.

Yes, i currently don't intend to support older releases and will focus on the
upcoming ones.

I will update the specs and src rpms and upload them over the weekend.

Comment 6 Sven Nierlein 2014-04-06 10:49:39 UTC
New source RPM: http://nierlein.com/fedora/2014-04-06/perl-Test-Image-GD-0.03-1.fc21.src.rpm

Comment 7 Petr Šabata 2014-04-08 12:50:32 UTC
Ok, this is better.  Two more things, though.

1. Can you explain the "MIDDLE DOT"s in your %description?
2. Some people care about bumping the Release with every new submission, even for reviews.  I do not, however you can't have multiple changelog entries for the same NVR.  Either merge them into one or bump the Release.

Comment 8 Sven Nierlein 2014-04-08 12:54:18 UTC
There was a complain about the NEVR in #1079733 already, so i will bump the number and upload new files.

Comment 9 Sven Nierlein 2014-04-21 13:25:17 UTC
New spec file is here:
http://nierlein.com/fedora/2014-04-21/perl-Test-Image-GD.spec

The middle dots were leftofers from copy & past, they are now removed. I also
increased the release number.

Comment 10 Sven Nierlein 2014-05-20 16:53:08 UTC
perl compat requires added and updated BRs and requires according to cpanspec.

new files are here:
http://nierlein.com/fedora/2014-05-20/perl-Test-Image-GD.spec
http://nierlein.com/fedora/2014-05-20/perl-Test-Image-GD-0.03-3.fc21.src.rpm

Comment 11 Petr Šabata 2014-05-21 07:57:09 UTC
Ok, I'm going to approve this.

However, you've just added yet another BR entry for File::Spec while, in fact, you don't need any; the code only requires File::Spec::Functions (which you already list), despite what Makefile.PL says.
Drop both of the File::Spec BRs before pushing.

I suggest sorting the dependency list so you don't overlook things like this next timne.

Comment 12 Sven Nierlein 2014-05-24 19:53:19 UTC
New Package SCM Request
=======================
Package Name: perl-Test-Image-GD
Short Description: Module for testing images using GD
Owners: sni
Branches: f21
InitialCC: perl-sig

Comment 13 Gwyn Ciesla 2014-05-27 12:52:07 UTC
Git done (by process-git-requests).

Comment 14 Sven Nierlein 2014-06-05 20:29:18 UTC
build and upload completed