Bug 759757

Summary:	Review Request: perl-Encode-EUCJPASCII - EucJP-ascii - An eucJP-open mapping
Product:	[Fedora] Fedora	Reporter:	Xavier Bachelot <xavier>
Component:	Package Review	Assignee:	Petr Pisar <ppisar>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	notting, package-review, ppisar
Target Milestone:	---	Flags:	ppisar: fedora-review+ gwync: fedora-cvs+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2012-01-06 17:21:48 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:

Description Xavier Bachelot 2011-12-03 16:14:36 UTC

Spec URL: http://www.bachelot.org/fedora/SPECS/perl-Encode-EUCJPASCII.spec
SRPM URL: http://www.bachelot.org/fedora/SRPMS/perl-Encode-EUCJPASCII-0.03-1.fc15.src.rpm
Description:
This module provides eucJP-ascii, one of eucJP-open mappings, and its
derivative.


This module is needed for better test coverage in perl-MIME-Charset.

Comment 1 Petr Pisar 2012-01-05 12:58:41 UTC

Source tar ball is original. Ok.
Summary verified from EUCJPASCII.pm. Ok.
Description verified from EUCJPASCII.pm. Ok.

Note: I'm not sure `eucJP-ascii' and `eucJP-open' are properly capitalized. Different texts use different capitalization and hyphenation. Let's hope イケダソジ, author of this module, knows better.

License verified from EUCJPASCII.pm. Ok.
URL and Source0 values are useful. Ok.

TODO: Remove useless BuildRoot definition, it's cleaning in %install section, and whole %clean section. They are not needed anymore.

TODO: Build-require perl(Encode) for tests (EUCJPASCII.pm:6)
TODO: Build-require perl(XSLoader) for tests (EUCJPASCII.pm:7)
TODO: Build-require perl(base) for tests (EUCJPASCII.pm:18)
TODO: Build-require perl(Encode::CJKConstants) for tests (EUCJPASCII.pm:23)
TODO: Build-require perl(Encode::JP::JIS7) for tests (EUCJPASCII.pm:23)

FIX: Build-require perl(File::Spec) (Makefile.PL:18)

All tests pass. Ok.

TODO: Remove useless %defattr from %files section.

$ rpmlint perl-Encode-EUCJPASCII.spec ../SRPMS/perl-Encode-EUCJPASCII-0.03-1.fc17.src.rpm ../RPMS/x86_64/perl-Encode-EUCJPASCII-*
perl-Encode-EUCJPASCII.src: W: spelling-error Summary(en_US) eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.src: W: spelling-error %description -l en_US eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.src: W: spelling-error %description -l en_US ascii -> ASCII, ASCIIs, As
perl-Encode-EUCJPASCII.x86_64: W: spelling-error Summary(en_US) eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.x86_64: W: spelling-error %description -l en_US eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.x86_64: W: spelling-error %description -l en_US ascii -> ASCII, ASCIIs, As
perl-Encode-EUCJPASCII.x86_64: E: zero-length /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII/EUCJPASCII.bs
3 packages and 1 specfiles checked; 1 errors, 6 warnings.

FIX: Remove empty *.bs files. (These empty bootstrap files for DynaLoader are useless.)

$ rpm -q -lv -p ../RPMS/x86_64/perl-Encode-EUCJPASCII-0.03-1.fc17.x86_64.rpm 
drwxr-xr-x    2 root    root                        0 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/Encode
-rw-r--r--    1 root    root                     5662 Oct 19  2009 /usr/lib64/perl5/vendor_perl/Encode/EUCJPASCII.pm
drwxr-xr-x    2 root    root                        0 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/auto
drwxr-xr-x    2 root    root                        0 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/auto/Encode
drwxr-xr-x    2 root    root                        0 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII
-rw-r--r--    1 root    root                        0 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII/EUCJPASCII.bs
-rwxr-xr-x    1 root    root                   759816 Jan  5 13:40 /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII/EUCJPASCII.so
drwxr-xr-x    2 root    root                        0 Jan  5 13:40 /usr/share/doc/perl-Encode-EUCJPASCII-0.03
-rw-r--r--    1 root    root                      569 Oct 19  2009 /usr/share/doc/perl-Encode-EUCJPASCII-0.03/Changes
-rw-r--r--    1 root    root                      496 Oct 19  2009 /usr/share/doc/perl-Encode-EUCJPASCII-0.03/README
Files permissions and layout are Ok.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Encode-EUCJPASCII-0.03-1.fc17.x86_64.rpm |sort |uniq -c
      1 libc.so.6()(64bit)  
      1 libc.so.6(GLIBC_2.2.5)(64bit)  
      1 perl(base)  
      1 perl(bytes)  
      1 perl(Encode)  
      1 perl(Encode::CJKConstants)  
      1 perl(Encode::Encoding)  
      1 perl(Encode::JP::JIS7)  
      1 perl(:MODULE_COMPAT_5.14.2)  
      1 perl(strict)  
      1 perl(warnings)  
      1 perl(XSLoader)  
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
      1 rtld(GNU_HASH)  
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/x86_64/perl-Encode-EUCJPASCII-0.03-1.fc17.x86_64.rpm |sort |uniq -c
      1 perl(Encode::EUCJPASCII) = 0.03
      1 perl-Encode-EUCJPASCII = 0.03-1.fc17
      1 perl-Encode-EUCJPASCII(x86-64) = 0.03-1.fc17
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/x86_64/perl-Encode-EUCJPASCII-0.03-1.fc17.x86_64.rpm 
Binary dependencies resolvable. Ok.

Package builds in F17 (http://koji.fedoraproject.org/koji/taskinfo?taskID=3621985). Ok.

Otherwise package is in line with Fedora and Perl packaging guidelines.


Please correct all `FIX' prefixed issues, consider fixing `TODO' items, and provide new spec file.

