Bug 754137 - Review Request: nagios-plugins-bdii - Nagios Probe for the BDII
Summary: Review Request: nagios-plugins-bdii - Nagios Probe for the BDII
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-15 14:00 UTC by Laurence Field
Modified: 2014-09-30 12:17 UTC (History)
4 users (show)

Fixed In Version: nagios-plugins-bdii-1.0.14-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-22 22:45:01 UTC
steve.traylen: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Laurence Field 2011-11-15 14:00:53 UTC
Spec URL: http://lfield.web.cern.ch/lfield/nagios-plugins-bdii.spec
SRPM URL: lfield.web.cern.ch/lfield/nagios-plugins-bdii-1.0.11-1.el5.src.rpm
Description: Nagios Probe for the BDII (Already in Fedora/EPEL)

Comment 1 Laurence Field 2011-11-15 14:01:38 UTC
Steve, Please could you take a look at this package too.  Thanks :)

Comment 2 Steve Traylen 2011-11-15 15:38:37 UTC
Please remember to add the 	 FE-NEEDSPONSOR block above until you have  a sponsor.

Steve.

Comment 3 Steve Traylen 2011-11-15 21:25:10 UTC
Quick first pass:

1) rpmlint of the .spec file

./nagios-plugins-bdii.spec:24: W: rpm-buildroot-usage %build make compile prefix=%{buildroot}

It's odd that the build depends upon the buildroot. The build root is temporary
location only used during the build and not in the final package, does 
the build and compile really depend upon this?

./nagios-plugins-bdii.spec: W: no-cleaning-of-buildroot %install

Do you plan to target EPEL5 as well? in which case your %install needs a clean.

2) The nagios-plugins-bdii-1.0.11-1.fc16.x86_64.rpm package contains just
one file.

/usr/lib64/nagios/plugins/contrib/check_bdii_entries

You don't require anything that contains or create the contrib directory yourself, see
http://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership

more over I see no reason why this is in crontrib, no other nagios sensors
appear to be, please can you be consistent to nagios-plugins which must
be the reference I would guess.

2.5) Having absolutely no documentation is odd. Add a README.Fedora or ask
     upstream to include a README at the very least.

3)  Url: vs URL:
URL: is more conventional and it's an acronym after all.

4)The SPEC file says ASL 2.0 but there is no indication as to any 
  license within the source code as far as I can see.
  Contact upstream and ask them to add 
  a license or at the very least have them clarify the license,
  if that's all they do include the email reply.

5) BuildRequires: openldap-devel
Include the architecture you require as well to avoid pointless package
installs during resolution, ie.

BuildRequires: openldap-devel%{?_isa}

6) Requires: openldap
is not required since this is determined automatically and 

7) The CFLAGS are incorrect:
/usr/bin/gcc -Wall -O2 -g -DLDAP_DEPRECATED -lldap src/check_bdii_entries.c -o /builddir/build/BUILD/nagios-plugins-bdii-1.0.11/build/check_bdii_entries

see 
http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Comment 4 Laurence Field 2011-11-16 09:51:09 UTC
Here is a new version

Spec URL: http://lfield.web.cern.ch/lfield/nagios-plugins-bdii.spec
SRPM URL: lfield.web.cern.ch/lfield/nagios-plugins-bdii-1.0.12-1.el5.src.rpm

Changes:

1) I have removed the use of buildroot for the compile and now remove the buildroot before the install as I am also targeting EPEL5.

2) I have put the plugin in /usr/lib64/nagios/plugins/ and added this directory to the package. I am now consistent with other nagios plugins.

rpm -ql nagios-plugins-ldap
/usr/lib64/nagios/plugins/check_ldap
/usr/lib64/nagios/plugins/check_ldaps


2.5) Having absolutely no documentation is odd. This is consistent with other plugins (see above).

3)  I have changed Url to URL in the specfile.

4) Upstream has added a license header to the source resulting in a new version.

5) BuildRequires is now openldap-devel%{?_isa}

6) Requires: openldap has been removed from the specfile.

7) The setting of CFLAGS has been removed.

