Bug 980222 - Review Request: perl-Class-Accessor-Classy - Accessors with minimal inheritance
Summary: Review Request: perl-Class-Accessor-Classy - Accessors with minimal inheritance
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-07-01 18:52 UTC by John C Peterson
Modified: 2013-08-03 19:14 UTC (History)
3 users (show)

Fixed In Version: perl-Class-Accessor-Classy-0.9.1-2.el6
Clone Of:
Environment:
Last Closed: 2013-08-02 21:50:38 UTC
Type: ---
Embargoed:
psabata: fedora-review+
petersen: fedora-cvs+


Attachments (Terms of Use)

Description John C Peterson 2013-07-01 18:52:30 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy-0.9.1-1.fc17.src.rpm

Description: The Class::Accessor::Classy Perl module provides an extremely small footprint accessor/mutator declaration scheme for fast and convenient object attribute setup. It is intended as a simple and speedy mechanism for preventing hash-key typos rather than a full-blown object system with type checking and so on.

The accessor methods appear as a hidden parent class of your package and generally try to stay out of the way. The accessors and mutators generated are of the form C<foo()> and C<set_foo()>, respectively.

This software is licensed under the same terms as Perl itself (GPL+ or Artistic), so there should not be any problems there.

This package is a pre-requisite for another Perl module that I would like to package: perl-CAD-Format-STL (which reads and writes stereo lithography format files used in 3d printing and other computer aided design applications).

Fedora Account System Username: jcp

Comment 1 Petr Šabata 2013-07-02 09:12:11 UTC
Do you plan to push this package into old EPEL repositories?  If not...

Line 37 is obsolete and should be removed:
 37 rm -rf %{buildroot}

So is line 39...:
 39 find %{buildroot} -depth -type d -exec rmdir {} 2>/dev/null \;

And also the whole %clean section and %defattr macro.


You're missing some build-time dependencies:
 - perl; called in spec, recommended
 - perl(attributes), lib/Class/Accessor/Classy.pm:676, recommended
 - perl(Carp), lib/Class/Accessor/Classy.pm:6, required
 - perl(strict), various places, recommended
 - perl(version), t/00-load.t:10, recommended
 - perl(warnings), various places, recommended


Note: Nowadays using simple 'perl' is preferred to the '%{__perl}' macro.  However, that's up to you.

Comment 2 John C Peterson 2013-07-03 07:27:12 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy-0.9.1-2.fc17.src.rpm

Hi Petr,

Thanks for doing the review.

I think I have addressed those issues in release two above.

I was indeed going to push this back to EPEL at some point. I don't like the look of the "rm -rf %{buildroot}" either, so I put those inside ?rhel conditionals. (Heaven forbid if %{buildroot} were "/" !!!)

I'm perhaps a little out of touch with building for EPEL, so I wasn't sure about an explicit Buildroot: ... definition. I had already removed the one that cpanspec had put in there. (I was under the impression that's not needed anymore, even for EPEL).

I'm a bit surprised that cpanspec missed all of those build dependencies. I'll know to perform my own search of the code for missed build dependencies in the future.

Comment 3 Petr Šabata 2013-07-03 08:31:53 UTC
(In reply to John C Peterson from comment #2)
> Spec URL: http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy.spec
> SRPM URL:
> http://www.eskimo.com/~jcp/perl-Class-Accessor-Classy-0.9.1-2.fc17.src.rpm
> 
> Hi Petr,
> 
> Thanks for doing the review.

Glad to help :)

> I think I have addressed those issues in release two above.
> 
> I was indeed going to push this back to EPEL at some point. I don't like the
> look of the "rm -rf %{buildroot}" either, so I put those inside ?rhel
> conditionals. (Heaven forbid if %{buildroot} were "/" !!!)

The conditionals aren't necessary and I'd say they make the spec even uglier.
Also the chances of %buildroot being something terribly wrong and the user doing the build having the permissions to do any damage there are really low.
And if that happens, it's their own fault ;)

> I'm perhaps a little out of touch with building for EPEL, so I wasn't sure
> about an explicit Buildroot: ... definition. I had already removed the one
> that cpanspec had put in there. (I was under the impression that's not
> needed anymore, even for EPEL).

You will need it for EPEL5.
https://fedoraproject.org/wiki/EPEL:Packaging

See the other relevant sections of EPEL guidelines too.  You won't need much for EPEL6 only.

> I'm a bit surprised that cpanspec missed all of those build dependencies.
> I'll know to perform my own search of the code for missed build dependencies
> in the future.

The stable versions of cpanspec only look into the META.* files which often don't contain much (or correct) information.  I'm currently working on a better, PPI-based dependency detection but peeking at the code will always be the best option.


Anyhow, the package conforms to the guidelines now.  Approving.

Comment 4 John C Peterson 2013-07-04 17:23:55 UTC
New Package SCM Request
=======================
Package Name: perl-Class-Accessor-Classy
Short Description: Accessors with minimal inheritance
Owners: jcp
Branches: f17 f18 f19 el6
InitialCC: perl-sig

Comment 5 Jens Petersen 2013-07-05 01:35:21 UTC
Git done (by process-git-requests).

F17 is in pre-EOL phase so no new packages being added to that branch.

Comment 6 Fedora Update System 2013-07-05 18:36:45 UTC
perl-Class-Accessor-Classy-0.9.1-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/perl-Class-Accessor-Classy-0.9.1-2.fc19

Comment 7 Fedora Update System 2013-07-05 18:53:02 UTC
perl-Class-Accessor-Classy-0.9.1-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/perl-Class-Accessor-Classy-0.9.1-2.fc18

Comment 8 Fedora Update System 2013-07-05 21:08:21 UTC
perl-Class-Accessor-Classy-0.9.1-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-Class-Accessor-Classy-0.9.1-2.el6

Comment 9 Fedora Update System 2013-07-06 17:59:28 UTC
perl-Class-Accessor-Classy-0.9.1-2.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 10 Fedora Update System 2013-08-02 21:50:38 UTC
perl-Class-Accessor-Classy-0.9.1-2.fc18 has been pushed to the Fedora 18 stable repository.

Comment 11 Fedora Update System 2013-08-02 22:12:08 UTC
perl-Class-Accessor-Classy-0.9.1-2.fc19 has been pushed to the Fedora 19 stable repository.

Comment 12 Fedora Update System 2013-08-03 19:14:01 UTC
perl-Class-Accessor-Classy-0.9.1-2.el6 has been pushed to the Fedora EPEL 6 stable repository.


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