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=791363 Paul Howarth <paul@xxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review+ --- Comment #3 from Paul Howarth <paul@xxxxxxxxxxxx> 2012-02-22 10:43:11 EST --- (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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review