Comment 5 Steve Traylen 2011-11-16 11:07:30 UTC
Add a protocol to your SRPM URL next time, easier to copy paste.

Please reread 
http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

CFLAGS is not meant to be unset.
Steve

Comment 6 Laurence Field 2011-11-16 11:26:34 UTC
I didn't unset it, just removed the setting of it in the Makefile. It how uses the one from the default environment.

Comment 7 Steve Traylen 2011-11-16 12:13:39 UTC
Look at some other examples of CFLAGS being set within Fedora .spec files. e.g

http://pkgs.fedoraproject.org/gitweb/?p=dpm-dsi.git;a=blob;f=dpm-dsi.spec;h=47e8319c1de19e2b98ee8c546f1a2e9f0bb6310b;hb=HEAD

Steve.

Comment 8 Laurence Field 2011-11-16 12:49:21 UTC
Sorry, I misunderstood. I have uploaded a new specfile.

Comment 9 Steve Traylen 2011-11-16 12:53:36 UTC
Please actually check the resulting compiler flags being used rather than edit and hope.

They have not changed.

/usr/bin/gcc  -lldap src/check_bdii_entries.c -o /afs/cern.ch/user/s/straylen/rpmbuild/BUILD/nagios-plugins-bdii-1.0.12/build/che
ck_bdii_entries

Comment 10 Laurence Field 2011-11-16 14:49:39 UTC
I have updated the specfile. Here is the result from the test.

rpmbuild --rebuild build/SRPMS/nagios-plugins-bdii-1.0.12-1.el5.src.rpm  2>/dev/null | grep gcc
/usr/bin/gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -lldap src/check_bdii_entries.c -o /usr/src/redhat/BUILD/nagios-plugins-bdii-1.0.12/build/check_bdii_entries

Comment 11 Steve Traylen 2011-11-22 21:41:16 UTC
Having added the 

%dir %{_libdir}/nagios/plugins/

directory to your package the same story is true for

%{_libdir}/nagios

I have since learnt that the nagios-common package is the trick here.

$ rpm -ql nagios-plugins

contains

/usr/lib64/nagios/plugins

and you should drop 
%dir %{_libdir}/nagios/plugins/
and instead require this package.

This package is a common package for all nagios plugins and contains
very little.

Steve.

Comment 12 Laurence Field 2011-11-29 14:39:14 UTC
I have updated the specfile so that the package requires nagios-common and the %dir line has been removed. A mistake in the description was also addressed. (Was pointed out by A. Elwell)

Comment 13 Steve Traylen 2011-11-29 18:51:18 UTC
Please provide a link to a new RPM and .spec file with increased release number.

Steve.

Comment 14 Laurence Field 2011-11-30 09:05:56 UTC
Here is a new version with an increased release number

Spec URL: http://lfield.web.cern.ch/lfield/nagios-plugins-bdii.spec
SRPM URL: http://lfield.web.cern.ch/lfield/nagios-plugins-bdii-1.0.12-2.el5.src.rpm

Comment 15 Steve Traylen 2011-12-01 00:08:01 UTC
1)

Changelog does not match release, and the spec file has changed 
with no corresponding changelog entry reflecting the change made in the
.spec file.

Please run rpmlint at every release:

$ rpmlint ../RPMS/x86_64/nagios-plugins-bdii-*
nagios-plugins-bdii.x86_64: W: incoherent-version-in-changelog 1.0.12-1.el6 ['1.0.12-2.el6', '1.0.12-2']
nagios-plugins-bdii.x86_64: W: no-documentation

2)
Can you take a look at or report upstream the compilation warnings,
not a requirement but worth following up.

rc/check_bdii_entries.c: In function 'main':
src/check_bdii_entries.c:168: warning: implicit declaration of function 'ldap_init'
src/check_bdii_entries.c:168: warning: assignment makes pointer from integer without a cast
....

2)
The %description is rather terse and includes BDII which is not described
anywhere and is not obvious to people in general I believe.

3)
The  tar ball in the .src.rpm does not match the instructions to create it.

