Bug 166317 - Review Request: perl-X11-Protocol
Review Request: perl-X11-Protocol
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
David Lawrence
http://search.cpan.org/~smccam/X11-Pr...
: Reopened
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-18 22:40 EDT by Duncan Ferguson
Modified: 2010-04-11 15:08 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-29 01:40:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


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

  None (edit)
Description Duncan Ferguson 2005-08-18 22:40:02 EDT
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 11:31:54 EDT
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 11:33:10 EDT
Created attachment 118159 [details]
Patch implementing review suggestions
Comment 3 Duncan Ferguson 2005-08-30 14:15:53 EDT
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 04:47:09 EDT
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 14:39:09 EDT
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 11:29:48 EDT
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 13:13:58 EDT
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 13:29:46 EDT
Due on Sunday.

I'll sponsor you but you need to go through the accounts system first.
Comment 9 Duncan Ferguson 2005-09-07 15:24:04 EDT
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 04:53:11 EDT
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 04:24:37 EDT
Attempted to resolve as "next release" but dont have the option to.
Comment 12 Paul Howarth 2005-10-14 04:55:35 EDT
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 01:38:18 EDT
Reopening bug to fix assignee.
Comment 14 Paul Howarth 2006-08-29 01:40:32 EDT
Asignee fixed, closing again.
Comment 15 Mark Chappell 2010-04-09 03:58:43 EDT
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 15:08:42 EDT
cvs done.

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