Bug 705108 - Review Request: shinken - python monitoring tool
Summary: Review Request: shinken - python monitoring tool
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jorge Gallegos
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-05-16 16:42 UTC by David Hannequin
Modified: 2012-10-15 18:33 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-09-25 21:27:37 UTC
Type: ---
Embargoed:
kad: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Hannequin 2011-05-16 16:42:18 UTC
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 
SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6-1.fc15.src.rpm
Description: The main goal of Shinken is to allow users to have a fully flexible  architecture for their monitoring system that can easily scale to large environments.

Comment 1 Volker Fröhlich 2011-05-19 21:11:28 UTC
You can use the name macro in the tarball name.

The license is AGPLv3+. Rpmlint still complains, because the license was forgotten in the list of acceptable licenses. It is included by now.

Requiring a Python newer or equal to 2.6 is not necessary, as all Fedora versions have it. If you consider maintaining it in EPEL 5 as well, you must add a clean section and rm -rf the buildroot in the install section. In that case please also pick a BuildRoot syntax from http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag

If you don't want to maintain in EPEL, drop the buildroot definition. Besides that, FOR_PACKAGERS points out Python 2.4 would also work.

Rather use %global than %define.

defattr seems unnecessary now: https://fedorahosted.org/fpc/ticket/77
But if you use it, add the minus in the end: %defattr(-,root,root,-)

Drop the -n %{name}-%{version} from the setup macro. It's not necessary. You also don't have to define the __python macro.

It's good practice to backup patched files like: %patch1 -p1 -b .something_that_identifies_the_patch

Don't use the install macro. Just use the normal command. Probably put a few comments to make everything clear. Especially when you decide to do other than obvious.

Why do you need to have executable __init__.py (chmod)?

The unitdir macro is not defined. Thus a rebuild fails.

The paths inside FROM_NAGIOS_TO_SHINKEN don't reflect the situation for Fedora. Other files may be affected as well. Please check the paths in the config files, whether the targets exist and are sane. I can see /usr/local/...

README is only a symlink pointing towards README.rst -- no need to include it.

Any chance to run the unit tests?

Comment 2 David Hannequin 2011-05-20 13:20:41 UTC
Hi,

Unitdir macro is defined in the file /etc/rpm/macros.systemd. I corrected the other errors before putting a new spec file.

Best regard

Comment 3 David Hannequin 2011-05-24 21:54:18 UTC
Hi,

New spec file and SRPM :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 
SRPM URL:
http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-1.fc15.src.rpm

Best regard

Comment 4 Volker Fröhlich 2011-05-26 20:41:43 UTC
That means, unitdir is only defined in F15 and up. Thus all the buildroot stuff is either unnecessary, as you only want to support F15 and up or you have to find an alternative for these cases.

Please keep an accurate changelog and also address the other comments so far.

Comment 5 David Hannequin 2011-05-29 17:29:06 UTC
Hi,

I want to package Shinken only in Fedora 15 and higher. 

I now use %global instead of %define. 

There is no patch for this release but I keep in mind your remark about the - b of the patch command.

I removed the macro commands to install and others. 

FROM_NAGIOS_TO_SHINKEN the file is not included in the RPM. I do not understand your comment. The files are specific to Fedora fedora folder.

New spec file and SRPM :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 
SRPM URL:
http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-2.fc15.src.rpm

Best regard

Comment 6 Volker Fröhlich 2011-05-29 20:16:16 UTC
The buildroot definition is also unnecessary then.

Please take a look at the result of licensecheck:

...
shinken/contactdowntime.py: AGPL (v3 or later) 
shinken/bin.py: AGPL (v3 or later) 
shinken/db_oracle.py: AGPL (v3 or later) 
shinken/log.py: AGPL (v3 or later) 
...

All the files say AGPLv3+ -- not AGPLv3.

Why don't you include FROM_NAGIOS_TO_SHINKEN? Is it not relevant to users of Shinken?

With "changelog" I meant what's inside the spec file. Just by looking at the changelog "you fixed something and you replaced some macro". Just put it there like you put it above.

What about the unit tests?

Comment 7 David Hannequin 2011-05-29 20:46:20 UTC
Hi,

What do you mean "What about the unit tests?" ? Shinken RPMs work fine and i tested on Fedora 15. You can see -> http://hvad.fedorapeople.org/fedora/shinken/shinken.txt.

