Review of perl(EBook::EPUB) , 25th August 2010.

mock in f15 fails, missing build requires.

PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
#   Failed test 'use EBook::EPUB;'
#   at t/00-load.t line 6.
#     Tried to use 'EBook::EPUB'.
#     Error:  Can't locate version.pm in @INC (@INC contains: /builddir/build/BUILD/EBook-EPUB-0.5/blib/lib /builddir/build/BUILD/EBook-EPUB-0.5/blib/arch /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5 /usr/share/perl5 /usr/lib64/perl5 /usr/share/perl5 /usr/local/lib64/perl5/site_perl/5.10.0/x86_64-linux-thread-multi /usr/local/lib/perl5/site_perl/5.10.0 /usr/lib64/perl5/vendor_perl/5.10.0/x86_64-linux-thread-multi /usr/lib/perl5/vendor_perl /usr/lib/perl5/site_perl) at /builddir/build/BUILD/EBook-EPUB-0.5/blib/lib/EBook/EPUB.pm line 27.
# BEGIN failed--compilation aborted at /builddir/build/BUILD/EBook-EPUB-0.5/blib/lib/EBook/EPUB.pm line 27.


- Package meets naming and packaging guidelines
Yes CPAN module called EBook::EPUB
- Spec file matches base package name.
Yes
- Spec has consistant macro usage.
Yes.
- Meets Packaging Guidelines.
Almost  but comments below concening ignore.txt 
- License
BSD.
- License field in spec matches
Yes, the README and the headers of the files make this clearly
BSD but they do say to go and read the LICESNE file.
- License file included in package
No it's not but it is referenced in the README.

This module is free software; you can redistribute it and/or modify it 
under the terms of the BSD license. See the LICENSE file included with 
this distribution.

So I think it is important in this case that upstream can be contacted
to try and add a LICENSE files.


- Spec in American English
- Spec is legible.
- Sources match upstream md5sum:
$ md5sum EBook-EPUB-0.5.tar.gz ../SOURCES/EBook-EPUB-0.5.tar.gz 
ed6a49cba2a173db3c479b229aec0cc2  EBook-EPUB-0.5.tar.gz
ed6a49cba2a173db3c479b229aec0cc2  ../SOURCES/EBook-EPUB-0.5.tar.gz

- Package needs ExcludeArch
No it does not
- BuildRequires correct
Mock okay.
- Spec handles locales/find_lang
None present.
- Package is relocatable and has a reason to be.
Not relocatable.
- Package has %defattr and permissions on files is good.
It does
- Package has a correct %clean section.
It does.
- Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Package is code or permissible content.
- Doc subpackage needed/used.
- Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

- Package is a GUI app and has a .desktop file

- Package compiles and builds on at least one arch.
- Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.
- No rpmlint output.
- final provides and requires are sane:



Issues:

1. I think there is no reason to include ignore.txt , it adds nothing
   to the package.
2. Can you contact upstream to request a LICENSE file.
3. I have been asked previously to change:


make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT

to

make pure_install DESTDIR=$RPM_BUILD_ROOT

and is something I always do.


4. The fail in mock of course.

Steve

(In reply to comment #1)
>
> 1. I think there is no reason to include ignore.txt , it adds nothing
>    to the package.

You're right, done.

> 2. Can you contact upstream to request a LICENSE file.

Done. http://rt.cpan.org/Ticket/Display.html?id=60759

> 3. I have been asked previously to change:
> 
> make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT
> to
> make pure_install DESTDIR=$RPM_BUILD_ROOT
> 
> and is something I always do.

Done.

> 4. The fail in mock of course.

Fixed by adding perl(version) in the BuildRequires

Spec URL:
http://people.parinux.org/~seyman/fedora/perl-EBook-EPUB/perl-EBook-EPUB.spec
SRPM URL:
http://people.parinux.org/~seyman/fedora/perl-EBook-EPUB/perl-EBook-EPUB-0.5-2.fc13.src.rpm

You man want to add a 

%{?perl_default_subpackage_tests}

after the preamble to generate the -test package. Up to you.

+ builds okay in mock.
+ rpmlint output completely clean.

APPROVED.

(In reply to comment #3)
>
> You man want to add a 
> %{?perl_default_subpackage_tests}
> after the preamble to generate the -test package. Up to you.

Good idea. Added.

> APPROVED.

Thanks for the review, Steve.

New Package SCM Request
=======================
Package Name: perl-EBook-EPUB
Short Description: Perl module for generating EPUB documents
Owners: eseyman
Branches: f12 f13 f14
InitialCC: perl-sig

Git done (by process-git-requests).

perl-EBook-EPUB-0.5-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/perl-EBook-EPUB-0.5-2.fc14

perl-EBook-EPUB-0.5-2.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/perl-EBook-EPUB-0.5-2.fc13

perl-EBook-EPUB-0.5-2.fc12 has been submitted as an update for Fedora 12.
https://admin.fedoraproject.org/updates/perl-EBook-EPUB-0.5-2.fc12

perl-EBook-EPUB-0.5-2.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update perl-EBook-EPUB'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/perl-EBook-EPUB-0.5-2.fc12

perl-EBook-EPUB-0.5-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

perl-EBook-EPUB-0.5-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

perl-EBook-EPUB-0.5-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.