Bug 433547

fwiw, I was also working on nsca lately. I uploaded my package and spec if you
want to take a look (this is still a work in progress) :
http://washington.kelkoo.net/fedora/SPECS/nsca.spec
http://washington.kelkoo.net/fedora/SRPMS/nsca-2.7.2-2.fc8.src.rpm

I did not had time to look closely at your package but I will as soon as I have
a bit of time.

Regards,
Xavier

Hi Xavier,

It looks like we started from the same DAG rpm.  :)  Don't forget that the
/etc/nagios/*.cfg files need to be mode 0600 and readable only by root because
they contain encryption passwords.

Since you've also worked on the same package, would you like to review and/or
comaintain this with me?

(In reply to comment #2)
> Hi Xavier,
> 
Hi Wart,

> It looks like we started from the same DAG rpm.  :)  Don't forget that the
> /etc/nagios/*.cfg files need to be mode 0600 and readable only by root because
> they contain encryption passwords.
> 
> Since you've also worked on the same package, would you like to review and/or
> comaintain this with me?

Sure, that's the plan, but I'm short on time at the moment so I just pointed you
at my unfinished work so you may (or may not) cherry-pick from it. I'll get back
to it next week hopefully.

Regards,
Xavier

Hi Wart,

Here are my preliminary comments on your package :

Summary is not capitalized.

I like nsca-client much better than what I used. Is it really useful to suffix
the packages with nagios- though ? This would clear rpmlint output from 
nagios-nsca.i386: W: incoherent-subsys /etc/rc.d/init.d/nsca $prog
nagios-nsca.i386: W: incoherent-init-script-name nsca

nagios, nrpe and all are in the group Applications/System. You probably want to
use the same.

I don't have the BR: on libmcrypt-devel and my package build fine. Is it
necessary to keep it ?

I have some requires on /sbin/service and /sbin/chkconfig, taken from nrpe
packages, not sure this is needed, can't find anything in the packaging guidelines.
Requires(preun): /sbin/service, /sbin/chkconfig
Requires(post): /sbin/chkconfig, /sbin/service
Requires(postun): /sbin/service

NetSaint can probably be stripped out of the description.

Upstream ships an initscript (generated at build time actually) that only lacks
reload, it might not worth to replace it completely.

0600 on the conf files is good.

No doc in nsca-client, add %doc Changelog LEGAL README SECURITY

Nothing more for today, let me know what you think.

Regards,
Xavier

2 enhancements to my earlier comments: 
 
> I don't have the BR: on libmcrypt-devel and my package build fine. Is it
> necessary to keep it ?
> 
Indeed, this is needed... Sorry, temporary brain failure.

> I have some requires on /sbin/service and /sbin/chkconfig, taken from nrpe
> packages, not sure this is needed, can't find anything in the packaging
guidelines.
> Requires(preun): /sbin/service, /sbin/chkconfig
> Requires(post): /sbin/chkconfig, /sbin/service
> Requires(postun): /sbin/service
>
Yes, this is needed, here's the reference : 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

I'll post an updated spec later.

Here's another iteration based on the package that you mailed me (thanks!).  I
think we should keep 'NetSaint' in the description because we're not disabling
any netsaint functionality.  I also merged the sed script that was modifying the
init script into the existing init script patch.  It seemed silly to use both a
patch and a sed script for the same file:

http://www.kobold.org/~wart/fedora/nsca.spec
http://www.kobold.org/~wart/fedora/nsca-2.7.2-4.fc8.src.rpm

I haven't tried the new init script myself; I'll take your word that it works as
expected.

(In reply to comment #6)
> Here's another iteration based on the package that you mailed me (thanks!).  I
> think we should keep 'NetSaint' in the description because we're not disabling
> any netsaint functionality. 

Ok

>  I also merged the sed script that was modifying the
> init script into the existing init script patch.  It seemed silly to use both a
> patch and a sed script for the same file:
> 
Agreed.

> http://www.kobold.org/~wart/fedora/nsca.spec
> http://www.kobold.org/~wart/fedora/nsca-2.7.2-4.fc8.src.rpm
> 
> I haven't tried the new init script myself; I'll take your word that it works as
> expected.

I need to diff the old one and the new one to make sure it's ok.

I see you actually used my spec as the new base, I hope this won't disqualify me
for the formal review. I'll put it in the next comment. Also this is my first
formal review, so I'll ask my sponsor to take a look.

+ source files match upstream:
33a98e7975f633a9489d7a8938ed6131

+ package meets naming and versioning guidelines.

+ specfile is properly named, is cleanly written and uses macros consistently.

+ dist tag is present.

+ build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

+ license field matches the actual license.

+ license is open source-compatible. License text not included upstream.

+ latest version is being packaged.

+ BuildRequires are proper.

+ compiler flags are appropriate.

+ package builds in mock ( ).

+ package installs properly

+ debuginfo package looks complete.

+ rpmlint is not silent, but the 2 warnings are expected :
nsca.i386: E: non-readable /etc/nagios/nsca.cfg 0600
nsca-client.i386: E: non-readable /etc/nagios/send_nsca.cfg 0600

+ final provides and requires are sane:
Provides: config(nsca) = 2.7.2-4.el5
Requires(interp): /bin/sh /bin/sh /bin/sh
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires(post): /bin/sh /sbin/chkconfig
Requires(preun): /bin/sh /sbin/chkconfig /sbin/service
Requires(postun): /bin/sh /sbin/service
Requires: /bin/sh config(nsca) = 2.7.2-4.el5 libc.so.6 libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIB
C_2.4) libmcrypt.so.4 libnsl.so.1 nagios rtld(GNU_HASH)

Provides: config(nsca-client) = 2.7.2-4.el5
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: config(nsca-client) = 2.7.2-4.el5 libc.so.6 libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4)
libc.so.6(GLIBC_2.4) libmcrypt.so.4 libnsl.so.1 rtld(GNU_HASH)

+ no shared libraries are added to the regular linker search paths.
- owns the directories it creates.
nsca-client should own /etc/nagios because it doesn't require nagios.
nsca doesn't because it requires nagios.

+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ scriptlets follow guidelines.
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.

just add %dir %{_sysconfdir}/nagios for the client and it should be good to go.

additional note : nsca-2.7.2-initreload.patch might need to be renamed now it
does more than just add reload to the init script.

I went ahead with the 2 changes above and fixed the initscript at the same time
(broken reload, missing condrestart and both of them missing from the usage
message).

http://washington.kelkoo.net/fedora/SPECS/nsca.spec
http://washington.kelkoo.net/fedora/SRPMS/nsca-2.7.2-5.fc8.src.rpm

You depending on the nagios package for both the client and server. Only
server needs nagios itself installed. 

What about the nagios user? Since your compiling NSCA requiring the nagios
using being present you'll need the user. This package should attempt to
create the user and otherwise silently ignore if it exists already.

(In reply to comment #10)
> You depending on the nagios package for both the client and server. Only
> server needs nagios itself installed. 
>
Only nsca (the daemon) Requires: nagios. nsca-client doesn't.
 
> What about the nagios user? Since your compiling NSCA requiring the nagios
> using being present you'll need the user. This package should attempt to
> create the user and otherwise silently ignore if it exists already.

The nagios user is not required at compile time, the --with-nsca-user and
--with-nsca-grp option are only used to generate the default config. The nagios
user will only be needed at run time and then nsca Requires: nagios so it will
be already there.

Xavier asked me to have a quick look.  Sorry I couldn't get to it yesterday.

A few meta-issues first:

Xavier, if you indeed are reviewing this, you should assign the ticket to
yourself and set the fedora-review flag to '?'.

The summary of the ticket should match the submitted package name.  Was it de

I don't have any particular issues with the reviewer providing fixed packages,
but  Michael does need to respond to the stuff in comments 8 and 9 before this
can move anywhere.

I am unfamiliar with nagios and related software, so I can really only look at
the basic packaging, which looks fine.  I'm looking at the package linked in
comment 9.  The scriptlets aren't exactly the same as the recommended ones, but
the differences are inconsequential.  Do note, however, that the "|| :" bit only
has an effect as the last executed command in a scriptlet; its sole purpose is
to prevent the scriptlet from returning a nonzero exit code.

Also note that I tend to just drop the bits of my review template that don't
apply, such as debuginfo and compiler flag bits for a noarch package, and the
GUI app bit for a non-gui app.  They're just placeholders to remind me to check
things when they apply.

Finally, I see some source files that are obviously GPLv2+, and others (nsca.c,
send_nsca.c) which say GPLv2.  Someone should ping upstream and see if they
intend GPLv2 only for the final product.  Otherwise I don't think GPLv2+ is
accurate for the license tag.

Thanks for your comments Jason.
I've already addressed the issues from comment 8 in the package in comment 9. So
the last remaining issues would be the preun and postun scriptlets and the
License: tag.
Michael, could you please look at the scriptlets[1] and contact upstream about
the license issue ?

FWIW, I'm interested in co-maintaining this package, at least for EPEL.

[1]
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

Sorry for the silence.  Xavier has pretty much been proactive in updating the
review package as needed.  If he'd like to become the primary maintainer and
have me as a comaintainer, I'd be more than happy to hand this review over to him.

In the meantime I'll ping upstream about the licensing issue.

I believe I can't be the primary maintainer and do the review at the same time.
You submitted the review, so you should be the primary maintainer. Like I wrote,
I'm genuinely interested in having this in EPEL, that's why I have been so
active. Sorry to be pushy, I certainly don't want to look as making a take over
on the package. Actually, I think we did a pretty good complementary job :-) 

Wart, still around ? Did upstream get back to you wrt license ? Also, do you
want me to fix the scriplets to be as in the guidelines ? I'd like to get this
package built and released asap.

Upstream never responded to the licensing question, so I've changed the license
to GPLv2 to best match the sources.  I also updated the scriplets to use  || :
on the last command.

http://www.kobold.org/~wart/fedora/nsca-2.7.2-6.fc8.src.rpm
http://www.kobold.org/~wart/fedora/nsca.spec

I think we're almost there...

looks good, APPROVED.

I'm willing to co-maintain this package for EPEL. I don't mind being
comaintainer for Fedora too if you wish so. My username is xavierb.

Cheers,
Xavier

New Package CVS Request
=======================
Package Name: nsca
Short Description: Nagios passive check daemon
Owners: wart, xavierb
Branches: F-8 EL-4 EL-5
InitialCC: 
Cvsextras Commits:

cvs done.

Imported and built for Rawhide.  Builds for F-8 and EL-{4,5} are coming along soon.