https://bugzilla.redhat.com/show_bug.cgi?id=1074482 Paul Howarth <paul@xxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review+ --- Comment #2 from Paul Howarth <paul@xxxxxxxxxxxx> --- Review checklist ================ - rpmlint clean - package and spec file naming OK - package meets guidelines - license is same as perl, matches upstream, and is OK for Fedora - upstream's license file is packaged - spec file written in English and is legible - source file matches upstream - package builds OK is mock for EPEL-6 (x86_64) and in koji for F-21 (x86_64) - build dependencies mostly ok (see below) - no locale data or shared libraries to concern ourselves with - no bundled libraries included - package is not intended to be relocatable - directory ownership is fine - no duplicate files - permissions are fine - macro usage is consistent - code, not content - no large docs - docs shouldn't affect runtime - no static libraries, development files or sub-packages to worry about - not a GUI app, no desktop file needed - all filenames are ASCII - no scriptlets - no pkgconfig file needed TODO ==== perl(Module::Build) version requirement should be 0.35, as per your patch. I don't see where perl(base), perl(File::Spec), perl(Scalar::Util), perl(Storable) and perl(vars) are used, except in example files and/or documentation, so they're not really needed as buildreqs or (in some cases) explicit runtime deps. Similarly, Data::Dumper is used by the test suite and in an example file, but there's no real runtime need for it, so the explicit require of it is unnecessary. Upstream has manually added Test::Fatal as a test dependency but doesn't actually use it, so it could be removed. They also specified Test::Simple 0.98 but as you've found, older versions work fine. Looking at the tests, I think Test::More version 0.82 is the actual requirement, for note() in t/00-compile.t. I'd drop the Test::Simple dependency and add the version dependency for perl(Test::More). Nits ==== You have the build requirements for the release tests, but don't run them; I'd suggest either dropping those buildreqs or running the release tests. Since you've added an explicit runtime dependency on perl(Net::Netrc), you should probably add a BuildRequires for it too. Unless there's a bootstrapping issue, I think it's good practice to build-require everything that's required at runtime, which can help you find some dependency issues at build-time that you wouldn't otherwise discover until the built package is pushed to a repo and someone tries to install it. The dependency filtering is usually placed just before %description in the spec. https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Location_of_macro_invocation I'd add a comment that your filtering is to remove under-specified dependencies, since you're explictly requiring particular versions of those modules. I'd put %{perl_vendorlib}/Net/ instead of %{perl_vendorlib}/* in the %files list; there's really no need for a wildcard there. Summary ======= Package has been put together carefully and isn't just the output of cpanspec; none of the issues above are blockers, so feel free to address them or not as you see fit. APPROVED. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review