Bug 603521 - (pynetsnmp) Review Request: pynetsnmp - Python ctypes bindings for NET-SNMP with Twisted integration
Review Request: pynetsnmp - Python ctypes bindings for NET-SNMP with Twisted ...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Ian Weller
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-13 13:08 EDT by Nathaniel McCallum
Modified: 2010-07-03 12:02 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-03 12:02:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ian: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nathaniel McCallum 2010-06-13 13:08:07 EDT
Spec URL: http://nathaniel.themccallums.org/rpms/pynetsnmp.spec
SRPM URL: http://nathaniel.themccallums.org/rpms/pynetsnmp-0.28.14-1.fc13.src.rpm
Description: Python ctypes bindings for NET-SNMP with Twisted integration
pynetsnmp is a set of Python ctypes binding for NET-SNMP, an
implementation of the Simple Network Management Protocol (SNMP).

pynetsnmp is a replacement for the various Python bindings provided by
PySNMP* implementations.  It also implements a glue with the Python
Twisted Matrix networking framework which replaces the TwistedSNMP
implementation.

Will be reviewed by Ian Weller.
Comment 1 Chen Lei 2010-06-14 00:53:10 EDT
I think this package should be renamed to python-pynetsnmp.


The namespace for this package is pynetsnmp rather than netsnmp, thus it'll much better to add python before module names.


Most py* packages should keep py* as its package name because the namespace for those modules normally don't include py.
Comment 2 Ian Weller 2010-06-14 09:14:43 EDT
I disagree. python-py* is redundant. The packaging guidelines also state:

"If the upstream source has "py" (or "Py") in its name, you can use that name for the package."

Users can run an rpm -ql to see what the namespace of the package is after the fact.
Comment 3 Ian Weller 2010-06-14 13:34:52 EDT
[  OK  ] specfiles match:
  b91de570b8543638cbb5509ec3df554e  pynetsnmp.spec
  b91de570b8543638cbb5509ec3df554e  pynetsnmp-0.28.14-1.fc13.src/pynetsnmp.spec
[  OK  ] source files match upstream:
[  OK  ] package meets naming and versioning guidelines.
[  OK  ] spec is properly named, cleanly written, and uses macros consistently.
  (Is there a more specific link than zenoss.com for pynetsnmp?)
[  OK  ] dist tag is present.
[  OK  ] build root is correct.
[  OK  ] license field matches the actual license.
[  OK  ] license is open source-compatible.
[  OK  ] license text included in package.
[  OK  ] latest version is being packaged.
[  OK  ] BuildRequires are proper.
[  N/A ] compiler flags are appropriate.
[  OK  ] %clean is present. 
[  OK  ] package builds in mock.
[  OK  ] package installs properly.
[  N/A ] debuginfo package looks complete.
[  OK  ] rpmlint is silent.
  (ignoring spelling warnings)
[FAILED] final provides and requires are sane
  pylint reports that this module depends on Twisted, but the package does not
  require it.
[  N/A ] %check is present and all tests pass:
[  N/A ] no shared libraries are added to the regular linker search paths.
[  OK  ] owns the directories it creates. 
[  OK  ] doesn't own any directories it shouldn't.
[  OK  ] no duplicates in %files.
[  OK  ] file permissions are appropriate.
[  N/A ] scriptlets match those on ScriptletSnippets page.
[  OK  ] code, not content.
[  OK  ] documentation is small, so no -docs subpackage is necessary.
[  OK  ] %docs are not necessary for the proper functioning of the package.
[  OK  ] no headers.
[  OK  ] no pkgconfig files.
[  OK  ] no libtool .la droppings.
[  N/A ] desktop files valid and installed properly.

