Bug 1079745 (perl-Excel-Template) - Review Request: perl-Excel-Template - Create Excel files from templates
Summary: Review Request: perl-Excel-Template - Create Excel files from templates
Keywords:
Status: CLOSED RAWHIDE
Alias: perl-Excel-Template
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: naemon perl-Excel-Template-Plus
TreeView+ depends on / blocked
 
Reported: 2014-03-23 16:40 UTC by Sven Nierlein
Modified: 2014-09-08 17:34 UTC (History)
3 users (show)

Fixed In Version: 0.34-5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-09-08 17:34:36 UTC
rc040203: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sven Nierlein 2014-03-23 16:40:07 UTC
Spec URL: http://nierlein.com/fedora/perl-Excel-Template.spec
SRPM URL: http://nierlein.com/fedora/perl-Excel-Template-0.34-1.fc21.src.rpm
Description:
Excel::Template creates Excel file from templates.
Fedora Account System Username: sni

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

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

Comment 1 Ralf Corsepius 2014-03-24 08:10:56 UTC
* 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

Comment 2 Sven Nierlein 2014-03-24 20:43:38 UTC
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.

Comment 3 Ralf Corsepius 2014-03-25 05:17:59 UTC
(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.

Comment 4 Sven Nierlein 2014-04-06 10:57:20 UTC
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

Comment 5 Sven Nierlein 2014-04-21 13:49:41 UTC
new spec file
http://nierlein.com/fedora/2014-04-21/perl-Excel-Template.spec

the only thing i changed is the release number.

Comment 6 Sven Nierlein 2014-05-20 14:17:45 UTC
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

Comment 7 Ralf Corsepius 2014-05-21 00:18:36 UTC
(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.

Comment 8 Sven Nierlein 2014-05-24 20:22:46 UTC
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?

Comment 9 Ralf Corsepius 2014-05-27 06:18:20 UTC
(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.

Comment 10 Sven Nierlein 2014-05-27 09:02:30 UTC
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?

Comment 11 Ralf Corsepius 2014-07-28 05:15:42 UTC
(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?

Comment 13 Ralf Corsepius 2014-08-19 17:07:18 UTC
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.

Comment 14 Ralf Corsepius 2014-09-05 06:38:21 UTC
Ping, Sven?

Comment 15 Sven Nierlein 2014-09-05 07:21:36 UTC
Sorry, quite busy lately. Hopefully i can continue on this during the weekend.

Comment 16 Sven Nierlein 2014-09-07 17:57:49 UTC
New Package SCM Request
=======================
Package Name: perl-Excel-Template
Short Description: Create Excel files from templates
Owners: sni
Branches: f21
InitialCC: perl-sig

Comment 17 Sven Nierlein 2014-09-08 10:58:10 UTC
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.

Comment 18 Gwyn Ciesla 2014-09-08 11:57:22 UTC
WARNING: SCM request was not the last comment.
WARNING: fedora-review flag not set to '+'

Comment 19 Gwyn Ciesla 2014-09-08 13:10:36 UTC
Git done (by process-git-requests).

Comment 20 Sven Nierlein 2014-09-08 17:34:36 UTC
build and upload completed


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