Bug 672205 - Review Request: pynag - Python Nagios plugin and configuration environment
Summary: Review Request: pynag - Python Nagios plugin and configuration environment
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-24 11:24 UTC by Tomas Edwardsson
Modified: 2012-05-26 22:08 UTC (History)
5 users (show)

Fixed In Version: pynag-0.4.1-6.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-05-13 01:53:47 UTC
Type: ---
j: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Tomas Edwardsson 2011-01-24 11:24:58 UTC
Spec URL: http://pynag.googlecode.com/svn/trunk/pynag.spec
SRPM URL: http://pynag.googlecode.com/files/pynag-0.3-1.src.rpm
Description: Python libraries for managing nagios configuration files and
writing python nagios plugins.

Comment 1 Tomas Edwardsson 2011-01-26 00:00:59 UTC
A couple of notes, I am a maintainer of pynag and I am also seeking a sponsor.

Comment 2 Tomas Edwardsson 2011-01-26 00:58:48 UTC
Rolled a new version according to input from rpmlint:

SRPM URL: http://pynag.googlecode.com/files/pynag-0.3-2.src.rpm

Comment 3 PRABIN KUMAR DATTA 2011-04-21 00:26:43 UTC
^ This is an Informal Review. ^

* MUST: rpmlint must be run on every package                           [Fix  ]

$ rpmlint -i SRPMS/pynag-0.3-2.src.rpm 
pynag.src: W: non-coherent-filename pynag-0.3-2.src.rpm pynag-0.3-2.fc13.src.rpm
The file which contains the package should be named
<NAME>-<VERSION>-<RELEASE>.<ARCH>.rpm.

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

$ rpmlint -i RPMS/noarch/pynag-0.3-2.fc14.noarch.rpm 
pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/remove_host_from_group.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/find_orphans.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/network_scan.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_contactgroup.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/parse_files.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_hostgroup.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_service_info.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/check_cpu.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_servicegroup.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/optimize_config.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/list_services.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/remove_host.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_command.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_service.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_contact.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_host.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/get_timeperiod.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/suggest_optimizations.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

pynag.noarch: W: spurious-executable-perm /usr/share/doc/pynag-0.3/examples/list_hosts_groups.py
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

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


* MUST: The package must be named according to the Package Naming    [Fix   ] 
  Guidelines
Check rpmlint output for pynag-0.3-2.src.rpm

* MUST: The spec file for the package MUST be legible.               [Fix   ] 


* Must: Spec file matches base package                               [OK   ] 

* Must: License must be licensed with a Fedora approved license and meet the
  Licensing Guidelines                                               [OK   ]

* Must: License in spec must match actual license                    [OK   ] 

* Must: License file included in %doc                                [OK   ]

* Must: Spec file written in American English                        [OK   ]

* Must: Tar ball matches upstream                                    [OK   ]

* Must: Package successfully builds binary RPMs                      [OK   ]
local build -(f14)
Koji build -(f15) http://koji.fedoraproject.org/koji/taskinfo?taskID=3014787

* Must: No duplicate files                                           [OK   ]

* Must: Macro use must be consistant                                 [OK   ]
Thou, for example:
%dir %{python_sitelib}/pynag
Can be written as
%dir %{python_sitelib}/%{name}

* Must: At the beginning of %install, each package MUST run rm -rf %{buildroot}
                                                                     [OK   ]

* Must: All filenames in rpm packages must be valid UTF-8            [OK   ]

* Other: %clean section and rm -fr $RPM_BUILD_ROOT are not required for F-13 and above. http://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

Comment 4 PRABIN KUMAR DATTA 2011-04-21 00:53:48 UTC
Also,
This is unnecessary, and in fact improper,: 
%doc AUTHORS README LICENSE CHANGES examples

It should be (^correct me if I am wrong^): 
%doc AUTHORS README LICENSE CHANGES
%doc examples

Comment 5 Tomas Edwardsson 2011-04-27 23:25:13 UTC
Thanks for the review, I broke up the examples into a seperate package, pynag-examples. New src.rpm at http://pynag.googlecode.com/files/pynag-0.3-3.src.rpm which should address all these issues.

I'm pretty sure the package conforms to naming, maybe a problem with running rpmlint against a src.rpm ?

