https://bugzilla.redhat.com/show_bug.cgi?id=1242726 Petr Šabata <psabata@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ --- Comment #6 from Petr Šabata <psabata@xxxxxxxxxx> --- > * 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. Ack, updated. > * The package summary could be a little bit more descriptive, "A Moose role > to add support for logging via Log::Any", maybe? Ack, summary changed. > * The BuildRoot tag is only needed in EPEL5. You may drop it. Ack, dropped. > * The perl version constraint isn't really all that useful in our case. Ack, version contraint dropped. > 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. Ack, all three added. > You also execute Makefile.PL, which requires `strict' and `warnings'. Add > `perl(strict)' and `perl(warnings)' to your list. Ack, both added. > 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. Ack, dropped. > * Runtime dependencies -- (...) 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. Ack, all three dropped. > * Line 38, `rm -rf $RPM_BUILD_ROOT' -- this is no longer needed unless > you're packaging for EPEL5. Ack, dropped. > * 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. Ack, changed to DESTDIR. > * 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. Ack, all dropped. > * 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 > ... Ack, looks good. > * Finally, your changelog format isn't valid[2] (missing e-mail address). Ack, also fixed. -- That went well! I'm going to approve this package and sponsor you into the Packager group. You may proceed with an SCM request[0]. Since this is a perl package, please, add `perl-sig' to InitialCC. Once the package is created in the PkgDB, you may also want to register it in Anitya[1], our upstream release monitoring service. It automatically checks for new versions, submits FMN[2] notifications and, optionally, files `please update' bugs for you. [0] https://fedoraproject.org/wiki/Package_SCM_admin_requests [1] https://release-monitoring.org/ [2] https://apps.fedoraproject.org/notifications/ -- 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