Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 773502 - Review Request: perl-Spreadsheet-read - universal API to read any spreadsheet
Summary: Review Request: perl-Spreadsheet-read - universal API to read any spreadsheet
Keywords:
Status: CLOSED INSUFFICIENT_DATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW 754678
TreeView+ depends on / blocked
 
Reported: 2012-01-12 01:05 UTC by Mark Goodwin
Modified: 2015-08-21 09:38 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-08-21 09:38:53 UTC


Attachments (Terms of Use)

Description Mark Goodwin 2012-01-12 01:05:15 UTC
Spec URL: http://people.redhat.com/mgoodwin/cpan/perl-Spreadsheet-read/perl-Spreadsheet-Read.spec

SRPM URL: http://people.redhat.com/mgoodwin/cpan/perl-Spreadsheet-read/perl-Spreadsheet-Read-0.45-1.fc15.src.rpm

Description: Spreadsheet::Read tries to transparently read *any* spreadsheet
and return its content in a universal manner independent of the parsing
module that does the actual spreadsheet scanning. It supports spreadsheets
handled by Text::CSV_XS, Spreadsheet::ParseExcel, Spreadsheet::XLSX and Spreadsheet::ReadSXC (note: XLSX and ReadSXC are not yet in Fedora. Depending
how this review request goes, I can add them too).

The cpanspec utility was used to download the tarball from cpan and
generate the spec. This request to add Spreadsheet:read is to satisfy
a missing dependency for pcp-import-sheet2pcp, the new version of which
is currently in updates-testing (see BZ 754678).

Have built this for {el4,el5,el6,f15,f16,f17}-candidate and tested on f15.

Comment 1 Michael Schwendt 2012-01-12 10:16:47 UTC
[Although I know a bit of Perl, I'm not a Perl packager, so I'm not up-to-date on Fedora Perl Packaging].


* A plain src.rpm rebuild is interactive here:

$ rpmbuild --rebuild perl-Spreadsheet-Read-0.45-1.fc15.src.rpm
[...]
+ cd Spreadsheet-Read-0.45
+ LANG=C
+ export LANG
+ unset DISPLAY
+ /usr/bin/perl Makefile.PL INSTALLDIRS=vendor
Do you want to install 'xlscat' (Convert Spreadsheet to plain text or CSV) ?  [n] 


That's a blocker. Several more setup questions are asked. This ought to be automatic with a default build config being applied.



* There's a test-suite included, suitable for a %check section. "make test" does something, but requires _at least_ perl-Test-Simple perl-Test-NoWarnings and then still warns about further build requirements as well as skipped tests.

Comment 2 Mark Goodwin 2012-01-12 21:24:05 UTC
(In reply to comment #1)

thanks for looking at this

> [Although I know a bit of Perl, I'm not a Perl packager, so I'm not up-to-date
> on Fedora Perl Packaging].

I'm not either - this probably should be reviewed by someone who is.

> * A plain src.rpm rebuild is interactive here:
> ..
> + /usr/bin/perl Makefile.PL INSTALLDIRS=vendor
> Do you want to install 'xlscat' (Convert Spreadsheet to plain text or CSV) ? 
> [n] 
> 
> That's a blocker. Several more setup questions are asked. This ought to be
> automatic with a default build config being applied.

My koji builds didn't stall or ask any questions - they just worked,
presumably using default answers. Reading Makefile.PL, it will not
ask questions if AUTOMATED_TESTING=1 in the environment, so I've made
that change in the spec (see diff below).
 
> * There's a test-suite included, suitable for a %check section. "make test"
> does something, but requires _at least_ perl-Test-Simple perl-Test-NoWarnings
> and then still warns about further build requirements as well as skipped tests.

ok, I added a few more build dependencies for the testing and added a %check
section. Seems to work, and tolerate any missing spreadsheet parsers. I've
copied the new spec and srpm up to the same location. Here's the diff :

--- old/perl-Spreadsheet-Read.spec	2012-01-11 19:47:36.884021000 -0500
+++ perl-Spreadsheet-Read.spec	2012-01-12 16:16:58.166961000 -0500
@@ -11,8 +11,11 @@
 BuildRequires:  perl >= 0:5.006
 BuildRequires:  perl(ExtUtils::MakeMaker)
 BuildRequires:  perl(IO::Scalar)
+BuildRequires:  perl(Test::NoWarnings)
+BuildRequires:  perl(Test::More) >= 0.98
 
 Requires:       perl(IO::Scalar)
+Requires:	perl(Test::More) >= 0.98
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))
 
 %description
