Bug 166317 - Review Request: perl-X11-Protocol
Summary: Review Request: perl-X11-Protocol
Keywords:
Status: CLOSED NEXTRELEASE
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: http://search.cpan.org/~smccam/X11-Pr...
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-08-19 02:40 UTC by Duncan Ferguson
Modified: 2010-04-11 19:08 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2006-08-29 05:40:32 UTC
Type: ---
Embargoed:
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch implementing review suggestions (1.79 KB, patch)
2005-08-26 15:33 UTC, Paul Howarth
no flags Details | Diff

Description Duncan Ferguson 2005-08-19 02:40:02 UTC
Please note, this is my first package and I need a sponsor.

Spec Url: http://queeg.dyndns.org/perl-X11-Protocol.spec
SRPM Url: http://queeg.dyndns.org/perl-X11-Protocol-0.54-1.src.rpm
Description: 

X11::Protocol is a client-side interface to the X11 Protocol (see X(1) for
information about X11), allowing perl programs to display windows and
graphics on X11 servers.

Comment 1 Paul Howarth 2005-08-26 15:31:54 UTC
Review:

- package and spec naming OK
- package meets guidelines
- license is same as perl (with one or two exceptions noted in README, which
  are more liberal)
- spec file written in English and is legible
- sources match upstream
- package builds fine on FC4 (i386)
- no BR's needed
- no locales, libraries, subpackages, pkgconfigs etc. to worry about
- not relocatable
- no directory ownership issues, but see below on clarity
- no duplicate files
- no permissions issues
- %clean section present and correct
- macro usage consistent
- code, not content
- no large docs
- docs don't affect runtime
- no scriptlets


Needswork:

- the test suite requires a running X server, so builds in mock will fail;
  what I would do is to edit the test script in %prep:

# Testing requires X - use "rpmbuild --with X"
%{!?_with_X:\
%{__perl} -pi -e 'print "print \"Remaining tests require X\n\"; exit 0;"
                       if /Insert your test code below/;' test.pl \
}

  This will run the full test suite if the "--with X" option is passed to
  rpmbuild, else just the quick test that the module can be loaded.


Nitpick:

- rpmlint doesn't like shebangs (#!/usr/bin/perl) at the start of perl modules;
  not that they do any harm, but I'd suggest editing them out, e.g.in %prep:

# Remove shebangs from module code
find . -name '*.pm' -exec sed -i -e '/^#!\/usr\/bin\/perl$/d' {} ';'

- BR: perl is redundant and should be deleted
- license text not included in package; suggest adding to %prep:

perldoc -t perlartistic > Artistic
perldoc -t perlgpl > COPYING

  and to %files:

%doc Artistic COPYING

- setting of CFLAGS and OPTIMIZE is redundant for noarch packages
- I'd include the examples directory as %doc
- I'd change:
%{perl_vendorlib}/X11*
  to:
%{perl_vendorlib}/X11/
  in the %files list as I think it's clearer that way


I'm far from being a sed or perl guru so if you can think of better/neater ways
of cleaning up these nitpicks, by all means use them.

Sponsorship:

Try reviewing a few of other people's packages - a list of packages awaiting
review can be found at:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-NEW&hide_resolved=1

That way, potential sponsors will gain confidence in your abilities as a packager.




Comment 2 Paul Howarth 2005-08-26 15:33:10 UTC
Created attachment 118159 [details]
Patch implementing review suggestions

Comment 3 Duncan Ferguson 2005-08-30 18:15:53 UTC
Patch applied - I was expecting to have to do some work, but you did it all for
me ;-)

Updated and available at:

Spec Url: http://queeg.dyndns.org/perl-X11-Protocol.spec
SRPM Url: http://queeg.dyndns.org/perl-X11-Protocol-0.54-2.src.rpm

Comment 4 Paul Howarth 2005-08-31 08:47:09 UTC
OK, obviously I'm happy enough with the package as it is to approve it now.

Do you have any plans for additional packages in Extras other than clusterssh?


Comment 5 Duncan Ferguson 2005-08-31 18:39:09 UTC
No plans at the moment - it seems hard enough finding time to do this while also
juggling young kids and a wife ;-)

Comment 6 Paul Howarth 2005-09-01 15:29:48 UTC
You'll need to create a Fedora Account for yourself as described at
http://fedoraproject.org/wiki/Extras/Contributors step 8.

FWIW, my first young'un is due to arrive in the next week or so...


Comment 7 Duncan Ferguson 2005-09-07 17:13:58 UTC
Is it time for congratulations yet?

Is there anything else I need to do before you (or someone else) approves the
package?

Comment 8 Paul Howarth 2005-09-07 17:29:46 UTC
Due on Sunday.

I'll sponsor you but you need to go through the accounts system first.

Comment 9 Duncan Ferguson 2005-09-07 19:24:04 UTC
I think i am already in there (under the user "duncs") but in a rejected state
(as i applied for cvsextras too early in the process).

Just tried adding myself again but got an error

_pg.error: ERROR: duplicate key violates unique constraint "role_person_id_key"

Are you able to change my status from "rejected" to "approved"?

Comment 10 Paul Howarth 2005-09-08 08:53:11 UTC
You should be OK now (did you get an email from the accounts system?).

Looking at http://fedoraproject.org/wiki/Extras/Contributors it seems that the
documentation is much improved on when I was going through this process :-)


Comment 11 Duncan Ferguson 2005-10-14 08:24:37 UTC
Attempted to resolve as "next release" but dont have the option to.

Comment 12 Paul Howarth 2005-10-14 08:55:35 UTC
Make sure you are in the "fedorabugs" group in the accounts system.

https://admin.fedora.redhat.com/accounts/userbox.cgi


Comment 13 Paul Howarth 2006-08-29 05:38:18 UTC
Reopening bug to fix assignee.

Comment 14 Paul Howarth 2006-08-29 05:40:32 UTC
Asignee fixed, closing again.

Comment 15 Mark Chappell 2010-04-09 07:58:43 UTC
Package Change Request
======================
Package Name: perl-X11-Protocol
New Branches: EL-5
Owners: tremble

Have had a private email from Jima and he's happy for me to take on the EL branches

Comment 16 Kevin Fenzi 2010-04-11 19:08:42 UTC
cvs done.


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