https://bugzilla.redhat.com/show_bug.cgi?id=980222
Bug ID: 980222 Summary: Review Request: perl-Class-Accessor-Classy - Accessors with minimal inheritance Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: jcp@eskimo.com QA Contact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org
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
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Petr Šabata psabata@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |psabata@redhat.com Assignee|nobody@fedoraproject.org |psabata@redhat.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #1 from Petr Šabata psabata@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #2 from John C Peterson jcp@eskimo.com ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Petr Šabata psabata@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #3 from Petr Šabata psabata@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
John C Peterson jcp@eskimo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #4 from John C Peterson jcp@eskimo.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Jens Petersen petersen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #5 from Jens Petersen petersen@redhat.com --- Git done (by process-git-requests).
F17 is in pre-EOL phase so no new packages being added to that branch.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #6 from Fedora Update System updates@fedoraproject.org --- 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.f...
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #7 from Fedora Update System updates@fedoraproject.org --- 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.f...
https://bugzilla.redhat.com/show_bug.cgi?id=980222
--- Comment #8 from Fedora Update System updates@fedoraproject.org --- 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.e...
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #9 from Fedora Update System updates@fedoraproject.org --- perl-Class-Accessor-Classy-0.9.1-2.el6 has been pushed to the Fedora EPEL 6 testing repository.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |perl-Class-Accessor-Classy- | |0.9.1-2.fc18 Resolution|--- |ERRATA Last Closed| |2013-08-02 17:50:38
--- Comment #10 from Fedora Update System updates@fedoraproject.org --- perl-Class-Accessor-Classy-0.9.1-2.fc18 has been pushed to the Fedora 18 stable repository.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|perl-Class-Accessor-Classy- |perl-Class-Accessor-Classy- |0.9.1-2.fc18 |0.9.1-2.fc19
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- perl-Class-Accessor-Classy-0.9.1-2.fc19 has been pushed to the Fedora 19 stable repository.
https://bugzilla.redhat.com/show_bug.cgi?id=980222
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|perl-Class-Accessor-Classy- |perl-Class-Accessor-Classy- |0.9.1-2.fc19 |0.9.1-2.el6
--- Comment #12 from Fedora Update System updates@fedoraproject.org --- perl-Class-Accessor-Classy-0.9.1-2.el6 has been pushed to the Fedora EPEL 6 stable repository.
package-review@lists.fedoraproject.org