Comment 6 Tomas Edwardsson 2011-08-10 18:38:24 UTC
New version released. http://pynag.googlecode.com/files/pynag-0.4-3.el6.src.rpm

Comment 7 Tomas Edwardsson 2012-01-19 20:51:53 UTC
Anything more I can do to move this along?

Comment 8 Matthias Runge 2012-03-02 20:08:24 UTC
Hi Tomas,

to get sponsored into the packager group, you must convince a sponsor to support you (or have somebody else to convince a sponsor....)

please read https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
and do some informal reviews.

You should note the bugzilla numbers here (as a reference for your sponsor).

Comment 9 Matthias Runge 2012-03-24 18:34:27 UTC
Any progress?

Comment 10 Tomas Edwardsson 2012-04-17 01:10:28 UTC
Hi Matthias

I've done 2 informal reviews on bug 800105 and 812121. Will look into doing more reviews in the next few days.

Comment 11 Tomas Edwardsson 2012-04-18 00:13:29 UTC
I ran some more tests and simplified the spec file removing unneeded %files lines amongst others.

Spec URL: http://pynag.googlecode.com/files/pynag.spec
SRPM URL: http://pynag.googlecode.com/files/pynag-0.4-4.fc16.src.rpm

Comment 12 Jason Tibbitts 2012-04-25 15:00:22 UTC
Wow, you've been at this for a while.  I'm looking over old review tickets and this one builds fine.   I'll work on this some more.

I'm going to assume that you actually intend to have this build for EL5; if not, the spec can be simplified considerably as EL6+ and all supported Fedora have rather simplified packaging requirements.

Comment 13 Tomas Edwardsson 2012-04-26 23:49:46 UTC
Yes it would be quite nice to have pynag available on EL5 as well.

Great to see someone looking into this, thanks :)

Comment 14 Jason Tibbitts 2012-05-07 23:28:36 UTC
I am very sorry for taking so long to get back to this, but here's a review.

Builds fine and rpmlint is silent.  Unfortunately there are a few problems:

Your package is noarch; there is no reason to define python_sitearch since you won't ever reference that macro.

The code is not GPLv2.  For example, Model/EventHandlers/__init__.py and Model/macros.py are GPLv3+.  The other code doesn't appear to even have any license statements; setup.py just says "GPL" and the LICENSE file explicitly says that if the code doesn't state a version, you can use any version you like.  So upstream (which I guess means you) really needs to clarify that.  It would be best to follow the GPL itself for that, since it tells you what to include in your source files, but at minimum you need to state somewhere what version of the GPL is in use.

When we see differing licenses on code we always wonder if the code comes from another project altogether, which would run afoul of https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries.  I'm not sure if that's the case since the copyright holder seems to be a committer on the project.  But if that's the case then the project seems to be a bit confused as to which license is supposed to be on its code, which raises other questions.

You should not in general have Requires: python; rpm should figure that out for itself.

You don't usually want to add compressed manpages; rpm will compress them properly using whatever compression method happens  to be preferred.  I guess upstream provides them compressed for some odd reason so there's not much you can do unless you want to uncompress them in the spec, which seems kind of pointless.

There is no need to duplicate all of the documentation in the -examples package.  It has a dependency on the main package so all of that documentation is guaranteed to be available.

The -examples package includes a README file which should be documentation.


* source files match upstream.  sha256sum:
  93d971e6f162d4bdaea6ab2735e9dbed1348d4bd64927e9bb1cb5fcca6dc2a54
   pynag-0.4.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summaries are OK.
* descriptions are OK.
* dist tag is present.
X license field does not appear to match the actual license.
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
X final provides:
  pynag-0.4-4.fc18.noarch.rpm
   pynag = 0.4-4.fc18
  =
   /usr/bin/python  
X  python >= 2.3
   python(abi) = 2.7

  pynag-examples-0.4-4.fc18.noarch.rpm
   pynag-examples = 0.4-4.fc18
  =
   /usr/bin/python  
   pynag  

? There might be bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
X documentation is duplicated between main and -examples packages.
* file permissions are appropriate.
* no generically named files.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Comment 15 Tomas Edwardsson 2012-05-09 01:39:32 UTC
Thanks Jason