Best regard

Comment 8 David Hannequin 2011-05-30 15:55:10 UTC
Hi,

For now, there is no unit test in the rpm Shinken because the author did not create a test section in the file setup.py. After some discussion with him we'll add a test section in the file setup.py in the future version.

In addition there are tests that are already visible on this and realize url http://shinken-monitoring.de:8080/ and I also test the RPM version also works on Fedora 15.

Best regard

Comment 9 David Hannequin 2011-05-31 07:16:29 UTC
Hi,

New spec file and SRPM :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 
SRPM URL:
http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-3.fc15.src.rpm

Best regard

Comment 10 Volker Fröhlich 2011-06-24 22:08:51 UTC
Please adapt the Requires for the sub-packages as described here: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Put the common Requires up to the base package, instead of repeating them over and over for each sub-package.

Explicitly naming Python is not necessary, as, for instance, python-pyro requires it.

Putting the Requires on one line each is a bit more readable.

Eventually delete the useless BuildRoot or correct it according to: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag

The Python macro is useless, as you can see here: http://fedoraproject.org/wiki/Packaging:Python#Macros

Also use the python macro instead of "/usr/bin/python".

There are some occurrences of "shinken" in the files section, where you should use the name macro.

Your base package states "%{_libdir}/nagios/plugins". Nevertheless, it doesn't require Nagios and would also claim ownership of that directory, I suppose. Probably these files should go to one of your sub-packages?

Comment 11 David Hannequin 2011-07-01 21:28:00 UTC
Hi,

Requires in the sub packages have been removed and i putting the Requires on one line each.

After discussion with the developer of Shinken, Shinken plugins can be place on Nagios Plugins but it's possible to put in an other directory. 

Shinken was released in version 0.6.5. I still have tests to do before uploading the SRPM and SPEC.

Best regard

Comment 12 David Hannequin 2011-09-18 16:07:08 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 
SRPM URL:
http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.5-1.fc15.src.rpm

Best regard
ps : In a few months of Shinken version 0.8 will come out with a WebUI

Comment 13 David Hannequin 2011-11-15 20:03:17 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 

SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.8.1-1.fc15.src.rpm

Sub-package Shinken poller requires the Nagios plugins as it uses them to make checks.

Best regard

