Bug 791363 - Review Request: perl-XML-DTDParser - Quick and dirty DTD parser
Summary: Review Request: perl-XML-DTDParser - Quick and dirty DTD parser
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Paul Howarth
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-02-16 20:28 UTC by Bill Pemberton
Modified: 2012-04-27 06:03 UTC (History)
3 users (show)

Fixed In Version: perl-XML-DTDParser-2.01-4.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-03-31 03:06:21 UTC
Type: ---
paul: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 684006 None None None Never

Internal Links: 684006

Description Bill Pemberton 2012-02-16 20:28:36 UTC
Spec URL: http://wfp.fedorapeople.org/perl-XML-DTDParser.spec
SRPM URL: http://wfp.fedorapeople.org/perl-XML-DTDParser-2.01-1.fc16.src.rpm

Description: A simple DTD parser that may be useful in conjunction with XML::Rules.

Comment 1 Paul Howarth 2012-02-17 15:36:56 UTC
First pass:

Package fails to build in mock:

Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.YlZXgq
+ umask 022
+ cd /builddir/build/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ cd /builddir/build/BUILD
+ rm -rf XML-DTDParser-2.01
+ /usr/bin/gzip -dc /builddir/build/SOURCES/XML-DTDParser-2.01.tar.gz
+ /usr/bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd XML-DTDParser-2.01
+ /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w .
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.feEGiz
+ exit 0
+ umask 022
+ cd /builddir/build/BUILD
+ cd XML-DTDParser-2.01
+ LANG=C
+ export LANG
+ unset DISPLAY
+ /usr/bin/perl Makefile.PL INSTALLDIRS=vendor
Checking if your kit is complete...
Looks good
Writing Makefile for XML::DTDParser
+ make -j4
cp DTDParser.pm blib/lib/XML/DTDParser.pm
Manifying blib/man3/XML::DTDParser.3pm
+ find . -type f -exec sed -i 's/\r//' '{}' ';'
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.mHe7cJ
+ umask 022
+ cd /builddir/build/BUILD
+ '[' /builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64 '!=' / ']'
+ rm -rf /builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64
++ dirname /builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64
+ mkdir -p /builddir/build/BUILDROOT
+ mkdir /builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64
+ cd XML-DTDParser-2.01
+ LANG=C
+ export LANG
+ unset DISPLAY
+ rm -rf /builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64
+ make pure_install PERL_INSTALL_ROOT=/builddir/build/BUILDROOT/perl-XML-DTDParser-2.01-1.fc18.x86_64
Makefile out-of-date with respect to Makefile.PL
Cleaning current config before rebuilding Makefile...
make -f Makefile.old clean > /dev/null 2>&1
/usr/bin/perl Makefile.PL "INSTALLDIRS=vendor"
Checking if your kit is complete...
Looks good
Writing Makefile for XML::DTDParser
==> Your Makefile has been rebuilt. <==
==> Please rerun the make command.  <==
false
make: *** [Makefile] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.mHe7cJ (%install)
    Bad exit status from /var/tmp/rpm-tmp.mHe7cJ (%install)
RPM build errors:
Child return code was: 1


I suspect that this is because you are fixing the line endings in %build after "make". You should fix line endings and make other source changes in %prep where possible, which would avoid this problem. Also I think only the README file actually needs its line-endings fixed anyway, so there's no need to touch the other files.

It's usual practice now to use DESTDIR rather than PERL_INSTALL_ROOT when installing.

Consider adding buildreqs for Perl core modules used by the module and its test suite: Exporter, FileHandle, strict, File::Spec, Cwd and Test.

Comment 2 Bill Pemberton 2012-02-20 14:24:07 UTC
It's odd that it doesn't build in mock for you, as I test build these before I upload them.

Anyhow, I've moved the line endings fix into prep and added the BuildRequires, except for strict at that doesn't seem to make sense since there's no way that could be broken out as a separate module/feature.  

Also, the use of PERL_INSTALL_ROOT instead of destdir is required as this module doesn't appear to honor destdir.

Spec URL: http://wfp.fedorapeople.org/perl-XML-DTDParser.spec
SRPM URL: http://wfp.fedorapeople.org/perl-XML-DTDParser-2.01-2.fc16.src.rpm

Comment 3 Paul Howarth 2012-02-22 15:43:11 UTC
(In reply to comment #2)
> It's odd that it doesn't build in mock for you, as I test build these before I
> upload them.

Odd indeed.

> Anyhow, I've moved the line endings fix into prep and added the BuildRequires,
> except for strict at that doesn't seem to make sense since there's no way that
> could be broken out as a separate module/feature.  
> 
> Also, the use of PERL_INSTALL_ROOT instead of destdir is required as this
> module doesn't appear to honor destdir.

destdir (lower-case) is a parameter that Module::Build uses, but this is an
ExtUtils::MakeMaker based distribution and that supports DESTDIR (upper case).

perl-XML-DTDParser Review
=========================

rpmlint output:
perl-XML-DTDParser.noarch: W: spelling-error %description -l en_US optionality -> nationality, proportionality, rationality
perl-XML-DTDParser.src: W: spelling-error %description -l en_US optionality -> nationality, proportionality, rationality
perl-XML-DTDParser.src:12: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)

The spelling-error is a false positive and can be ignored.
You should fix the mixed-use-of-spaces-and-tabs and use one or the other
consistently.