Resolution: Package NOT approved.

Comment 2 Xavier Bachelot 2012-01-05 16:28:28 UTC

Thanks for the review, Petr.
Comments for FIX and TODO inline.

Spec URL: http://www.bachelot.org/fedora/SPECS/perl-Encode-EUCJPASCII.spec
SRPM URL:
http://www.bachelot.org/fedora/SRPMS/perl-Encode-EUCJPASCII-0.03-2.fc15.src.rpm

(In reply to comment #1)

> TODO: Remove useless BuildRoot definition, it's cleaning in %install section,
> and whole %clean section. They are not needed anymore.
>
Needed for EPEL5, so I'd rather keep them.
 
> TODO: Build-require perl(Encode) for tests (EUCJPASCII.pm:6)
> TODO: Build-require perl(XSLoader) for tests (EUCJPASCII.pm:7)
> TODO: Build-require perl(base) for tests (EUCJPASCII.pm:18)
> TODO: Build-require perl(Encode::CJKConstants) for tests (EUCJPASCII.pm:23)
> TODO: Build-require perl(Encode::JP::JIS7) for tests (EUCJPASCII.pm:23)
> 
Added.

> FIX: Build-require perl(File::Spec) (Makefile.PL:18)
>
Added.

> TODO: Remove useless %defattr from %files section.
> 
Needed for EPEL5.
 
> FIX: Remove empty *.bs files. (These empty bootstrap files for DynaLoader are
> useless.)
> 
Done. The spec is now following more closely the perl spec template, while the previous version was generated with cpanspec.

I've compared the build log from both the older spec and the newer with the BuildRequires added, but I don't see a difference, so I'm a bit puzzled... Are the BRs really needed ?

Comment 3 Petr Pisar 2012-01-06 09:19:39 UTC

SPEC file changes:

--- perl-Encode-EUCJPASCII.spec	2011-11-29 11:16:42.000000000 +0100
+++ perl-Encode-EUCJPASCII.spec.1	2012-01-05 17:19:24.000000000 +0100
@@ -1,6 +1,6 @@
 Name:           perl-Encode-EUCJPASCII
 Version:        0.03
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        EucJP-ascii - An eucJP-open mapping
 License:        GPL+ or Artistic
 Group:          Development/Libraries
@@ -10,6 +10,12 @@
 BuildRequires:  perl(ExtUtils::MakeMaker)
 BuildRequires:  perl(Test::More)
 BuildRequires:  perl(Test::Pod)
+BuildRequires:  perl(File::Spec)
+BuildRequires:  perl(Encode)
+BuildRequires:  perl(XSLoader)
+BuildRequires:  perl(base)
+BuildRequires:  perl(Encode::CJKConstants)
+BuildRequires:  perl(Encode::JP::JIS7)
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))
 
 %{?perl_default_filter}
@@ -22,17 +28,16 @@
 %setup -q -n Encode-EUCJPASCII-%{version}
 
 %build