python_sitearch removed.

Contacted authors and set license to GPLv2 everywhere.

There are no bundled libraries, just a commiter which was using the wrong license.

I removed the python-2.3 require, it was on a note that anyone trying to build it on older distributions would fail since we haven't tested it with older python.

I removed the docs from examples and added the examples/README file instead.

Also version 0.4.1 was just released, so this is a new version with updated spec file and src.rpm:

Spec URL: http://pynag.googlecode.com/git-history/0.4.1/pynag.spec
SRPM URL: http://pynag.googlecode.com/files/pynag-0.4.1-6.src.rpm

Also noting that there are 3 new scripts deployed, /usr/bin/pynag-* with relevant man pages.

Comment 16 Jason Tibbitts 2012-05-09 18:45:58 UTC
OK, the new package looks good.

APPROVED

One detail I forgot to mention previously relates to this line in %install:
  test "x$RPM_BUILD_ROOT" != "x" && rm -rf $RPM_BUILD_ROOT
It's really not necessary to do that and hasn't been for a really long time now.  More modern releases don't need any of that, but since you want to build on RHEL5, you just need:
  rm -rf $RPM_BUILD_ROOT

Anyway, this isn't particularly problematic; it's just a cleanliness issue.  I've already sponsored you into the packager group, so once those new permissions propagate through the system you'll be able to make your SCM request.

As your sponsor I'm here to help you through the rest of the process, so please don't hesitate to ask any questions you may have.  It is probably easiest to reach me via IRC on #fedora-devel; I see you've been there recently and any questions you ask can potentially be answered by anyone in the channel if I don't happen to be around.  Or you can contact me via email.

Comment 17 Tomas Edwardsson 2012-05-09 22:57:59 UTC
New Package SCM Request
=======================
Package Name: pynag
Short Description: Python Nagios plug-in and configuration environment
Owners: tommi
Branches: f16 el5 el6
InitialCC:

Comment 18 Jason Tibbitts 2012-05-09 23:04:21 UTC
Surely you'll want an f17 branch as well.

Comment 19 Tomas Edwardsson 2012-05-09 23:16:37 UTC
Yes, meant to put that in there as well...

Since this is automated I probably have to submit a Package Change Request to add f17?

Comment 20 Jason Tibbitts 2012-05-10 01:43:11 UTC
Yes, just paste in another.  Only the last one is processed.

Comment 21 Tomas Edwardsson 2012-05-10 08:15:22 UTC
New Package SCM Request
=======================
Package Name: pynag
Short Description: Python Nagios plug-in and configuration environment
Owners: tommi
Branches: f16 f17 el5 el6
InitialCC:

Comment 22 Gwyn Ciesla 2012-05-10 12:24:22 UTC
Git done (by process-git-requests).

Comment 23 Fedora Update System 2012-05-10 14:56:34 UTC
pynag-0.4.1-6.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/pynag-0.4.1-6.fc17

Comment 24 Fedora Update System 2012-05-10 16:01:27 UTC
pynag-0.4.1-6.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pynag-0.4.1-6.fc16

Comment 25 Fedora Update System 2012-05-10 16:02:46 UTC
pynag-0.4.1-6.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/pynag-0.4.1-6.el5

Comment 26 Fedora Update System 2012-05-10 16:03:33 UTC
pynag-0.4.1-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pynag-0.4.1-6.el6

Comment 27 Fedora Update System 2012-05-10 20:41:20 UTC
pynag-0.4.1-6.fc17 has been pushed to the Fedora 17 testing repository.

Comment 28 Fedora Update System 2012-05-13 01:53:47 UTC
pynag-0.4.1-6.fc16 has been pushed to the Fedora 16 stable repository.

Comment 29 Fedora Update System 2012-05-26 07:55:23 UTC
pynag-0.4.1-6.fc17 has been pushed to the Fedora 17 stable repository.

Comment 30 Fedora Update System 2012-05-26 22:06:04 UTC
pynag-0.4.1-6.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 31 Fedora Update System 2012-05-26 22:08:37 UTC
pynag-0.4.1-6.el5 has been pushed to the Fedora EPEL 5 stable repository.


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