Bug 172677 - Review Request: perl-Readonly
Summary: Review Request: perl-Readonly
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul Howarth
QA Contact: David Lawrence
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 172676 173053
TreeView+ depends on / blocked
 
Reported: 2005-11-08 08:02 UTC by Michael A. Peters
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2005-12-08 19:01:32 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
spec file (2.30 KB, text/plain)
2005-11-08 15:16 UTC, Michael A. Peters
no flags Details
Updated spec file (1.88 KB, text/plain)
2005-11-13 06:58 UTC, Michael A. Peters
no flags Details
Updated spec file (1.88 KB, text/plain)
2005-12-08 18:09 UTC, Michael A. Peters
no flags Details
current spec file (1.94 KB, text/plain)
2005-12-08 18:25 UTC, Michael A. Peters
no flags Details

Description Michael A. Peters 2005-11-08 08:02:26 UTC
Spec Name or Url: http://mpeters.us/fc_extras/perl-Readonly.spec
SRPM Name or Url: http://mpeters.us/fc_extras/perl-Readonly-1.03-1.src.rpm
Description:

Readonly.pm provides a facility for creating non-modifiable scalars,
arrays, and hashes.

Readonly.pm
* Creates scalars, arrays (not lists), and hashes.
* Creates variables that look and work like native perl variables.
* Creates global or lexical variables.
* Works at runtime or compile time.
* Works with deep or shallow data structures.
* Prevents reassignment of Readonly variables.

Comment 1 Michael A. Peters 2005-11-08 08:08:19 UTC
This package has two source files.

The perl-Readonly src could produce a noarch rpm, it does not *need* the binary
Readonly::XS module - but it benefits from it performance wise - and thus should
be made available.

Readonly::XS is only of value if Readonly is installed.

Rather than building them as separate packages with a noarch perl-Readonly
package requiring the binary arch specific perl-Readonly-XS package, I figured
it was cleaner to just package both in the same rpm.

Comment 2 Michael A. Peters 2005-11-08 15:16:57 UTC
Created attachment 120815 [details]
spec file

I've attached the spec file as the server where it is hosted seems to be having
some timeout issues for some people.

Comment 3 Ralf Corsepius 2005-11-10 17:56:48 UTC
I am still unable to access your site.

Therefore only some comments on the spec file from the attachment:

1. FE's packaging policy recommends to ship one package per perl distribution,
and I don't see many compelling reasons to make an exception in this case.
I.e. I recommend to split this package into 2 packages:
perl-Readonly and perl-Readonly-XS

2. Readonly wants to ship this file:
/usr/lib/perl5/vendor_perl/5.8.6/benchmark.pl
IMO, shipping a script of this name at this location should be avoided.
I recommend to move it elsewhere (eg. %doc) or to remove it entirely.

Comment 4 Michael A. Peters 2005-11-10 18:47:55 UTC
I'm going to contact my service provider and ask why access isn't working for
some people.

-=-

With respect to splitting it - I can do that, but then perl-Readonly-XS will
need to Require perl-Require because it is useless without perl-Readonly.

However - perl-Readonly should also require perl-Readonly-XS because it gives
perl-Readonly-XS a performance boost, and perl scripts that use perl-Readonly
will not require perl-Readonly-XS by automatic dep resolution, so the only way
to get perl-Readonly-XS pulled in with yum is to have perl-Readonly require it.

Since the two modules will thus need to require each other, and are maintained
by the same maintainer, and released in pair (new version of one means new
version of other) - does it really make sense for them to be separate packages
in this case?

Comment 5 Ville Skyttä 2005-11-12 09:04:39 UTC
I think the bundling makes sense in this case.  I'd personally add "Provides: 
perl-Readonly-XS = %{version}". 
 
And the inclusion of benchmark.pl the way it's done now looks pretty suspicious 
indeed.  If it can't be removed or moved to %doc, it needs to renamed to 
something less generic. 

