Bug 980222 - Review Request: perl-Class-Accessor-Classy - Accessors with minimal inheritance
Review Request: perl-Class-Accessor-Classy - Accessors with minimal inheritance
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Petr Šabata
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-01 14:52 EDT by John C Peterson
Modified: 2013-08-03 15:14 EDT (History)
3 users (show)

See Also:
Fixed In Version: perl-Class-Accessor-Classy-0.9.1-2.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-02 17:50:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
psabata: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description John C Peterson 2013-07-01 14:52:30 EDT
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 05:12:11 EDT
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 03:27:12 EDT
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 04:31:53 EDT
(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 13:23:55 EDT
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-04 21:35:21 EDT
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 14:36:45 EDT
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 14:53:02 EDT
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 17:08:21 EDT
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 13:59:28 EDT
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 17:50:38 EDT
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 18:12:08 EDT
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 15:14:01 EDT
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.