https://bugzilla.redhat.com/show_bug.cgi?id=1242726 --- Comment #4 from Petr Šabata <psabata@xxxxxxxxxx> --- So, the review... I'll start from the top :) * You should always package the latest version available. In this case, that would be 0.004002, released in April. The difference[0] is minimal so I'll just continue reviewing the version you've packaged for now. Updating will be straightforward. * The package summary could be a little bit more descriptive, "A Moose role to add support for logging via Log::Any", maybe? * Note: The Group tag is optional nowdays. Still, since you're packaging for EPEL, you need to keep it. * The BuildRoot tag is only needed in EPEL5. You may drop it. * The perl version constraint isn't really all that useful in our case. * Build dependencies -- Packaging Guidelines used to list a set of packages that were guaranteed to be present in every buildroot. That is no longer the case, therefore I suggest explicitly listing everything your package is directly using at build time, including every perl mdoule or pragma, no matter how common they might be. Your package calls `make' (lines 35, 40 and 48), `find' (lines 42 and 43), and `rm' and `rmdir' (lines 38, 42, 43 and 51). You should therefore buildrequire `make', `findutils' and `coreutils'. If you drop some of these (see below), drop the added dependency too, of course. You also execute Makefile.PL, which requires `strict' and `warnings'. Add `perl(strict)' and `perl(warnings)' to your list. Log::Any::Adapter isn't actually used anywhere in the code, it's only mentioned in the metadata and, judging from the changelog, I think upstream meant to remove it. Drop it from your BuildRequires. * Runtime dependencies -- rpmbuild tries its best and generates most runtime dependencies automatically by scanning the files present in the resulting RPM package. There's also some basic support for Perl (via perl-generators) so you don't have to list all the required perl modules explicitly. In your case, both `Moo::Role' and and `Log::Any' are automatically detected. Furthermore, as I've already mentioned, Log::Any::Adapter isn't used at all. You can drop these three `Requires' lines. To check the resulting dependencies, query the package with the --requires/-R option: $ rpm -qRp perl-MooX-Log-Any-0.004001-1.fc23.noarch.rpm perl(:MODULE_COMPAT_5.22.0) perl(Log::Any) perl(Log::Any) perl(Log::Any::Adapter) perl(Moo::Role) perl(Moo::Role) rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsXz) <= 5.2-1 (you can see the duplicates here -- one comes from perl-generators, one from your explicit `Requires') * Note: Line 34, `%{__perl} Makefile.PL INSTALLDIRS=vendor' -- this is fine. I just want to mention the possibility to add `NO_PACKLIST=1' with EU::MM 6.76+ to supress packlist generation. This is only supported in Fedora. EPEL's EE::MM is too old. * Line 38, `rm -rf $RPM_BUILD_ROOT' -- this is no longer needed unless you're packaging for EPEL5. * Note: Line 40, `make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT' -- DESTDIR is supported in both Fedora and EPEL. You can use it instead of PERL_INSTALL_ROOT here. * Note: Line 42, `find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} \;' -- you wouldn't need this with NO_PACKLIST. Just so you know. * Line 43 (`find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;') isn't necessary. * The whole %clean section is only needed in EPEL5. Drop it. * The standard %defattr definition is no longer needed. Not even in EPEL. Drop it. * Documentation. Don't package `dist.ini', `weaver.ini' or `META.json'. These have no value for the end user. Also, `README.md' doesn't provide anything the module perldoc doesn't. In you case, I would keep just the simple `README" file. The license text requires special handling. Fedora mandates[1] that license texts are installed with a special macro, %license, which ensures they get installed even if documentation is disabled. However, EPEL doesn't support this macro (yet?). There are many ways to work around this. You can, for example, check whether %_licensedir is defined and if it isn't, define %license as %doc: %{!?_licensedir:%global license %%doc} %license LICENSE %doc Changes README ... * Finally, your changelog format isn't valid[2] (missing e-mail address). That should be it. Feel free to ask if anything's unclear. [0] https://metacpan.org/diff/file?target=CAZADOR/MooX-Log-Any-0.004002/&source=CAZADOR/MooX-Log-Any-0.004001/ [1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines [2] https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs -- 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