Comment 6 Ville Skyttä 2005-11-12 09:14:31 UTC
(In reply to comment #5) 
> I'd personally add "Provides: perl-Readonly-XS = %{version}".  
 
Oops, s/%{version}/%{version}-%{release}/ 

Comment 7 Ralf Corsepius 2005-11-13 04:51:09 UTC
(In reply to comment #5)
> I think the bundling makes sense in this case.
I could not disagree more.

* Readonly-XS and Readonly are independent, there is no dependency between both.
Actually, Readonly-XS is a sort of overlay plugin to Readonly

* Readonly is noarch/Readonly-XS is arch'ed.
Probs will occur, should Readonly-XS not be buildable on one particular
architecture.

* Splitting both makes the *specs much simpler and less error-prone.

=> There is no technically need to merge these perl-dists into one, therefore
there is no need to make an exception from the FE packaging rules.

Comment 8 Michael A. Peters 2005-11-13 06:09:59 UTC
OK.
I'll build them separately and submit a spec file for perl-Readonly-XS.
I'll fix the benchmark.pl issue as well - I suspect it can be thrown into %doc
but I'll verify first.

Comment 9 Michael A. Peters 2005-11-13 06:56:19 UTC
New rpm and spec file:

http://mpeters.us/fc_extras/perl-Readonly-1.03-2.src.rpm
http://mpeters.us/fc_extras/perl-Readonly.spec

I'll attach the spec file in case there are still issues with people not being
able to access my site.

Comment 10 Michael A. Peters 2005-11-13 06:58:03 UTC
Created attachment 120998 [details]
Updated spec file

Comment 11 Paul Howarth 2005-12-08 11:25:06 UTC
Review:

- rpmlint clean
- package and spec naming OK
- spec file written in English and is legible
- sources match upstream
- package builds OK on FC4 (i386) and in mock for rawhide (i386)
- no locales, libraries, subpackages or pkgconfigs to worry about
- not relocatable
- no directory ownership or permissions problems
- no duplicate files
- code, not content
- %clean section present and correct
- macro usage is consistent
- no large docs
- docs don't affect runtime
- no desktop entry needed
- no scriptlets

Needswork:

- license is same as perl (i.e. GPL or Artistic), not just Artistic
- redundant BR perl (listed in exceptions section of packaging guidelines)
- redundant BR's perl(Test::More) and perl(Test::Harness) - both modules are
bundled with perl

Suggestions:

- use %{?_smp_mflags} with make in %build
- "find $RPM_BUILD_ROOT -type f -name '*.bs' -a -size 0 -exec rm -f {} ';'" not
needed for noarch packages


Comment 12 Michael A. Peters 2005-12-08 17:57:29 UTC
OK - Thanks, fixed those issues

http://mpeters.us/fc_extras/perl-Readonly-1.03-3.src.rpm
http://mpeters.us/fc_extras/perl-Readonly.spec

Comment 13 Michael A. Peters 2005-12-08 18:09:31 UTC
Created attachment 122036 [details]
Updated spec file

New spec file and src.rpm:

http://mpeters.us/fc_extras/perl-Readonly-1.03-4.src.rpm
http://mpeters.us/fc_extras/perl-Readonly.spec

Comment 14 Paul Howarth 2005-12-08 18:20:54 UTC
Looks good. Approved.

You should change the %{?_smp_mflags} in the changelog to %%{?_smp_mflags}
though - try "rpm -q --changelog perl-Readonly-1.03-4.src.rpm" to see why. You
can do this post-cvs-import if you prefer.


Comment 15 Michael A. Peters 2005-12-08 18:25:21 UTC
Created attachment 122038 [details]
current spec file

Sorry - attached old spec file last time.

Comment 16 Michael A. Peters 2005-12-08 18:34:13 UTC
(In reply to comment #14)
> Looks good. Approved.
> 
> You should change the %{?_smp_mflags} in the changelog to %%{?_smp_mflags}
> though - try "rpm -q --changelog perl-Readonly-1.03-4.src.rpm" to see why. You
> can do this post-cvs-import if you prefer.
>

Oh yes - definitely. My bad - I know about that.



Comment 17 Michael A. Peters 2005-12-08 19:01:32 UTC
imported, owners list updated, build succesful in devel branch


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