Comment 16 Steve Traylen 2011-12-01 00:11:49 UTC
Can you at least request that upstream adds a LICENSE file to their tar
ball please.

http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text


Steve.

Comment 17 Laurence Field 2011-12-02 15:33:28 UTC
Spec URL: http://lfield.web.cern.ch/lfield/nagios-plugins-bdii.spec
SRPM URL:
http://lfield.web.cern.ch/lfield/nagios-plugins-bdii-1.0.13-1.el5.src.rpm

Upstream have provided a new version (1.0.13) which addresses the compilation warnings and includes a LICENSE file in the source. The description has also been changed so that it is consistent with the other nagios-plugin packages.
The tar ball in the srpm should now be identical what is obtained using the instructions specified in the spec file.

Comment 18 Laurence Field 2011-12-02 15:37:35 UTC
Sorry wrong link.

SRPM URL:
http://lfield.web.cern.ch/lfield/nagios-plugins-bdii-1.0.13-1.el6.src.rpm

Comment 19 Laurence Field 2011-12-05 15:50:57 UTC
Scratch build result

http://koji.fedoraproject.org/koji/taskinfo?taskID=3563946

Comment 20 Steve Traylen 2011-12-07 18:34:08 UTC
Source matches after s/_1_0_12/_1_0_13/

 svn export http://svnweb.cern.ch/guest/gridinfo/nagios-plugins-bdii/tags/R_1_0_13 nagios-plugins-bdii-1.0.13


I believe the %description should be longer but it is not a requirement.



Check it basically runs:

/usr/lib64/nagios/plugins/check_bdii_entries
Error: Hostname ( -H option ) not specified.

Usage: check_bdii_entries -H host [OPTION] 

Options: 
	-p	The port of the BDII
	-b	The base of the BDII
	-f	The filter to use for the search
	-f	Note: Shortcuts 'site', 'service' and 'stats' can be


Fine

Only problem

As there is now a license file please include it :

https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Please fix the above before import.


PACKAGE APPROVED

Comment 21 Laurence Field 2011-12-08 10:37:29 UTC
The description is consistent with other nagios plugins

rpm -qli nagios-plugins-ldap 
Description: Provides check_ldap support for Nagios.

rpm -qli nagios-plugins-http
Description: Provides check_http support for Nagios.

Comment 23 Laurence Field 2011-12-12 11:13:20 UTC
New Package SCM Request
=======================
Package Name: nagios-plugins-bdii
Short Description: Nagios Plugin - check_bdii_entries
Owners: lfield
Branches: f16 el5 el6
InitialCC:

Comment 24 Gwyn Ciesla 2011-12-12 13:45:25 UTC
Git done (by process-git-requests).

Comment 25 Fedora Update System 2011-12-12 15:20:52 UTC
nagios-plugins-bdii-1.0.14-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/nagios-plugins-bdii-1.0.14-1.fc16

Comment 26 Fedora Update System 2011-12-12 15:26:55 UTC
nagios-plugins-bdii-1.0.14-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/nagios-plugins-bdii-1.0.14-1.el5

Comment 27 Fedora Update System 2011-12-12 15:32:48 UTC
nagios-plugins-bdii-1.0.14-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/nagios-plugins-bdii-1.0.14-1.el6

Comment 28 Fedora Update System 2011-12-12 19:26:57 UTC
nagios-plugins-bdii-1.0.14-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 29 Fedora Update System 2011-12-22 22:45:01 UTC
nagios-plugins-bdii-1.0.14-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 30 Fedora Update System 2011-12-29 22:00:00 UTC
nagios-plugins-bdii-1.0.14-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 31 Fedora Update System 2011-12-29 22:01:31 UTC
nagios-plugins-bdii-1.0.14-1.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 32 Steve Traylen 2014-09-30 11:20:53 UTC
Package Change Request
======================
Package Name: nagios-plugins-bdii
New Branches: epel7
Owners: stevetraylen

Comment 33 Gwyn Ciesla 2014-09-30 12:17:50 UTC
Git done (by process-git-requests).


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