Bug 1079751 (perl-Date-Calc-XS) - Review Request: perl-Date-Calc-XS - XS wrapper and C library plug-in for Date::Calc
Summary: Review Request: perl-Date-Calc-XS - XS wrapper and C library plug-in for Date...
Keywords:
Status: CLOSED RAWHIDE
Alias: perl-Date-Calc-XS
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: David Dick
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: naemon
TreeView+ depends on / blocked
 
Reported: 2014-03-23 17:35 UTC by Sven Nierlein
Modified: 2014-05-19 22:30 UTC (History)
4 users (show)

Fixed In Version: 6.3-4
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-05-19 22:30:25 UTC
ddick: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
Fix for non-utf8 encoding in CREDITS.txt (909 bytes, patch)
2014-04-22 11:14 UTC, David Dick
no flags Details | Diff

Description Sven Nierlein 2014-03-23 17:35:53 UTC
Spec URL: http://nierlein.com/fedora/perl-Date-Calc-XS.spec
SRPM URL: http://nierlein.com/fedora/perl-Date-Calc-XS-6.3-1.fc21.src.rpm
Description:
Date::Calc::XS is a XS wrapper and C library plug-in for Date::Calc

Fedora Account System Username: sni

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

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, #1079749

Comment 1 Sven Nierlein 2014-04-06 10:47:11 UTC
Updated spec file according to comments from related bugs and
uploaded new files here:

http://nierlein.com/fedora/2014-04-06/perl-Date-Calc-XS.spec
http://nierlein.com/fedora/2014-04-06/perl-Date-Calc-XS-6.3-1.fc21.src.rpm

Comment 2 David Dick 2014-04-09 09:14:39 UTC
removed FE-NEEDSPONSOR

Comment 3 David Dick 2014-04-09 10:12:10 UTC
Hi Sven,

Congrats and welcome to fedora!

The following things need to get fixed in this package

FIX:

Licensing.

Given that DateCalc.h, DateCalc.c and ToolBox.h are only licensed under the LGPLv2+ and the rest of the package is "GPL+ or Artistic", i would suggest the License entry should be "LGPLv2+ and GPL+ or Artistic". Petr? Any comments?

BR: perl(strict) # line 13 of Makefile.PL
BR: perl(vars) # line 16 of lib/Date/Calc/XS.pm
BR: perl(Date::Calc::Object) # line 10 of t/m011.t
BR: perl(Exporter) # line 20 of lib/Date/Calc/XS.pm
BR: perl(DynaLoader) # line 21 of lib/Date/Calc/XS.pm
BR: perl(bytes) # line 3 of t/f029.t
BR: perl(Date::Calendar) # line 16 of t/m009.t (packaged as part of perl-Date-Calc)
BR: perl(Date::Calendar::Profiles) # line 15 of t/m009.t (also part of perl-Date-Calc)
BR: perl(Bit::Vector) # line 7 of t/m009.t
BR: perl(Date::Calendar::Year) # line 129 of t/f000.t (part of perl-Date-Calc)
BR: perl(Config) # line 20 of Makefile.PL

Your changelog entries need to be either resolved into one entry per release (fine with me) or each individual entry should get a new release number

The following things should be fixed;

TODO:

The following versions are specified in Makefile.PL.  However, they may restrict you if you want to do older builds (<= EPEL6/F19).  If you can want to build for older versions, we can discuss these requirements.

BR: perl(Carp::Clan) >= 6.01 
BR: perl(Bit::Vector) >= 7.1
BR: perl(Date::Calc) >= 6.3

To fix rpmlint complaints, include the following line;
find $RPM_BUILD_ROOT -type f -name '*.bs' -size 0 -exec rm -f {} \;
following
find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} \;

Replace the various %doc lines with

%doc CHANGES.txt README.txt CREDITS.txt license

Replace Source0 (by author) with 
Source0: http://www.cpan.org/modules/by-module/Date/Date-Calc-XS-%{version}.tar.gz

This will protect you if the author changes.

Comment 4 Petr Šabata 2014-04-09 10:53:35 UTC
(In reply to David Dick from comment #3)
> Licensing.
> 
> Given that DateCalc.h, DateCalc.c and ToolBox.h are only licensed under the
> LGPLv2+ and the rest of the package is "GPL+ or Artistic", i would suggest
> the License entry should be "LGPLv2+ and GPL+ or Artistic". Petr? Any
> comments?

Yup.  Add parenthesis around "GPL+ or Artistic" :)

