Bug 1091144 - Review Request: perl-Parse-DMIDecode - Interface to SMBIOS using dmidecode
Summary: Review Request: perl-Parse-DMIDecode - Interface to SMBIOS using dmidecode
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul Howarth
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1095662
TreeView+ depends on / blocked
 
Reported: 2014-04-25 03:12 UTC by David Dick
Modified: 2014-05-27 19:12 UTC (History)
2 users (show)

Fixed In Version: perl-Parse-DMIDecode-0.03-1.el6
Clone Of:
Environment:
Last Closed: 2014-05-21 23:26:58 UTC
Type: ---
Embargoed:
paul: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Dick 2014-04-25 03:12:17 UTC
Spec URL: http://ddick.fedorapeople.org/packages/perl-Parse-DMIDecode.spec
SRPM URL: http://ddick.fedorapeople.org/packages/perl-Parse-DMIDecode-0.03-1.fc20.src.rpm
Description: Interface to SMBIOS using dmidecode
Fedora Account System Username: ddick

Comment 2 David Dick 2014-04-25 04:53:32 UTC
Re-opened.  I needed to ExclusiveArch this package as the dmidecode package it depends uses one.  Also, needed to skip pod testing on el6.

Ready to go again.

koji builds at 

rawhide http://koji.fedoraproject.org/koji/taskinfo?taskID=6777078

el6 http://koji.fedoraproject.org/koji/taskinfo?taskID=6777077

Comment 3 David Dick 2014-04-26 00:42:44 UTC
removed the unnecessary debug builds.

koji builds at

rawhide http://koji.fedoraproject.org/koji/taskinfo?taskID=6781764

el6 http://koji.fedoraproject.org/koji/taskinfo?taskID=6781767

Comment 4 Paul Howarth 2014-05-09 15:16:44 UTC
A few quick comments before I go through the review checklist:

The POD problem for EL-6 is https://rt.cpan.org/Public/Bug/Display.html?id=52296; the patch https://rt.cpan.org/Ticket/Attachment/699959/360879/fix-pod-urls.patch attached to that ticket fixes the issue and doesn't break other builds, so I think that would be a better fix than skipping the test on EL-6.

Please add a spec comment about why setting %debug_package to %{nil} is desired (I know, but not everybody would get it).

Use of macros for commands like %{__rm} is discouraged in the guidelines.

Also, be aware that Nicola (upstream) hasn't updated any of her CPAN packages since January 2008, so if there's any bugs that need fixing, you're probably on your own. I know this as current maintainer of perl-RRD-Simple...

Comment 5 Paul Howarth 2014-05-09 15:57:26 UTC
rpmlint
=======
perl-Parse-DMIDecode.x86_64: E: no-binary
This is to be expected; the package is really noarch but has to be arch-specific
because its dependency, dmidecode, is not available on all architectures.

Review Checks
=============
- rpmlint OK
- package and spec file naming OK
- package meets guidelines
- license is ASL 2.0, OK for Fedora and matches upstream
- upstream provides license file and it's packaged as %doc
- spec file is legible and written in English
- source matches upstream, including timestamp
- package builds OK in mock for F-19 .. Rawhide and EPEL-6 .. EPEL-7 (i386 and x86_64)
- package is "ExclusiveArch: %{ix86} x86_64 ia64", with explanation included
- build dependencies somewhat over-specified - see below
- no locale data, libraries, devel files to concern ourselves with
- no bundled libraries
- package is not intended to be relocatable
- directory ownership and permissions OK
- no duplicate files
- macro usage is consistent
- code, not content
- no large docs to worry about
- docs don't affect runtime
- not a GUI app, no desktop file needed
- filenames are all ASCII
- no scriptlets or sub-packages

Nits
====
perl(Cwd) and perl(File::Spec) are only needed by Makefile.PL, which you don't
use, so there's no need to BuildRequire them.

perl(Config) is not needed if AUTOMATED_TESTING is set at build time, which it is.

perl(constant) is only used in the example code, so is not needed for the build.

Build.PL asks for Test::Deep but it's not actually used.

No blockers here. APPROVED.

Comment 6 David Dick 2014-05-10 03:09:27 UTC
(In reply to Paul Howarth from comment #4)
> A few quick comments before I go through the review checklist:
> 
> The POD problem for EL-6 is
> https://rt.cpan.org/Public/Bug/Display.html?id=52296; the patch
> https://rt.cpan.org/Ticket/Attachment/699959/360879/fix-pod-urls.patch
> attached to that ticket fixes the issue and doesn't break other builds, so I
> think that would be a better fix than skipping the test on EL-6.

Agreed.  Thanks for this.  Applied.

> Please add a spec comment about why setting %debug_package to %{nil} is
> desired (I know, but not everybody would get it).

Done.

> Use of macros for commands like %{__rm} is discouraged in the guidelines.

Okay.  Removed.

> Also, be aware that Nicola (upstream) hasn't updated any of her CPAN
> packages since January 2008, so if there's any bugs that need fixing, you're
> probably on your own. I know this as current maintainer of perl-RRD-Simple...

Thanks for the warning.

Comment 7 David Dick 2014-05-10 03:33:48 UTC
(In reply to Paul Howarth from comment #5)
> perl(Cwd) and perl(File::Spec) are only needed by Makefile.PL, which you
> don't
> use, so there's no need to BuildRequire them.

Fair.  Removed.

> perl(Config) is not needed if AUTOMATED_TESTING is set at build time, which
> it is.

Done.

> perl(constant) is only used in the example code, so is not needed for the
> build.

Done.
 
> Build.PL asks for Test::Deep but it's not actually used.

And done.

> No blockers here. APPROVED.

Thanks for the review Paul.  Most appreciated.

Comment 8 David Dick 2014-05-10 03:36:52 UTC
New Package SCM Request
=======================
Package Name: perl-Parse-DMIDecode
Short Description: Interface to SMBIOS using dmidecode
Owners: ddick
Branches: f20 el6 epel7
InitialCC: perl-sig

Comment 9 Gwyn Ciesla 2014-05-12 11:52:25 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2014-05-12 12:24:25 UTC
perl-Parse-DMIDecode-0.03-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-Parse-DMIDecode-0.03-1.fc20

Comment 11 Fedora Update System 2014-05-12 12:34:44 UTC
perl-Parse-DMIDecode-0.03-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-Parse-DMIDecode-0.03-1.el6

Comment 12 Fedora Update System 2014-05-12 23:28:03 UTC
perl-Parse-DMIDecode-0.03-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 13 Fedora Update System 2014-05-21 23:26:58 UTC
perl-Parse-DMIDecode-0.03-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 14 Fedora Update System 2014-05-27 19:12:25 UTC
perl-Parse-DMIDecode-0.03-1.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.