Bug 442377 - Review Request: python-pysctp - Python binding for the SCTP network protocol
Review Request: python-pysctp - Python binding for the SCTP network protocol
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rakesh Pandit
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-14 12:12 EDT by Neil Horman
Modified: 2008-09-11 13:21 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-11 13:21:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rpandit: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Neil Horman 2008-04-14 12:12:42 EDT
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 12:17:46 EDT
ping, can someoneae please review this?  Thanks!
Comment 2 Rakesh Pandit 2008-09-09 14:33:21 EDT
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 14:34:26 EDT
You should include doc files like draft in doc folder, README (it has some essential info) ChangeLog
Comment 4 Neil Horman 2008-09-09 16:30:52 EDT
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 01:18:57 EDT
Your link to spec has old spec file. I am using srpm which has latest spec though :)
Comment 6 Rakesh Pandit 2008-09-10 01:26:49 EDT
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 02:37:22 EDT
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 08:42:40 EDT
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 08:47:24 EDT
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@redhat.com
Comment 10 Jason Tibbitts 2008-09-10 20:05:19 EDT
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.