Comment 5 David Dick 2014-04-09 10:55:52 UTC
Thanks Petr.

So the License line should read

License: LGPLv2+ and ( GPL+ or Artistic )

Comment 6 Sven Nierlein 2014-04-21 13:45:31 UTC
new spec file:
http://nierlein.com/fedora/2014-04-21/perl-Date-Calc-XS.spec

I changed the license, the %doc and the BRs. I cannot reproduce the rpmlint issues, the only complains are:

perl-Date-Calc-XS.x86_64: W: file-not-utf8 /usr/share/doc/perl-Date-Calc-XS/CREDITS.txt
perl-Date-Calc-XS.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Date-Calc-XS/license/GNU_GPL.txt
perl-Date-Calc-XS.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Date-Calc-XS/license/GNU_LGPL.txt
1 packages and 0 specfiles checked; 2 errors, 1 warnings.


You mentioned a few more BRs, should we add them, even if they are not in the test requires from Makefile.PL?

Comment 7 Ralf Corsepius 2014-04-22 09:37:25 UTC
Missing: 
Requires:  perl(:MODULE_COMPAT_%(eval "`perl -V:version`"; echo $version))

(In reply to Sven Nierlein from comment #6)
> I changed the license, the %doc and the BRs.
Next time also provide an src.rpm. It's impossible to review a package from *.spec only.

> perl-Date-Calc-XS.x86_64: W: file-not-utf8
> /usr/share/doc/perl-Date-Calc-XS/CREDITS.txt
This is a MUSTFIX. You are supposed to convert this file to UTF-8.

Comment 8 David Dick 2014-04-22 11:14:01 UTC
Hi Sven,

Yes, the BRs are required.  Otherwise your packages will be more prone to breaking because of changes in other packages (such as packages being taken out of the default build environment), and it'll be harder to check if your package is going to be affected by a change.

You can fix the utf8 warning by applying the attached patch.  First include the line

Patch1:  date_calc_credits_utf8.patch

and follow the line 

%setup -q -n Date-Calc-XS-%{version}

with

%patch1 -p1

The incorrect-fsf-address can be addressed by filing a rt.cpan.org bug against Date-Calc-XS.

The rpmlint issues i was referring to come from the following package

$ rpm -q rpmlint -i
Name        : rpmlint
Version     : 1.5
Release     : 8.fc20
Architecture: noarch
Install Date: Wed 19 Feb 2014 09:19:27 PM EST
Group       : Development/Tools
Size        : 1225796
License     : GPLv2
Signature   : RSA/SHA256, Tue 11 Feb 2014 01:17:08 PM EST, Key ID 2eb161fa246110c1
Source RPM  : rpmlint-1.5-8.fc20.src.rpm
Build Date  : Tue 11 Feb 2014 08:10:57 AM EST
Build Host  : arm02-builder06.arm.fedoraproject.org
Relocations : (not relocatable)
Packager    : Fedora Project
Vendor      : Fedora Project
URL         : http://sourceforge.net/projects/rpmlint/
Summary     : Tool for checking common errors in RPM packages
Description :
rpmlint is a tool for checking common errors in RPM packages.  Binary
and source packages as well as spec files can be checked.

$ rpmlint rpmbuild/RPMS/x86_64/perl-Date-Calc-XS-6.3-3.fc20.x86_64.rpm 
perl-Date-Calc-XS.x86_64: W: file-not-utf8 /usr/share/doc/perl-Date-Calc-XS/CREDITS.txt
perl-Date-Calc-XS.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Date-Calc-XS/license/GNU_GPL.txt
perl-Date-Calc-XS.x86_64: E: zero-length /usr/lib64/perl5/vendor_perl/auto/Date/Calc/XS/XS.bs
perl-Date-Calc-XS.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Date-Calc-XS/license/GNU_LGPL.txt
1 packages and 0 specfiles checked; 3 errors, 1 warnings.

Comment 9 David Dick 2014-04-22 11:14:48 UTC
Created attachment 888444 [details]
Fix for non-utf8 encoding in CREDITS.txt

Comment 10 David Dick 2014-04-25 08:45:33 UTC
Corrected.  This is covered in Perl/Tips at https://fedoraproject.org/wiki/Perl/Tips?rd=PackagingTips/Perl#file-not-utf8

Something like

iconv --from=ISO-8859-1 --to=UTF-8 CREDITS.txt >CREDITS.fixed
mv CREDITS.fixed CREDITS.txt

should do it.  Don't forget to add the

BR: iconv

if you choose to use this.

Comment 11 David Dick 2014-05-14 11:53:45 UTC
Hi Sven,

The following is a patch summarizing the remaining work for this bug.

--- perl-Date-Calc-XS.spec      2014-04-21 23:42:28.000000000 +1000
+++ perl-Date-Calc-XS.proposed  2014-05-14 21:35:30.415071204 +1000
@@ -6,27 +6,43 @@
 Group:          Development/Libraries
 URL:            http://search.cpan.org/dist/Date-Calc-XS/
 Source0:        http://www.cpan.org/modules/by-module/Date/Date-Calc-XS-%{version}.tar.gz
+# glibc-common contains the iconv binary
+BuildRequires:  glibc-common
 BuildRequires:  perl
+BuildRequires:  perl(Bit::Vector)
+BuildRequires:  perl(bytes)
+BuildRequires:  perl(Config)
+BuildRequires:  perl(Date::Calc::Object)
+BuildRequires:  perl(Date::Calendar)
+BuildRequires:  perl(Date::Calendar::Profiles)
+BuildRequires:  perl(Date::Calendar::Year)
+BuildRequires:  perl(DynaLoader)
+BuildRequires:  perl(Exporter)
 BuildRequires:  perl(Test::More) >= 0.47
 BuildRequires:  perl(ExtUtils::MakeMaker)
 BuildRequires:  perl(Carp::Clan) >= 6.01
 BuildRequires:  perl(Bit::Vector) >= 7.1
 BuildRequires:  perl(Date::Calc) >= 6.3
+BuildRequires:  perl(strict)
+BuildRequires:  perl(vars)
+Requires:       perl(:MODULE_COMPAT_%(eval "`perl -V:version`"; echo $version))
 
 %description
 Date::Calc::XS is a XS wrapper and C library plug-in for Date::Calc
 
 %prep
 %setup -q -n Date-Calc-XS-%{version}
+iconv --from=ISO-8859-1 --to=UTF-8 CREDITS.txt >CREDITS.fixed
+mv CREDITS.fixed CREDITS.txt
 
 %build
-%{__perl} Makefile.PL INSTALLDIRS=vendor
+%{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="$RPM_OPT_FLAGS"
 make %{?_smp_mflags}
 
 %install
 make pure_install DESTDIR=$RPM_BUILD_ROOT
 find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} \;
+find $RPM_BUILD_ROOT -type f -name '*.bs' -size 0 -exec rm -f {} \;
 %{_fixperms} $RPM_BUILD_ROOT/*
 
 %check

Hope this helps.

Comment 12 Sven Nierlein 2014-05-17 14:38:41 UTC
oh great, thanks a lot.

Here are the new files:
http://nierlein.com/fedora/2014-05-17/perl-Date-Calc-XS-6.3-4.fc21.src.rpm
http://nierlein.com/fedora/2014-05-17/perl-Date-Calc-XS.spec

Upstream ticket has been created to address the incorrect-fsf-address issue:
https://rt.cpan.org/Ticket/Display.html?id=95735

Comment 13 David Dick 2014-05-17 21:17:45 UTC
License is ok.

Build Requires/Requires ok.

rpmlint only complains about addressed incorrect-fsf-address.

koji build at http://koji.fedoraproject.org/koji/taskinfo?taskID=6859850

Comment 14 David Dick 2014-05-17 21:26:25 UTC
Package approved

Comment 15 Sven Nierlein 2014-05-19 16:55:02 UTC
New Package SCM Request
=======================
Package Name: perl-Date-Calc-XS
Short Description: XS wrapper and C library plug-in for Date::Calc
Owners: sni
Branches: f21
InitialCC: perl-sig

Comment 16 Gwyn Ciesla 2014-05-19 17:11:09 UTC
Git done (by process-git-requests).

Comment 17 Sven Nierlein 2014-05-19 22:30:25 UTC
import completed


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