Comment 14 Jorge Gallegos 2012-04-02 19:02:31 UTC
(In reply to comment #11)
> Hi,
> 
> Requires in the sub packages have been removed and i putting the Requires on
> one line each.
> 
> After discussion with the developer of Shinken, Shinken plugins can be place on
> Nagios Plugins but it's possible to put in an other directory. 
> 
> Shinken was released in version 0.6.5. I still have tests to do before
> uploading the SRPM and SPEC.
> 
> Best regard

I think most of the issues have been worked out for this spec? Some comments:

 - I'd still change %{_libdir}/nagios to %{_libdir}/shinken, because that directory is already owned by the nagios package I think
 - You're sed'ing /usr/bin/python as the interpreter on several files, I would put /usr/bin/env python instead (not sure if shinken is python3 compatible or etc)
 - Volker was probably referring to that python_sitelib macro you have on top, check http://fedoraproject.org/wiki/Packaging:Python#Macros, since you stated you want shinken to be supported in fedora 15+: "In Fedora 13 and greater, the following macros are defined for you: ... snip... python_sitelib" 
 - Also, seems like shinken is up to v1.0.1 (http://www.shinken-monitoring.org/download/) so maybe you should update from upstream too?

Otherwise, very exciting! I'd like to see shinken in fedora some time soon!

Comment 15 David Hannequin 2012-04-09 13:41:08 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 

SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-1.0.1-1.fc16.src.rpm

I update from upstream and fix some issues.  

Sorry but shinken isn't compatible with python 3.

Best regard

Comment 16 Jorge Gallegos 2012-07-12 05:11:06 UTC
Huh. This is odd, I remember building this package without problems before, but I am now getting a permission error http://kad.fedorapeople.org/packages/shinken/build.log even when 

I assume it has to do with the version bump?

I see that you addressed the other issues in the spec file. My only gripe would be the use of the 'nagios' user/group for this, but since shinken and nagios can be interchangeable I tentatively say it is fine. Just look into the build issue (see log above).

Comment 17 David Hannequin 2012-07-22 13:57:07 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 

SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-1.0.1-2.fc17.src.rpm.

I add patch to fix build issue.

Nest regard

Comment 18 David Hannequin 2012-09-09 18:32:52 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 

SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-1.0.1-2.fc17.src.rpm.

I delete unnecessary require.

Nest regard

Comment 19 Jorge Gallegos 2012-09-09 20:51:19 UTC
I'm very sorry it took this long to respond, here's my official review.

This is the fedora-review output. Scroll down to the Issues section, briefly:
 - Put COPYING under %docs
 - Drop defattr in all %files sections as that is not needed anymore (unless 
   you are packaging for EPEL, which you aren't)

Regards

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[!]: EXTRA Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[ ]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files scheduler section. This is OK if
     packaging for EPEL5. Otherwise not needed
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[ ]: MUST Package contains desktop file if it is a GUI application.
[ ]: MUST Development files must be in a -devel package
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[ ]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[ ]: MUST Large documentation files are in a -doc subpackage, if required.
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[ ]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "*No copyright* AGPL (v3 or later)", "AGPL (v3 or
     later)", "MIT/X11 (BSD like)" For detailed output of licensecheck see
     file: /home/kad/Downloads/shinken/licensecheck.txt
[ ]: MUST License file installed when any subpackage combination is installed.
[ ]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[ ]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST No %config files under /usr.
[ ]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[ ]: MUST Package is not relocatable.
[ ]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[ ]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[!]: SHOULD Buildroot is not present
     Note: Invalid buildroot found: %{_tmppath}/%{name}-%{version}-buildroot
[x]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL5 is required
[ ]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Latest version is packaged.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[ ]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX tarball generation or download is documented.
[x]: SHOULD SourceX / PatchY prefixed with %{name}.
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files scheduler section. This is OK if
     packaging for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Rpmlint
-------
Checking: shinken-1.0.1-2.fc16.noarch.rpm
          shinken-arbiter-1.0.1-2.fc16.noarch.rpm
          shinken-reactionner-1.0.1-2.fc16.noarch.rpm
          shinken-scheduler-1.0.1-2.fc16.noarch.rpm
          shinken-poller-1.0.1-2.fc16.noarch.rpm
          shinken-broker-1.0.1-2.fc16.noarch.rpm
          shinken-receiver-1.0.1-2.fc16.noarch.rpm
          shinken-1.0.1-2.fc16.src.rpm
shinken.noarch: W: invalid-license AGPLv3+
shinken.noarch: W: non-standard-uid /var/run/shinken nagios
shinken.noarch: W: non-standard-gid /var/run/shinken nagios
shinken.noarch: W: non-standard-uid /var/lib/shinken nagios
shinken.noarch: W: non-standard-gid /var/lib/shinken nagios
shinken.noarch: W: non-standard-uid /var/log/shinken nagios
shinken.noarch: W: non-standard-gid /var/log/shinken nagios
shinken-arbiter.noarch: W: invalid-license AGPLv3+
shinken-reactionner.noarch: W: invalid-license AGPLv3+
shinken-scheduler.noarch: W: invalid-license AGPLv3+
shinken-poller.noarch: W: invalid-license AGPLv3+
shinken-broker.noarch: W: invalid-license AGPLv3+
shinken-receiver.noarch: W: invalid-license AGPLv3+
shinken.src: W: invalid-license AGPLv3+
8 packages and 0 specfiles checked; 0 errors, 14 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint shinken-arbiter shinken-scheduler shinken-broker
shinken-arbiter.noarch: I: enchant-dictionary-not-found en_US
shinken-arbiter.noarch: W: invalid-license AGPLv3+
shinken-scheduler.noarch: W: invalid-license AGPLv3+
shinken-broker.noarch: W: invalid-license AGPLv3+
3 packages and 0 specfiles checked; 0 errors, 3 warnings.
# echo 'rpmlint-done:'

Diff spec file in url and in SRPM
---------------------------------
--- /home/kad/Downloads/shinken.spec	2012-09-09 11:30:03.000000000 -0700
+++ /home/kad/Downloads/shinken/srpm-unpacked/shinken.spec	2012-09-09 13:36:05.334926188 -0700
@@ -2,5 +2,5 @@
 Name:           shinken
 Version:        1.0.1
-Release:        3%{?dist}
+Release:        2%{?dist}
 URL:            http://%{name}-monitoring.org
 Source0:        http://%{name}-monitoring.org/pub/%{name}-%{version}.tar.gz
@@ -12,4 +12,5 @@
 Requires:       python-pyro 
 Requires:       python-simplejson 
+Requires:       python-sqlite2
 Requires:       systemd-units
 Requires:       nmap 
@@ -415,7 +416,4 @@
 
 %changelog
-* Sun Sep 09 2012 David Hannequin <david.hannequin> - 1.0.1-3
-- Delete require python-sqlite2.
-
 * Sun Jul 22 2012 David Hannequin <david.hannequin> - 1.0.1-2
 - Add build patch. 
Requires
--------
shinken-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    /usr/bin/perl  
    /usr/bin/python  
    config(shinken) = 1.0.1-2.fc16
    nmap  
    perl(HTML::Entities)  
    perl(MIME::QuotedPrint)  
    perl(Mail::Sendmail) >= 0.75
    python  
    python(abi) = 2.7
    python-pyro  
    python-simplejson  
    python-sqlite2  
    sudo  
    systemd-units  

shinken-arbiter-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-2.fc16

shinken-reactionner-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-2.fc16

shinken-scheduler-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-2.fc16

shinken-poller-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    nagios-plugins-all  
    shinken = 1.0.1-2.fc16

shinken-broker-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    mysql-connector-python  
    python-memcached  
    python-redis  
    shinken = 1.0.1-2.fc16

shinken-receiver-1.0.1-2.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-2.fc16

Provides
--------
shinken-1.0.1-2.fc16.noarch.rpm:
    
    config(shinken) = 1.0.1-2.fc16
    shinken = 1.0.1-2.fc16

shinken-arbiter-1.0.1-2.fc16.noarch.rpm:
    
    shinken-arbiter = 1.0.1-2.fc16

shinken-reactionner-1.0.1-2.fc16.noarch.rpm:
    
    shinken-reactionner = 1.0.1-2.fc16

shinken-scheduler-1.0.1-2.fc16.noarch.rpm:
    
    shinken-scheduler = 1.0.1-2.fc16

shinken-poller-1.0.1-2.fc16.noarch.rpm:
    
    shinken-poller = 1.0.1-2.fc16

shinken-broker-1.0.1-2.fc16.noarch.rpm:
    
    shinken-broker = 1.0.1-2.fc16

shinken-receiver-1.0.1-2.fc16.noarch.rpm:
    
    shinken-receiver = 1.0.1-2.fc16

MD5-sum check
-------------
http://shinken-monitoring.org/pub/shinken-1.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : 811da51e80f2b32a2c6d0114ce90efe9e091aefd82489d21df35e0426f99fbfb
  CHECKSUM(SHA256) upstream package : 811da51e80f2b32a2c6d0114ce90efe9e091aefd82489d21df35e0426f99fbfb


Generated by fedora-review 0.2.2 (9f8c0e5) last change: 2012-08-09
Command line :/usr/bin/fedora-review -n shinken
External plugins:

Comment 20 David Hannequin 2012-09-10 20:25:12 UTC
Hi,

New spec file and SRPM with fix :
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec 

SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-1.0.1-4.fc17.src.rpm.

I delete unnecessary defattr and add COPYING.

Nest regard

Comment 21 Jorge Gallegos 2012-09-13 03:35:14 UTC
All right, I verified manually that COPYING exists in the final rpm, everything looks good to me. You now have to request an SCM Admin Request http://fedoraproject.org/wiki/Package_SCM_admin_requests and etc. You already know the drill. Good stuff! and thanks for bearing with me :)

Details:


Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[ ]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[ ]: MUST Package contains desktop file if it is a GUI application.
[ ]: MUST Development files must be in a -devel package
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[ ]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[ ]: MUST Large documentation files are in a -doc subpackage, if required.
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[ ]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "*No copyright* AGPL (v3 or later)", "AGPL (v3 or
     later)", "MIT/X11 (BSD like)" For detailed output of licensecheck see
     file: /home/kad/Downloads/shinken/licensecheck.txt
[ ]: MUST License file installed when any subpackage combination is installed.
[ ]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[ ]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST No %config files under /usr.
[ ]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[ ]: MUST Package is not relocatable.
[ ]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[ ]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[!]: SHOULD Buildroot is not present
     Note: Invalid buildroot found: %{_tmppath}/%{name}-%{version}-buildroot
[x]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL5 is required
[ ]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Latest version is packaged.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[ ]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX tarball generation or download is documented.
[x]: SHOULD SourceX / PatchY prefixed with %{name}.
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Rpmlint
-------
Checking: shinken-1.0.1-4.fc17.noarch.rpm
          shinken-arbiter-1.0.1-4.fc17.noarch.rpm
          shinken-reactionner-1.0.1-4.fc17.noarch.rpm
          shinken-scheduler-1.0.1-4.fc17.noarch.rpm
          shinken-poller-1.0.1-4.fc17.noarch.rpm
          shinken-broker-1.0.1-4.fc17.noarch.rpm
          shinken-receiver-1.0.1-4.fc17.noarch.rpm
          shinken-1.0.1-4.fc17.src.rpm
shinken.noarch: W: invalid-license AGPLv3+
shinken.noarch: W: non-standard-uid /var/run/shinken nagios
shinken.noarch: W: non-standard-gid /var/run/shinken nagios
shinken.noarch: W: non-standard-uid /var/lib/shinken nagios
shinken.noarch: W: non-standard-gid /var/lib/shinken nagios
shinken.noarch: W: non-standard-uid /var/log/shinken nagios
shinken.noarch: W: non-standard-gid /var/log/shinken nagios
shinken-arbiter.noarch: W: invalid-license AGPLv3+
shinken-arbiter.noarch: W: only-non-binary-in-usr-lib
shinken-reactionner.noarch: W: invalid-license AGPLv3+
shinken-reactionner.noarch: W: only-non-binary-in-usr-lib
shinken-scheduler.noarch: W: invalid-license AGPLv3+
shinken-scheduler.noarch: W: only-non-binary-in-usr-lib
shinken-poller.noarch: W: invalid-license AGPLv3+
shinken-poller.noarch: W: only-non-binary-in-usr-lib
shinken-broker.noarch: W: invalid-license AGPLv3+
shinken-broker.noarch: W: only-non-binary-in-usr-lib
shinken-receiver.noarch: W: invalid-license AGPLv3+
shinken-receiver.noarch: W: only-non-binary-in-usr-lib
shinken.src: W: invalid-license AGPLv3+
8 packages and 0 specfiles checked; 0 errors, 20 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint shinken-arbiter shinken-scheduler shinken-broker
shinken-arbiter.noarch: W: invalid-license AGPLv3+
shinken-arbiter.noarch: W: only-non-binary-in-usr-lib
shinken-scheduler.noarch: W: invalid-license AGPLv3+
shinken-scheduler.noarch: W: only-non-binary-in-usr-lib
shinken-broker.noarch: W: invalid-license AGPLv3+
shinken-broker.noarch: W: only-non-binary-in-usr-lib
3 packages and 0 specfiles checked; 0 errors, 6 warnings.
# echo 'rpmlint-done:'

Requires
--------
shinken-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    /usr/bin/perl  
    /usr/bin/python  
    config(shinken) = 1.0.1-4.fc17
    nmap  
    perl(HTML::Entities)  
    perl(MIME::QuotedPrint)  
    perl(Mail::Sendmail) >= 0.75
    python  
    python(abi) = 2.7
    python-pyro  
    python-simplejson  
    sudo  
    systemd-units  

shinken-arbiter-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-4.fc17

shinken-reactionner-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-4.fc17

shinken-scheduler-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-4.fc17

shinken-poller-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    nagios-plugins-all  
    shinken = 1.0.1-4.fc17

shinken-broker-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    mysql-connector-python  
    python-memcached  
    python-redis  
    shinken = 1.0.1-4.fc17

shinken-receiver-1.0.1-4.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    /usr/bin/env  
    shinken = 1.0.1-4.fc17

Provides
--------
shinken-1.0.1-4.fc17.noarch.rpm:
    
    config(shinken) = 1.0.1-4.fc17
    shinken = 1.0.1-4.fc17

shinken-arbiter-1.0.1-4.fc17.noarch.rpm:
    
    shinken-arbiter = 1.0.1-4.fc17

shinken-reactionner-1.0.1-4.fc17.noarch.rpm:
    
    shinken-reactionner = 1.0.1-4.fc17

shinken-scheduler-1.0.1-4.fc17.noarch.rpm:
    
    shinken-scheduler = 1.0.1-4.fc17

shinken-poller-1.0.1-4.fc17.noarch.rpm:
    
    shinken-poller = 1.0.1-4.fc17

shinken-broker-1.0.1-4.fc17.noarch.rpm:
    
    shinken-broker = 1.0.1-4.fc17

shinken-receiver-1.0.1-4.fc17.noarch.rpm:
    
    shinken-receiver = 1.0.1-4.fc17

MD5-sum check
-------------
http://shinken-monitoring.org/pub/shinken-1.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : 811da51e80f2b32a2c6d0114ce90efe9e091aefd82489d21df35e0426f99fbfb
  CHECKSUM(SHA256) upstream package : 811da51e80f2b32a2c6d0114ce90efe9e091aefd82489d21df35e0426f99fbfb


Generated by fedora-review 0.2.2 (9f8c0e5) last change: 2012-08-09
Command line :/usr/bin/fedora-review -n shinken -m fedora-17-x86_64
External plugins:

Comment 22 David Hannequin 2012-09-15 09:30:08 UTC
New Package SCM Request
=======================
Package Name: shinken
Short Description: python monitoring tool
Owners: hvad
Branches: f17 f18 el6
InitialCC:

Comment 23 Gwyn Ciesla 2012-09-15 14:45:42 UTC
Git done (by process-git-requests).

Comment 24 Michael S. 2012-09-23 10:23:33 UTC
Jorge, you should not approve a review if you do not check everything. Fedora-review is not a tool that does everything by magic, and you forgot to fill several fields.

The rpmlint error abou wrong license is also something to look at.

Comment 25 Mohamed El Morabity 2012-09-23 11:22:30 UTC
(In reply to comment #24)
Some points to rework:

- Requiring python is useless, since python-piro and python-sumplejson already require Python. By the way, requiring python2 is better than simply python, in case the default Python version changes in Fedora.

-If you provide an external man page (see Source1), please specify where it comes from.

- All the Requires(post/postun/preun) on systemd-units are missing from all subpackages providing services:
  http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Manual_scriptlets_.28Fedora_17_or_older.29

- Making executable all Python files installed in %{python_sitelib} is not the right way to fix all rpmlint issues you got about them:
  https://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Remove_shebang_from_Python_libraries
  I suggest you to drop all "chmod +x/sed..."/"sed -i -e '1d;2i#!/usr/bin/env python' ..." lines by:

   for lib in $(find %{buildroot}%{python_sitelib}/%{name}/ -name "*.py" ); do
    sed '/\#!\/usr\/bin\//d' $lib > $lib.new &&
    touch -r $lib $lib.new &&
    mv $lib.new $lib
   done

- shinken is noarch, setting CFLAGS in %build is useless.

- Please drop echo printing in %pre when creating user/group. RPM installations are supposed to be quiet.

- I really don't feel comfortable with the Requires on sudo (see nmap_discovery_runner.py). Without any guidelines about this, you should address this issue to fedora-devel to get better advice.

Comment 26 Jorge Gallegos 2012-09-24 20:31:35 UTC
My apologies. David, in the face of way more experienced reviewers I defer to them with a couple of comments:
 - About licensing, I believe it got discussed in Comment 6 up there.
 - About systemd, it is probably a good idea to replace the scriptlets with the presets as discussed in https://bugzilla.redhat.com/show_bug.cgi?id=850016

Regards

(In reply to comment #24)
> Jorge, you should not approve a review if you do not check everything.
> Fedora-review is not a tool that does everything by magic, and you forgot to
> fill several fields.
> 
> The rpmlint error abou wrong license is also something to look at.

Comment 27 Fedora Update System 2012-09-25 21:50:45 UTC
shinken-1.0.1-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/shinken-1.0.1-5.fc17

Comment 28 Fedora Update System 2012-09-25 21:52:38 UTC
shinken-1.0.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/shinken-1.0.1-5.fc18

Comment 29 Fedora Update System 2012-10-15 18:33:22 UTC
shinken-1.0.1-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/shinken-1.0.1-5.el6


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