@@ -24,7 +27,7 @@
 %setup -q -n Spreadsheet-Read-%{version}
 
 %build
-%{__perl} Makefile.PL INSTALLDIRS=vendor
+AUTOMATED_TESTING=1 %{__perl} Makefile.PL INSTALLDIRS=vendor
 make %{?_smp_mflags}
 
 %install
@@ -37,6 +40,9 @@
 
 %{_fixperms} $RPM_BUILD_ROOT/*
 
+%check
+make test
+
 %clean
 rm -rf $RPM_BUILD_ROOT

Comment 3 Michael Schwendt 2012-01-15 22:24:47 UTC
> AUTOMATED_TESTING=1

Confirmed. No questions about the example scripts are asked anymore.


>  Requires:       perl(IO::Scalar)
> +Requires: perl(Test::More) >= 0.98

http://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Even if it may seem that it is specific to libraries only, it covers explicit Requires in general. Comments in the spec file, which explain the explicit Requires, are good packaging practice.


* Please keep the spec %changelog accurate. Your updates to the spec invalidated the only %changelog entry about cpanspec usage.


* A few more packages for the tests are available in Fedora already, btw, just in case you consider the tests worthwhile:

  perl-Text-CSV
  perl-Text-CSV_XS
  perl-Test-Pod
  perl-Test-Pod-Coverage
  perl-Spreadsheet-ParseExcel

t/00_pod.t ..... ok   
t/01_pod.t ..... ok   
t/10_basics.t .. ok     
t/11_call.t .... ok     
t/20_csv.t ..... # Parser: Text::CSV_XS-0.82
t/20_csv.t ..... ok     
t/21_csv.t ..... ok    
t/22_csv.t ..... ok   
t/23_csv.t ..... ok     
t/24_csv.t ..... ok     
t/30_xls.t ..... # Parser: Spreadsheet::ParseExcel-0.59
t/30_xls.t ..... ok       
t/31_clr.t ..... ok     
t/32_fmt.t ..... ok    
t/33_misc.t .... ok   
t/34_dates.t ... ok    
t/35_perc.t .... ok    
t/36_xls.t ..... ok       
t/40_sxc.t ..... skipped: No SXC parser found
t/45_ods.t ..... skipped: No SXC parser found
t/46_clr.t ..... skipped: No OpenOffice ODS parser found
t/50_sc.t ...... # Parser: Spreadsheet::Read-0.45
t/50_sc.t ...... ok    
t/51_sc.t ...... ok    
t/60_xlsx.t .... skipped: No M$-Excel parser found
t/61_clr.t ..... skipped: No M$-Excel parser found
t/62_fmt.t ..... skipped: No M$-Excel parser found
t/63_misc.t .... skipped: No M$-Excel parser found
t/64_dates.t ... skipped: No M$-Excel parser found
t/65_perc.t .... skipped: No M$-Excel parser found
All tests successful.


* You get an 'APPROVED' from me, provided that you update the %changelog within pkg git and add comments to the explicit Requires.

Comment 4 Ralf Corsepius 2012-01-16 07:46:26 UTC
Requires:       perl(Test::More) >= 0.98
This Requires: is wrong. 

There is no run-time dependency between this package and Test::More:
# grep More Spreadsheet-Read-0.45/Read.pm

Requires:       perl(IO::Scalar)
I am not sure about this one. Looks like this is implemented as optional run-time requirement, while it actually is a mandatory requirement.
I.e. I am inclined to believe this Requires: might be right.


...
t/40_sxc.t ..... skipped: No SXC parser found
t/45_ods.t ..... skipped: No SXC parser found
...
If I understand the code correctly, these warnings indicate that some optional modules, these tests are trying exercise are not available.

I haven't checked all details, but in case of the 40_sxc.t, Spreadsheet::ReadSXC seems to be required to exercise this test.
I'd recommend to also package such optional perl-modules and to BR: them.
This would assure these packages are tested at build-time and are (optionally) available to users at run-time.

Comment 5 Miroslav Suchý 2015-08-21 09:38:53 UTC
No response for years. Closing. Feel free to reopen if you want to continue.


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