[Bug 1242726] Review Request: perl-MooX-Log-Any - Role to add Log::Any

[bugzilla at redhat.com]

https://bugzilla.redhat.com/show_bug.cgi?id=1242726

Petr Šabata <psabata at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+



--- Comment #6 from Petr Šabata <psabata at redhat.com> ---
> * 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