Bug 442377 - Review Request: python-pysctp - Python binding for the SCTP network protocol
Summary: Review Request: python-pysctp - Python binding for the SCTP network 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: Rakesh Pandit
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-04-14 16:12 UTC by Neil Horman
Modified: 2008-09-11 17:21 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-09-11 17:21:25 UTC
Type: ---
Embargoed:
rpandit: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Neil Horman 2008-04-14 16:12:42 UTC
Spec URL: http://nhorman.fedorapeople.org/python-pysctp-0.3.1-1.fc8.src.rpm
SRPM URL: http://nhorman.fedorapeople.org/python-pysctp.spec
Description: python-pysctp is the python binding code for the SCTP network protocol.   SCTP uses a set of extensions to the standard BSD socket api, and this pythhon module exposes those extensions to the python programming language, making the protocol useable

Comment 1 Neil Horman 2008-08-05 16:17:46 UTC
ping, can someoneae please review this?  Thanks!

Comment 2 Rakesh Pandit 2008-09-09 18:33:21 UTC
Build successfully
http://koji.fedoraproject.org/koji/taskinfo?taskID=816108


[rpmbuild@rocky SRPMS]$ rpmlint python-pysctp-0.3.1-1.fc9.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[rpmbuild@rocky i386]$ rpmlint python-pysctp-debuginfo-0.3.1-1.fc9.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[rpmbuild@rocky i386]$ rpmlint -i python-pysctp-0.3.1-1.fc9.i386.rpm 
python-pysctp.i386: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

python-pysctp.i386: E: devel-dependency lksctp-tools-devel
Your package has a dependency on a devel package but it's not a devel package
itself.

I guess you can ignore this one.

1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Requires and BuildRequires have lot of repeated dependencies

python-setuptools-devel will provide you python & python-setuptools so you can remove both.

Once you update I will do a detailed review.

Comment 3 Rakesh Pandit 2008-09-09 18:34:26 UTC
You should include doc files like draft in doc folder, README (it has some essential info) ChangeLog

Comment 4 Neil Horman 2008-09-09 20:30:52 UTC
New files, with issues above addressed:
Spec URL: http://nhorman.fedorapeople.org/python-pysctp-0.3.1-2.fc8.src.rpm
SRPM URL: http://nhorman.fedorapeople.org/python-pysctp.spec

Comment 5 Rakesh Pandit 2008-09-10 05:18:57 UTC
Your link to spec has old spec file. I am using srpm which has latest spec though :)

Comment 6 Rakesh Pandit 2008-09-10 05:26:49 UTC
python-pysctp.i386: W: incoherent-version-in-changelog 0.3.1-1 0.3.1-2.fc9
The last entry in %changelog contains a version identifier that is not
coherent with the epoch:version-release tuple of the package.

1 packages and 0 specfiles checked; 1 errors, 1 warnings.

You have forgotten to update the changeLog. Please do it.

python-pysctp.i386: E: devel-dependency lksctp-tools-devel
IGNORED

Comment 7 Rakesh Pandit 2008-09-10 06:37:22 UTC
APPROVED

Summary:
1. Fix changelog entry before importing
2. Please look at some details mentioned below before requesting for CVS and importing

Required:

[!] rpmlint -- pasted above. Correctly fill the changelog entry
[x] Naming -- Okay, we already have package names like python-pydns python-pyspf python-pycurl

    You can however keep pysctp also because it is an upstream name and is treated as an exception in
python naming convention
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28python_modules.29 

It is your choice. But make this choice before requesting for CVS :)

[x] md5sum

Source downloaded from project hosting
[rpmbuild@rocky python]$ md5sum pysctp-0.3.1.tar.gz 
7fd43fa7a7f31cff557d0a69fa751e5f  pysctp-0.3.1.tar.gz

Source from srpm
[rpmbuild@rocky python-pysctp-0.3.1-2.fc8]$ md5sum pysctp-0.3.1.tar.gz 
7fd43fa7a7f31cff557d0a69fa751e5f  pysctp-0.3.1.tar.gz

Match

[x] %files section - includes all files

In spite of using {python_sitelib}/* you may consider using {python_sitelib}/<modulename>/<regex> which is better option. This is optional though, but what you are doing is bad practice. 

[x] license - fedora approved
[x] license field - matches actual license
[x] source does not have license file - may be requested upstream
[?] eggs -

Check whether eggs are handled correctly in F-8, you may like to have a look at 
https://fedoraproject.org/wiki/Packaging/Python/Eggs#Providing_Eggs_for_non-setuptools_packages

My mock seems not working right now so could not check for f-8 specifically. But it seems to me okay - looking at spec

[x] spec file legible and written in American English
[x] source URL correct
[x] Builds successfully on koji
[x] owns all directories created
[x] %makeinstall %clean rm -rf $RPM_BUILD_ROOT
[NA] dev, header file, static libs 


Optional suggestions:
[x] may be you could request upstream about including license file
[!] Description could be bit  better in my view.  

*You may like to ignore them*

Key NA = N/A, x = Check, ! = Problem, ? = Not evaluated

Comment 8 Neil Horman 2008-09-10 12:42:40 UTC
Thank you!

I've got a fixed up version of the package here, I'll be keeping the python-pysctp naming conventions, and I've verified that it builds on all arches.  I'll inquire upstream about the need for an explicit copy of the LGPLv2 license file shortly.  Thanks!

Comment 9 Neil Horman 2008-09-10 12:47:24 UTC
New Package CVS Request
=======================
Package Name: python-pysctp
Short Description: python bindings for the SCTP sockets extension api
Owners: nhorman
Branches: devel, F-9, EL-4, EL-4
InitialCC: nhorman

Comment 10 Jason Tibbitts 2008-09-11 00:05:19 UTC
You listed EL-4 twice.  I assumed you meant EL-5 for one of them.

InitialCC needs to be a Fedora account, but it is pointless to CC yourself on a package you own.  I've just skipped that line.

CVS done.


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