- package and spec naming OK
- package meets guidelines
- license in spec matches upstream and is OK for Fedora
- no upstream license file to include in package
- spec file written in English and is legible
- source tarball matches upstream, including timestamp
- package builds fine in mock
- build dependencies are comprehensive and correct
- no locale data or libraries to worry about
- package not designed to be relocatable
- directory ownership OK
- no duplicate files
- permissions sane
- macro usage is consistent
- code, not content
- no large documentation to consider
- docs don't affect runtime
- no static libraries or devel files to consider
- no libtool archives to consider
- not a GUI application so no desktop file needed
- filenames are all plain ASCII

Suggestions (all non-blockers):

* Use DESTDIR rather than PERL_INSTALL_ROOT

* Sort buildreqs for readability

* For larger modules, it may help to split buildreqs into groups based on
  where they're needed, e.g. Module Build/Module Runtime/Test Suite/Release Tests
  See for example:
  http://pkgs.fedoraproject.org/gitweb/?p=perl-Test-Version.git;a=blob;f=perl-Test-Version.spec;hb=master

* Consider making the %files list more explicit, so you won't accidentally ship
  additional files if an upstream update includes some bundled code that
  shouldn't be in Fedora:

  %files
  %defattr(-,root,root,-)
  %doc Changes README
  %{perl_vendorlib}/XML/
  %{_mandir}/man3/XML::DTDParser.3pm*

* Consider dropping the use of macros for commands; there's no real benefit to
  using "%{__id_u} -n" rather than "id -nu", "%{__perl}" rather than "perl",
  or "%{__sed}" rather than "sed"

* Try to maintain the same format for your changelog entries, as it makes it
  more readable; one entry in this spec has a dash between your email address
  and the version-release but the other doesn't. I'd suggest always using a
  dash as the changelog entries added during mass rebuilds do so.

The only thing that definitely needs fixing here is the mixed use of spaces
and tabs. Having now seen three of your packages, I'm comfortable enough to
sponsor you. They're all small perl modules, which don't really lend themselves
to demonstrating the full range of your packaging skills and knowledge of the
guidelines, but on the other hand you are receptive to advice without just
accepting anything a reviewer tells you, which I think bodes well.

APPROVED.

I've now sponsored you for membership of the packager group. Feel free to email me if you have any issues with Fedora packaging, processes etc.

Comment 4 Bill Pemberton 2012-02-22 18:30:01 UTC
Missing the tabs and spaces thing, I see I have to get more in the habit of running rpmlint on everything I do.

So addressing that and a few other complaints.

Spec URL: http://wfp.fedorapeople.org/perl-XML-DTDParser.spec
SRPM URL: http://wfp.fedorapeople.org/perl-XML-DTDParser-2.01-3.fc16.src.rpm

Comment 5 Bill Pemberton 2012-02-22 18:33:13 UTC
New Package SCM Request
=======================
Package Name: perl-XML-DTDParser
Short Description: Quick and dirty DTD parser
Owners: wfp
Branches: f16 f17
InitialCC:

Comment 6 Gwyn Ciesla 2012-02-22 19:10:49 UTC
Git done (by process-git-requests).

Comment 7 Paul Howarth 2012-02-22 20:20:37 UTC
It's usual for perl module packages to have perl-sig in the InitialCC: so that the perl-devel-list gets copied in on bug reports etc. Not compulsory though.

Comment 8 Paul Howarth 2012-03-18 10:39:13 UTC
Bill, you only seem to have done a build for Rawhide; is there some problem with doing the f16/f17 builds?

Comment 9 Fedora Update System 2012-03-19 13:05:32 UTC
perl-XML-DTDParser-2.01-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/perl-XML-DTDParser-2.01-3.fc16

Comment 10 Fedora Update System 2012-03-19 13:19:18 UTC
perl-XML-DTDParser-2.01-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-XML-DTDParser-2.01-3.fc17

Comment 11 Fedora Update System 2012-03-20 06:06:02 UTC
perl-XML-DTDParser-2.01-3.fc17 has been pushed to the Fedora 17 testing repository.

Comment 12 Fedora Update System 2012-03-31 03:06:21 UTC
perl-XML-DTDParser-2.01-3.fc16 has been pushed to the Fedora 16 stable repository.

Comment 13 Paul Howarth 2012-04-09 16:26:35 UTC
Something I missed in the review:

The directory %{perl_vendorlib}/XML/ should be owned by the package, and it isn't.

Please change in the %%files list:
%{perl_vendorlib}/XML/DTDParser.pm
to:
%{perl_vendorlib}/XML/

(this is one of 119 perl module packages I found with this type of problem)

Comment 14 Fedora Update System 2012-04-09 16:59:47 UTC
perl-XML-DTDParser-2.01-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/perl-XML-DTDParser-2.01-4.fc16

Comment 15 Fedora Update System 2012-04-09 17:07:33 UTC
perl-XML-DTDParser-2.01-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-XML-DTDParser-2.01-4.fc17

Comment 16 Fedora Update System 2012-04-12 02:35:57 UTC
perl-XML-DTDParser-2.01-3.fc17 has been pushed to the Fedora 17 stable repository.

Comment 17 Fedora Update System 2012-04-26 20:05:38 UTC
perl-XML-DTDParser-2.01-4.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2012-04-27 06:03:47 UTC
perl-XML-DTDParser-2.01-4.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.


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