[Bug 773502] New: Review Request: perl-Spreadsheet-read - universal API to read any spreadsheet
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: perl-Spreadsheet-read - universal API to read any spreadsheet
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Summary: Review Request: perl-Spreadsheet-read - universal API to read any spreadsheet Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: mgoodwin@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: --- Regression: --- Mount Type: --- Documentation: ---
Spec URL: http://people.redhat.com/mgoodwin/cpan/perl-Spreadsheet-read/perl-Spreadshee...
SRPM URL: http://people.redhat.com/mgoodwin/cpan/perl-Spreadsheet-read/perl-Spreadshee...
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Mark Goodwin mgoodwin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fche@redhat.com Blocks| |754678
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
--- Comment #1 from Michael Schwendt mschwendt@gmail.com 2012-01-12 05:16:47 EST --- [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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
--- Comment #2 from Mark Goodwin mgoodwin@redhat.com 2012-01-12 16:24:05 EST --- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |mschwendt@gmail.com Flag| |fedora-review+
--- Comment #3 from Michael Schwendt mschwendt@gmail.com 2012-01-15 17:24:47 EST ---
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rc040203@freenet.de
--- Comment #4 from Ralf Corsepius rc040203@freenet.de 2012-01-16 02:46:26 EST --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Michael Schwendt bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW Assignee|bugs.michael@gmx.net |nobody@fedoraproject.org Flags|fedora-review+ |
https://bugzilla.redhat.com/show_bug.cgi?id=773502
Miroslav Suchý msuchy@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED CC| |msuchy@redhat.com Blocks| |201449 (FE-DEADREVIEW) Resolution|--- |INSUFFICIENT_DATA Last Closed| |2015-08-21 05:38:53
--- Comment #5 from Miroslav Suchý msuchy@redhat.com --- No response for years. Closing. Feel free to reopen if you want to continue.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=201449 [Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter response should be blocking this bug.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla