* These Requires: should be removed from the *.spec:
Requires:       perl(XML::Parser)
Requires:       perl(IO::Scalar)
Requires:       perl(File::Basename)
Requires:       perl(Spreadsheet::WriteExcel::Utility)
Rpm automatically detects them, causing the manually specified ones to appear twice on the resulting binary rpms.


* BuildRequires:  perl(Spreadsheet::WriteExcel)
should match the run-time requirement:
Requires:       perl(Spreadsheet::WriteExcel) >= 0.42

i.e. change
 BuildRequires:  perl(Spreadsheet::WriteExcel)
into
 BuildRequires:  perl(Spreadsheet::WriteExcel) >= 0.42


* I'd recommend to "chmod"s the source-files in %build ("Fix the sources") 
instead to "chmod" them after installation in %install ("Workaround the bug").


* The package ships a LICENCE file
=> MUST add LICENCE to %doc

Spec file and src rpm from above updated, new build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6669658

rpmlint now complains about:
perl-Excel-Template.noarch: E: incorrect-fsf-address /usr/share/doc/perl-Excel-Template/LICENSE

but according too google, this has to be fixed upstream.

(In reply to Sven Nierlein from comment #2)
> Spec file and src rpm from above updated,
Please increment NEVR and provide a corresponding changelog within the spec file each time you change something in a spec file.

Not doing so forces reviewers to hunt down your files and verify whether they are looking at the correct files.

> rpmlint now complains about:
> perl-Excel-Template.noarch: E: incorrect-fsf-address
> /usr/share/doc/perl-Excel-Template/LICENSE
> 
> but according too google, this has to be fixed upstream.
Correct, this is an rpmlint sillyness, which is safe to be ignored.

updated spec file according to comments of the related bugs and
uploaded new files (this time into a new folder):

http://nierlein.com/fedora/2014-04-06/perl-Excel-Template.spec
http://nierlein.com/fedora/2014-04-06/perl-Excel-Template-0.34-1.fc21.src.rpm

new spec file
http://nierlein.com/fedora/2014-04-21/perl-Excel-Template.spec

the only thing i changed is the release number.

cpan ticket has been opened to change the fsf address.

Besides that i added the compat perl requires and changed the BRs and requires according to cpanspec:

http://nierlein.com/fedora/2014-05-20/perl-Excel-Template.spec
http://nierlein.com/fedora/2014-05-20/perl-Excel-Template-0.34-3.fc21.src.rpm

(In reply to Sven Nierlein from comment #6)
> cpan ticket has been opened to change the fsf address.
As I wrote above, this is an rpmlint sillyness, which is safe to be ignored.

> Besides that i added the compat perl requires and changed the BRs and
> requires according to cpanspec:
You still have got them wrong.

The explicit R: mentioned in the first section of
https://bugzilla.redhat.com/show_bug.cgi?id=1079745#c1
still MUST be removed.

Ok, now i understand. I've corrected this.

There is one "Requires: perl(Test::More)" left which i am unsure about. The
Makefile.PL mentions this as requires (and as test_requires) but its obviously a mistake and i informed upstream about it. Would you remove it from the spec file or keep it?

(In reply to Sven Nierlein from comment #8)
> Ok, now i understand. I've corrected this.
Updated src.rpm, please.
 
> There is one "Requires: perl(Test::More)" left which i am unsure about.
You need to distinguish run-time requirements (Requires:) from build-time requirements. 

In almost all cases, Test::More is a build-time-only requirement, which is required to exercise a perl module's tests (commonly found under t/ in a source-tree), but is not required at run-time (commonly found under lib/ in a source-tree).

FYI: Unlike other reviewers I do not provide you with spec-patches for didactical reasons, because I want you to understand what you are doing and do not want you "blindly copy and paste" others' works. Sorry, if this feels inconvenient to you. I am doing so for your own sake.

thanks, i am not expecting a patch file and i am familiar with perl modules, i
was just wondering how to deal with such situations when there is a useless
requirement in the Makefile.PL.

The Makefile.PL contains:
test_requires 'Test::More' => '0.47';
requires 'Test::More'      => 0;

Test::More is only used for tests below t/. Would we still add Test::More as
runtime requirement because the Makefile.PL says so?

(In reply to Sven Nierlein from comment #10)

> Test::More is only used for tests below t/. Would we still add Test::More as
> runtime requirement because the Makefile.PL says so?
Probably no. This looks like an upstream bug to me.

Any update on this package?

Updated package is here:
http://nierlein.com/fedora/2014-07-28/perl-Excel-Template.spec
http://nierlein.com/fedora/2014-07-28/perl-Excel-Template-0.34-4.fc22.src.rpm

Sorry, it took so long to return to this review. I have been on vacation which had caused this review to drop off my personal radar.

APPROVED.

One remark: The "Requires: perl(Spreadsheet::WriteExcel) >= 0.42" seems superflous to me. At least I can't spot any reference to it.
Also, this requires seems redundant to 
"Requires: perl(Spreadsheet::WriteExcel::Utility)"
which rpm's deptracking adds automatically.

Ping, Sven?

Sorry, quite busy lately. Hopefully i can continue on this during the weekend.

New Package SCM Request
=======================
Package Name: perl-Excel-Template
Short Description: Create Excel files from templates
Owners: sni
Branches: f21
InitialCC: perl-sig

just realized you didn't actually set the review flag? Should i implement your remarks first? I'd done that with the final pkg otherwise.

WARNING: SCM request was not the last comment.
WARNING: fedora-review flag not set to '+'

Git done (by process-git-requests).

build and upload completed