Take a look at the package requires.
Comment 4 Chen Lei 2010-06-14 13:58:58 EDT
(In reply to comment #2)
> I disagree. python-py* is redundant. The packaging guidelines also state:
> "If the upstream source has "py" (or "Py") in its name, you can use that name
> for the package."
> Users can run an rpm -ql to see what the namespace of the package is after the
> fact.    

I know what naming guideline states, I don't intend to oppose this review request. actually the naming of python modules in fedora is a bit inconsistency.


yum list python-py\* , you'll find it also exists many packages. Since most py* modules in pypi don't include py in their namespace, excluding python in package name is reasonable under most circumstance, things became much worse when you look at *py or *python packages in fedora.

I'll suggest python-SIG to state more clearly on python modules naming scheme.
Comment 5 Nathaniel McCallum 2010-06-14 14:25:13 EDT
If this package were to be titled something other than pynetsnmp, I would think it would be python-netsnmp (stripping off the 'py').

In any case, the pynetsnmp spec and srpm are updated (same URL).  I created a -twisted subpackage with the proper deps for the portion of the library that has twisted integration.  I also added an explicit dep on net-snmp-libs since pynetsnmp calls it via ctypes.
Comment 6 Ian Weller 2010-06-14 14:30:36 EDT
On the naming: if anything, the package should match the upstream tarball name.

Nathan, you need to bump the release number, add a new changelog entry at the bottom for what you changed, and upload a new SRPM with each iteration of a package review.
Comment 8 Chen Lei 2010-06-14 14:49:55 EDT
(In reply to comment #5)
> If this package were to be titled something other than pynetsnmp, I would think
> it would be python-netsnmp (stripping off the 'py').

The naming guideline for python modules is ambiguous, so the name of py* python modules in fedora are a bit inconsistency. Currently both pynetsnmp and python-pynetsnmp is accetptable, this won't be an issue until FPC decides to modify the naming guideline. 

See http://fedoraproject.org/wiki/PackageNamingGuidelines#Addon_Packages_.28python_modules.29

Packages of python modules should take into account the upstream name of the python module. This makes a package name format of python-$NAME. When in doubt, use the name of the module that you type to import it in a script.  

There is an exception to this rule. If the upstream source has "py" (or "Py") in its name, you can use that name for the package. So, for example, pygtk is acceptable.
Comment 9 Ian Weller 2010-06-14 23:04:20 EDT
Re-review summary:

[FAILED] license text included in package.
  COPYRIGHT.txt and LICENSE.txt need to be included in the subpackage as well.
[FAILED] rpmlint is silent.
  pynetsnmp.noarch: E: explicit-lib-dependency net-snmp-libs
      (this one can be ignored because it is required and rpm doesn't
      automatically generate this dependency)
  pynetsnmp.src:33: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line
      33)
  pynetsnmp-twisted.noarch: W: no-documentation
      (solved by adding COPYRIGHT.txt and LICENSE.txt to -twisted)
  (spelling warnings removed)
[  OK  ] final provides and requires are sane
[FAILED] owns the directories it creates. 
  pynetsnmp needs to own %{python_sitelib}/pynetsnmp. You can make this happen
  by adding this to your %files list:
      %dir %{python_sitelib}/pynetsnmp
Comment 10 Nathaniel McCallum 2010-06-14 23:16:14 EDT
Updated.
Comment 11 Ian Weller 2010-06-14 23:57:04 EDT
Assuming http://nathaniel.themccallums.org/rpms/pynetsnmp.spec and http://nathaniel.themccallums.org/rpms/pynetsnmp-0.28.14-3.fc13.src.rpm ...

[  OK  ] license text included in package.
[FAILED] rpmlint is silent.
  pynetsnmp.src:33: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 33)
  (this still needs fixed)
[  OK  ] owns the directories it creates. 

I'm not going to make you reupload a new SRPM just for spacing, but be sure it's fixed before it's checked into CVS.

--------------------------------------
 This package (pynetsnmp) is APPROVED
--------------------------------------
however, is still blocking on sponsorship.
Comment 12 Nathaniel McCallum 2010-06-19 14:54:29 EDT
New Package CVS Request
=======================
Package Name: pynetsnmp
Short Description: Python ctypes bindings for NET-SNMP with Twisted integration
Owners: npmccallum
Branches: F-12 F-13
InitialCC:
Comment 13 Kevin Fenzi 2010-06-20 22:25:36 EDT
CVS done (by process-cvs-requests.py).

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