-%{__perl} Makefile.PL INSTALLDIRS=vendor
+%{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="$RPM_OPT_FLAGS"
 make %{?_smp_mflags}
 
 %install
 rm -rf $RPM_BUILD_ROOT
-
-make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT
-
-find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} \;
-find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;
-
+make pure_install DESTDIR=$RPM_BUILD_ROOT
+find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} ';'
+# Remove the next line from noarch packages (unneeded)
+find $RPM_BUILD_ROOT -type f -name '*.bs' -a -size 0 -exec rm -f {} ';'
+find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null ';'
 %{_fixperms} $RPM_BUILD_ROOT/*
 
 %check
@@ -45,7 +50,12 @@
 %defattr(-,root,root,-)
 %doc Changes README
 %{perl_vendorarch}/*
+%exclude %dir %{perl_vendorarch}/auto/
 
 %changelog
+* Thu Jan 05 2012 Xavier Bachelot <xavier> 0.03-2
+- Follow the rpmdevtools perl spec template to fix packaging bugs.
+- Add missing BuildRequires.
+
 * Tue Nov 29 2011 Xavier Bachelot <xavier> 0.03-1
 - Initial Fedora release.


> > TODO: Remove useless BuildRoot definition, it's cleaning in %install
> > section, and whole %clean section. They are not needed anymore.
> > TODO: Remove useless %defattr from %files section.
> Needed for EPEL5, so I'd rather keep them.
Ok.

> > TODO: Build-require perl(Encode) for tests (EUCJPASCII.pm:6)
> > TODO: Build-require perl(XSLoader) for tests (EUCJPASCII.pm:7)
> > TODO: Build-require perl(base) for tests (EUCJPASCII.pm:18)
> > TODO: Build-require perl(Encode::CJKConstants) for tests (EUCJPASCII.pm:23)
> > TODO: Build-require perl(Encode::JP::JIS7) for tests (EUCJPASCII.pm:23)
> > 
> Added.
> > FIX: Build-require perl(File::Spec) (Makefile.PL:18)
> Added.
+BuildRequires:  perl(File::Spec)
+BuildRequires:  perl(Encode)
+BuildRequires:  perl(XSLoader)
+BuildRequires:  perl(base)
+BuildRequires:  perl(Encode::CJKConstants)
+BuildRequires:  perl(Encode::JP::JIS7)
Ok.

> > FIX: Remove empty *.bs files. (These empty bootstrap files for DynaLoader are
> > useless.)
> Done. The spec is now following more closely the perl spec template, while the
> previous version was generated with cpanspec.
+find $RPM_BUILD_ROOT -type f -name '*.bs' -a -size 0 -exec rm -f {} ';'
Ok.

> I've compared the build log from both the older spec and the newer with the
> BuildRequires added, but I don't see a difference, so I'm a bit puzzled... Are
> the BRs really needed ?
The Perl modules are used directly by this code, so you need to build-require them directly too. You cannot assume a module keeps in the the same RPM package forever (e.g. the Encode is currently part of perl, but If someone decides to sub-package it into perl-Encode or even dual-live it as packe sourced from CPAN, this will not be true anymore) or a module keeps available through indirect dependency (like File::Spec packaged as perl-PathTools).

$ rpmlint perl-Encode-EUCJPASCII.spec ../SRPMS/perl-Encode-EUCJPASCII-0.03-2.fc17.src.rpm ../RPMS/x86_64/perl-Encode-EUCJPASCII-*
perl-Encode-EUCJPASCII.src: W: spelling-error Summary(en_US) eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.src: W: spelling-error %description -l en_US eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.src: W: spelling-error %description -l en_US ascii -> ASCII, ASCIIs, As
perl-Encode-EUCJPASCII.x86_64: W: spelling-error Summary(en_US) eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.x86_64: W: spelling-error %description -l en_US eucJP -> eggcup, equip, uncap
perl-Encode-EUCJPASCII.x86_64: W: spelling-error %description -l en_US ascii -> ASCII, ASCIIs, As
3 packages and 1 specfiles checked; 0 errors, 6 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Encode-EUCJPASCII-0.03-2.fc17.x86_64.rpm 
drwxr-xr-x    2 root    root                        0 Jan  6 09:57 /usr/lib64/perl5/vendor_perl/Encode
-rw-r--r--    1 root    root                     5662 Oct 19  2009 /usr/lib64/perl5/vendor_perl/Encode/EUCJPASCII.pm
drwxr-xr-x    2 root    root                        0 Jan  6 09:57 /usr/lib64/perl5/vendor_perl/auto/Encode
drwxr-xr-x    2 root    root                        0 Jan  6 09:57 /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII
-rwxr-xr-x    1 root    root                   759824 Jan  6 09:57 /usr/lib64/perl5/vendor_perl/auto/Encode/EUCJPASCII/EUCJPASCII.so
drwxr-xr-x    2 root    root                        0 Jan  6 09:57 /usr/share/doc/perl-Encode-EUCJPASCII-0.03
-rw-r--r--    1 root    root                      569 Oct 19  2009 /usr/share/doc/perl-Encode-EUCJPASCII-0.03/Changes
-rw-r--r--    1 root    root                      496 Oct 19  2009 /usr/share/doc/perl-Encode-EUCJPASCII-0.03/README
File permissions and layout is Ok.

Package builds in F17 (http://koji.fedoraproject.org/koji/taskinfo?taskID=3624174). Ok.


Resolution: Package APPROVED.

Comment 4 Xavier Bachelot 2012-01-06 09:55:21 UTC

Thanks for the review and explanations.

New Package SCM Request
=======================
Package Name: perl-Encode-EUCJPASCII
Short Description: EucJP-ascii - An eucJP-open mapping
Owners: xavierb
Branches: f15 f16 el5 el6
InitialCC: perl-sig

Comment 5 Gwyn Ciesla 2012-01-06 14:22:55 UTC

Git done (by process-git-requests).

Comment 6 Xavier Bachelot 2012-01-06 17:21:48 UTC

Imported and built for rawhide, F16, F15, EL6 and EL5.