Bug 743615

# No binaries here, do not build a debuginfo package
%global debug_package %{nil}

Why is not use BuildArch: noarch?


URL:           http://folk.uio.no/trondham/software/%{plugin}.html
Source0:       http://folk.uio.no/trondham/software/files/%{plugin}-%{version}.tar.gz

I don't see the value in using the %{plugin} macro here.

BuildRequires: /usr/bin/pod2man

Well, simply using BuildRequires: perl should be safe and faster?

Requires:      perl(Config::Tiny)
Requires:      perl(Net::SNMP)
Requires:      perl(Crypt::Rijndael)

Please let rpm find these.

You will find this page useful: 

http://fedoraproject.org/wiki/Packaging:Perl

Provides:      nagios-plugins-check-openmanage = %{version}-%{release}
Obsoletes:     nagios-plugins-check-openmanage < 3.7.2-3

Please explain the need for these lines.

%{nagiospluginsdir}/*
%{_mandir}/man8/*.8*
%{_mandir}/man5/*.5*
%dir %{_sysconfdir}/nagios
%config(noreplace) %{_sysconfdir}/nagios/*

1)I like more explicit file list, however that's me.
2) I guess %dir %{_sysconfdir}/nagios is owned by nagios-plugins
and not needed in this package?

Would be nice if you could create a FAS account and do a koji scratch build.

(In reply to comment #1)
> # No binaries here, do not build a debuginfo package
> %global debug_package %{nil}
> 
> Why is not use BuildArch: noarch?

Because I use the %{_libdir} macro. As far as I can see, the placement for Nagios plugins in Fedora/EPEL is /usr/lib64/nagios/plugins and /usr/lib/nagios/plugins for 64bit and 32bit arches, respectively. If not for this, it would be a noarch package.

> URL:           http://folk.uio.no/trondham/software/%{plugin}.html
> Source0:      
> http://folk.uio.no/trondham/software/files/%{plugin}-%{version}.tar.gz
> 
> I don't see the value in using the %{plugin} macro here.

Mostly cosmetic reasons. I couldn't use %{name} :)

> BuildRequires: /usr/bin/pod2man
> 
> Well, simply using BuildRequires: perl should be safe and faster?

Maybe. If it's safer/better/preferred to use package names instead in BuildRequires I'll change it.

> Requires:      perl(Config::Tiny)
> Requires:      perl(Net::SNMP)
> Requires:      perl(Crypt::Rijndael)
> 
> Please let rpm find these.

No, rpmbuild doesn't find these. The first two aren't found because they're only invoked if the user requests a certain feature via options, i.e. there are no "use Foo::Bar" that rpmbuild would find. The last one is only needed if the user wishes to use SNMPv3 with AES. Neither the plugin nor Net::SNMP requires it.

> You will find this page useful: 
> 
> http://fedoraproject.org/wiki/Packaging:Perl

Thanks.

> Provides:      nagios-plugins-check-openmanage = %{version}-%{release}
> Obsoletes:     nagios-plugins-check-openmanage < 3.7.2-3
> 
> Please explain the need for these lines.

I have for a long time supplied RPM packages for download (not part of a repo). These lines are included to make the transition to Fedora/EPEL packages easier for existing users of the RPM packages.

> %{nagiospluginsdir}/*
> %{_mandir}/man8/*.8*
> %{_mandir}/man5/*.5*
> %dir %{_sysconfdir}/nagios
> %config(noreplace) %{_sysconfdir}/nagios/*
> 
> 1)I like more explicit file list, however that's me.
> 2) I guess %dir %{_sysconfdir}/nagios is owned by nagios-plugins
> and not needed in this package?

No, %{_sysconfdir}/nagios is not owned by any package that this package requires.

> Would be nice if you could create a FAS account and do a koji scratch build.

Yes, I will.

Thanks for your thoughts and comments :)

(In reply to comment #2)
> (In reply to comment #1)
> > Would be nice if you could create a FAS account and do a koji scratch build.
> 
> Yes, I will.

Any news here? :)

As you are searching for a sponsor and this is your only review request so far, have you done some informal review requests yet?

For more information on how to get sponsored see:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Submitting_quality_new_packages

As my attention was needed elsewhere lately, this was pushed to the background and I apologize for that. In the interest in moving things forward, I have addressed most of the issues in comment #1 and uploaded new versions of the spec file and SRPM here:

http://folk.uio.no/trondham/review2/nagios-plugins-openmanage.spec
http://folk.uio.no/trondham/review2/nagios-plugins-openmanage-3.7.3-2.el6.src.rpm

I have done a scratch build as requested:

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

I also include results from rpmlint for your convenience:

$ rpmlint nagios-plugins-openmanage.spec nagios-plugins-openmanage-3.7.3-2.el6.src.rpm 
nagios-plugins-openmanage.src: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.src: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
1 packages and 1 specfiles checked; 0 errors, 2 warnings.

In reply to comment #3, I'm afraid that I haven't done much of informal review requests yet, but I'll get to it as I'm still in need of a sponsor.

I forgot rpmlint on the binary RPM:

$ rpmlint nagios-plugins-openmanage-3.7.3-2.el6.x86_64.rpm 
nagios-plugins-openmanage.x86_64: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.x86_64: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.x86_64: E: no-binary
nagios-plugins-openmanage.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 3 warnings.

The last two are normal for Nagios plugins, as the actual plugins are placed under %{_libdir} even if they are architecture independent.

Review:

- name ok
- noarch not possible (maybe file a featurerequest at nagios to make it possible to install plugins into /usr/share/nagios/plugins?)
- BR/R ok
- BuildRoot and defattr could be left out, but as you are targeting el5: ok
- license ok
- no *.la
- rpmlint ignorable:
  $ rpmlint /home/tom/rpmbuild/SRPMS/nagios-plugins-openmanage-
    3.7.3-2.fc16.src.rpm /home/tom/rpmbuild/RPMS/x86_64/nagios-plugins-
    openmanage-3.7.3-2.fc16.x86_64.rpm
    nagios-plugins-openmanage.x86_64: E: no-binary
    nagios-plugins-openmanage.x86_64: W: only-non-binary-in-usr-lib
    2 packages and 0 specfiles checked; 1 errors, 1 warnings.
- no libs
- koji scratch build successful:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=3542984
- source match upstream:
  708257eedc7003d9c5fc3fba8200e572  check_openmanage-3.7.3.tar.gz
- files:
  * proper inclusion of man pages
  * %config there

NEEDSWORK:
- files:
  * %dir %{_sysconfdir}/nagios double owned:
    rpm -qf /etc/nagios
    nagios-3.2.3-11.fc16.x86_64

    This package R nagios-common, which doesn't R nagios, so the directory is
    unowned. It would be best to add the directory to nagios-common:
    Added to the list in bug 756839
    I wouldn't own it here and wait for nagios-common to pick it up...

- pre-build binary:
  https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
  Please delete the check_openmanage.exe binary in %prep, although you don't
  use them later on.

##############################################################

Let's wait for an answer of the nagios maintainer, but as your package looks 
fine otherwise and you made valid points in other reviews you are now:

##############################################################

SPONSORED

(In reply to comment #6)

> NEEDSWORK:
> - files:
>   * %dir %{_sysconfdir}/nagios double owned:
>     rpm -qf /etc/nagios
>     nagios-3.2.3-11.fc16.x86_64
> 
>     This package R nagios-common, which doesn't R nagios, so the directory is
>     unowned. It would be best to add the directory to nagios-common:
>     Added to the list in bug 756839
>     I wouldn't own it here and wait for nagios-common to pick it up...

The installed config file is just an example config with everything commented out. Also, the config file is optional and most users will probably not use it. In retrospect it shouldn't live in /etc/nagios as this directory should only contain actual Nagios config files, not config files for plugins.

In light of this I have removed it from /etc/nagios and only include it as documentation ("example.conf") instead.

> - pre-build binary:
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
>   Please delete the check_openmanage.exe binary in %prep, although you don't
>   use them later on.

Done.

Updated spec and SRPM available here:
http://folk.uio.no/trondham/review3/nagios-plugins-openmanage.spec
http://folk.uio.no/trondham/review3/nagios-plugins-openmanage-3.7.3-3.el6.src.rpm

(In reply to comment #7)
> (In reply to comment #6)
> 
> > NEEDSWORK:
> > - files:
> >   * %dir %{_sysconfdir}/nagios double owned:
> >     rpm -qf /etc/nagios
> >     nagios-3.2.3-11.fc16.x86_64
> > 
> >     This package R nagios-common, which doesn't R nagios, so the directory is
> >     unowned. It would be best to add the directory to nagios-common:
> >     Added to the list in bug 756839
> >     I wouldn't own it here and wait for nagios-common to pick it up...
> 
> The installed config file is just an example config with everything commented
> out. Also, the config file is optional and most users will probably not use it.
> In retrospect it shouldn't live in /etc/nagios as this directory should only
> contain actual Nagios config files, not config files for plugins.
> 
> In light of this I have removed it from /etc/nagios and only include it as
> documentation ("example.conf") instead.

Great

> > - pre-build binary:
> >  
> > https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
> >   Please delete the check_openmanage.exe binary in %prep, although you don't
> >   use them later on.
> 
> Done.

Yepp.

Looks completely fine now.

###############################################################

APPROVED

###############################################################


You can now proceed and do a SCM request:
https://fedoraproject.org/wiki/Package_SCM_admin_requests

Information for adding the package to the git repository is at:
https://fedoraproject.org/wiki/New_package_process_for_existing_contributors
https://fedoraproject.org/wiki/Using_Fedora_GIT

Information for shipping the updates to EL and released fedora branches:
https://fedoraproject.org/wiki/PackageMaintainers/UpdatingPackageHowTo#Submit_your_update_to_Bodhi

Don't hesitate to ask (here or private mail), when you run into any problems.

New Package SCM Request
=======================
Package Name: nagios-plugins-openmanage
Short Description: Nagios plugin to monitor hardware health on Dell servers
Owners: trondham
Branches: f15 f16 el5 el6
InitialCC:

Git done (by process-git-requests).

Thomas, please take ownership of review BZs, thanks!

nagios-plugins-openmanage-3.7.3-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.fc16

nagios-plugins-openmanage-3.7.3-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.fc15

nagios-plugins-openmanage-3.7.3-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.el6

nagios-plugins-openmanage-3.7.3-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.el5

nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 testing repository.

nagios-plugins-openmanage-3.7.3-3.fc15 has been pushed to the Fedora 15 stable repository.

nagios-plugins-openmanage-3.7.3-3.fc16 has been pushed to the Fedora 16 stable repository.

nagios-plugins-openmanage-3.7.3-3.el5 has been pushed to the Fedora EPEL 5 stable repository.

nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 stable repository.