Bug 992951 - Review Request: perl-MooX-Types-MooseLike - Some Moosish types and a type builder
Review Request: perl-MooX-Types-MooseLike - Some Moosish types and a type bui...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
Fedora Extras Quality Assurance
:
: 1120201 (view as bug list)
Depends On: 992753
Blocks: 992697
  Show dependency treegraph
 
Reported: 2013-08-05 05:28 EDT by Simone Caronni
Modified: 2014-07-16 09:01 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-19 03:30:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paul: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Simone Caronni 2013-08-05 05:28:28 EDT
Spec URL: http://slaanesh.fedorapeople.org/perl-MooX-Types-MooseLike.spec
SRPM URL: http://slaanesh.fedorapeople.org/perl-MooX-Types-MooseLike-0.25-1.fc19.src.rpm
Description: See MooX::Types::MooseLike::Base for a list of available base types. Its source also provides an example of how to build base types, along with both parameterizable and non-parameterizable.
Fedora Account System Username: slaanesh
Comment 1 Paul Howarth 2013-08-11 08:17:12 EDT
rpmlint output
==============
$ rpmlint ~/perl-MooX-Types-MooseLike-0.25-1.fc20.*
perl-MooX-Types-MooseLike.noarch: W: spelling-error %description -l en_US parameterizable -> parameter
perl-MooX-Types-MooseLike.src: W: spelling-error %description -l en_US parameterizable -> parameter
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

Nothing to worry about there.

requires
========
perl(:MODULE_COMPAT_5.18.0)
perl(Carp)
perl(Exporter) >= 5.57
perl(List::Util)
perl(Module::Runtime)
perl(Module::Runtime) >= 0.012
perl(MooX::Types::MooseLike)
perl(Scalar::Util)
perl(strict)
perl(warnings)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

provides
========
perl(MooX::Types::MooseLike) = 0.25
perl(MooX::Types::MooseLike::Base) = 0.25
perl-MooX-Types-MooseLike = 0.25-1.fc20

review checklist
================

- rpmlint OK
- package and spec file naming OK
- package meets guidelines
- license is same as perl, OK for Fedora and correctly specified in spec
- upstream did not include a license file to package
- spec file written in English and is legible
- source file matches upstream
- package builds OK in mock for Rawhide x86_64
- buildreqs mostly OK but see below
- no libraries, locale data, devel files etc. to worry about
- package is not intended to be relocatable
- directory ownership OK
- no duplicate files
- permissions are sane
- macro usage is consistent
- code, not content
- no large docs
- docs don't affect runtime
- not a GUI app, no desktop file needed
- all filenames are ASCII
- no scriptlets present or needed
- no subpackages present or needed
- requires/provides OK

Nits:

The Test::More version requirement of this package means that it won't even build for EPEL-6,
so there's no point in including various bits of boilerplate that are only needed for EPEL-5
support:

 * BuildRoot specification
 * Emptying the buildroot at the start of %install
 * Having a %clean section
 * The default %defattr(-,root,root,-) isn't even needed for EPEL-5

There's never a need to remove empty directories from the buildroot.

I don't really see the value to end users of including META.json.

The usual convention these days is to use DESTDIR rather than PERL_INSTALL_ROOT.

If you want to explicitly require Module::Runtime ≥ 0.012, you should probably
filter the automatically-generated dependency on Module::Runtime (any version):
%global __provides_exclude ^perl\\(Module::Runtime\\)$

For completeness, the following are also needed as they are used either by the module
or its test suite:

BuildRequires: perl(Carp)
BuildRequires: perl(Exporter) >= 5.57
BuildRequires: perl(IO::Handle)
BuildRequires: perl(List::Util)
BuildRequires: perl(Moo::Role)
BuildRequires: perl(overload)
BuildRequires: perl(Role::Tiny)
BuildRequires: perl(Scalar::Util)
BuildRequires: perl(strict)
BuildRequires: perl(warnings)

There's also an argument for having a runtime dependency on perl(Moose::Util::TypeConstraints), but as that would pull in Moose, there's also a good argument for not including it. It's probably better for users that need that functionality to require it themselves.


None of these are blockers.

APPROVED.
Comment 2 Simone Caronni 2013-08-11 12:44:08 EDT
I'm currently on holiday with very limited GPRS connectivity. I will address all the issues you pointed in the previous comment when I come back on the 19th of August. I will remove all EPEL support from it (originally it was meant to update an EPEL perl package which has a dependency on this).

Thanks very much for the review; if you need any review in return please let me know.
Comment 3 Paul Howarth 2013-08-11 13:20:04 EDT
(In reply to Simone Caronni from comment #2)
> I'm currently on holiday with very limited GPRS connectivity. I will address
> all the issues you pointed in the previous comment when I come back on the
> 19th of August. I will remove all EPEL support from it (originally it was
> meant to update an EPEL perl package which has a dependency on this).

Well, before you go hacking out the EPEL support, it may be possible to hack the test suite to work with an old version of Test::More. I've done this myself many times, and usually the thing you need to look for are:

* If a test uses done_testing and you want to use it with Test::More < 0.88 (e.g. EPEL-5), you'll need to remove the done_testing and specify how many tests to expect (plan).

* If a test uses subtests (needs Test::More 0.94, or Test::More 0.96 if there's no plan), you'll need to remove the subtests and run them as part of the main test (EPEL-6 had Test::More 0.94 I think).

* Change note() to diag() to work with Test::More < 0.82 (e.g. EPEL-5).

You'll also need to check out the other dependencies for the package to see if they can be satisfied in EPEL. If you're planning on having EPEL support it's best to try building the entire dependency stack you need in EPEL to make sure it's viable before you start submitting the package reviews, particularly if you're going to have to hack test suites in a few packages to get it to work. You might get near the top of the stack before realizing it's not going to be possible, which would save you some time in package reviews if you know it in advance.

> Thanks very much for the review; if you need any review in return please let
> me know.

The only one I have outstanding is for perl-Archive-Any-Lite (Bug #991693) but hopefully someone will take it before you get back from holiday :-)
Comment 4 Simone Caronni 2013-08-18 14:24:30 EDT
Thanks for all your helpful information!

Said this I've hacked away EPEL support. This is a new requirement for a perl package that not need to be forcefully updated in EPEL; so I will leave the old version for that branch that uses a different syntax. Fortunately we run the perl script using this on Fedora.

Spec URL: http://slaanesh.fedorapeople.org/perl-MooX-Types-MooseLike.spec
SRPM URL: http://slaanesh.fedorapeople.org/perl-MooX-Types-MooseLike-0.25-2.fc19.src.rpm
Comment 5 Simone Caronni 2013-08-18 14:26:35 EDT
New Package SCM Request
=======================
Package Name: perl-MooX-Types-MooseLike
Short Description: Some Moosish types and a type builder
Owners: slaanesh
Branches: f19
Comment 6 Kevin Fenzi 2013-08-18 15:15:39 EDT
Git done (by process-git-requests).
Comment 7 Ralf Corsepius 2014-07-16 09:01:53 EDT
*** Bug 1120201 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.