Bug 213832

rpmlint -v arpwatch-2.1a15-1.fc7.src.rpm
I: arpwatch checking
W: arpwatch strange-permission arpwatch.init 0775

Nothing important but please fix that.

rpmlint -v ../RPMS/i386/arpwatch-2.1a15-1.i386.rpm
I: arpwatch checking
W: arpwatch conffile-without-noreplace-flag /etc/rc.d/init.d/arpwatch
The question is whether the init script should be %config at all or not.

W: arpwatch conffile-without-noreplace-flag /var/lib/arpwatch/ethercodes.dat
Probably OK.

E: arpwatch non-standard-uid /var/lib/arpwatch/ethercodes.dat pcap
E: arpwatch non-standard-gid /var/lib/arpwatch/ethercodes.dat pcap
E: arpwatch executable-marked-as-config-file /etc/rc.d/init.d/arpwatch
E: arpwatch non-standard-uid /var/lib/arpwatch/arp.dat pcap
E: arpwatch non-standard-gid /var/lib/arpwatch/arp.dat pcap
E: arpwatch zero-length /var/lib/arpwatch/arp.dat
E: arpwatch non-standard-uid /var/lib/arpwatch pcap
E: arpwatch non-standard-gid /var/lib/arpwatch pcap
These are OK.

IIRC no initscript has been marked as %config, yet (which doesn't make sense 
anway IMHO).

I've seen some (sshd, but I unmarked it for FC-6).
So I agree that the initscript should not be %config.

* The service related scriptlets don't follow the guidelines

* the adduser followed by a chsh is a bit odd.

* why isn't the useradd only done for the first install?

* in %files /etc should certainly be %{sysconfdir}

* just a comment, but perl one liner could be replaced by a 
  sed, (or even a patch to be sure that it breaks if the files
  change in some way)

* similarly the awk embedding could also be done as a patch
  and you'll use your scripts to regenerate the patch.

* if there is a libpcap library nearby (see line 209 to 221
  in configure.ac) it is used, for example in the BUILD directory. 
  Maybe this part of the configure/configure.ac could be patched out.
  It shouldn't be annoying in mock, however.

* maybe the oui.txt could be provided in another package and be used
  to regenerate ethercode.dat to have more uptodate infos. I have found
  http://www.mail-archive.com/debian-legal@lists.debian.org/msg32779.html
  on that subject.

* some Requires are missing. I think /usr/sbin/sendmail or smtpdaemon
  (and also maybe /usr/sbin/sendmail as BuildRequires). snmpwalk is 
  more or less required for arpsnmp, but it is better in my opinion not
  to have a dependency nor do something special, the sysadm should be able
  to figure out.

(In reply to comment #4)

> * some Requires are missing. I think /usr/sbin/sendmail or smtpdaemon
>   (and also maybe /usr/sbin/sendmail as BuildRequires). snmpwalk is 
>   more or less required for arpsnmp, but it is better in my opinion not
>   to have a dependency nor do something special, the sysadm should be able
>   to figure out.

I missed the arpfetch script. Since it requires snmpwalk, it should be
required by arpwatch. 

Reading arpfetch, I see that it modifies the PATH at the beginning, I think 
this should be patched out.

> * The service related scriptlets don't follow the guidelines
What do you mean?

> * the adduser followed by a chsh is a bit odd.
This was probably some preventive action for the case when the user was already
there. I'd leave it on packager to decide if chsh should be removed or not.

> * in %files /etc should certainly be %{sysconfdir}
I've overlooked that. Miroslav, please correct that.

> * if there is a libpcap library nearby (see line 209 to 221
  in configure.ac) it is used, for example in the BUILD directory. 
  Maybe this part of the configure/configure.ac could be patched out.
  It shouldn't be annoying in mock, however.
I've noticed that too, I don't think it must be fixed but it would be good idea.

> * some Requires are missing. I think /usr/sbin/sendmail or smtpdaemon
>   (and also maybe /usr/sbin/sendmail as BuildRequires). snmpwalk is 
>   more or less required for arpsnmp, but it is better in my opinion not
>   to have a dependency nor do something special, the sysadm should be able
>   to figure out.
I agree that sendmail probably should be required. smtpdaemon not as it is
definitely optional thing and arpwatch is usable without it.

I don't think that the package should require snmpwalk. This is unnecessary
bloat in case someone wants to use just arpwatch.

(In reply to comment #6)
> > * The service related scriptlets don't follow the guidelines
> What do you mean?

The scriptlet are not the same than in 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e
It may not be problematic, those scriptlets could even be better, but
maybe this could be argued.

> > * the adduser followed by a chsh is a bit odd.
> This was probably some preventive action for the case when the user was already
> there. I'd leave it on packager to decide if chsh should be removed or not.

It seems a bit bad to me. I don't think it is a good idea to 
modify the user after the first install. The sysadm may have changed
something on purpose.


> > * if there is a libpcap library nearby (see line 209 to 221
>   in configure.ac) it is used, for example in the BUILD directory. 
>   Maybe this part of the configure/configure.ac could be patched out.
>   It shouldn't be annoying in mock, however.
> I've noticed that too, I don't think it must be fixed but it would be good idea.

This is definitely not a blocker, but an improvement.

> I agree that sendmail probably should be required. smtpdaemon not as it is
> definitely optional thing and arpwatch is usable without it.

smtpdaemon is more or less similar with /usr/sbin/sendmail, in fact
the semantics of this virtual provide is not clear to me. 
/usr/sbin/sendmail is certainly better anyway.

> I don't think that the package should require snmpwalk. This is unnecessary
> bloat in case someone wants to use just arpwatch.

In that case arpfetch shouldn't be installed but in %doc. Otherwise there
is a non-functional script installed. Another option would be to 
document prominently in fedora specific place (%description or README.fedora
for example) that snmpwalk is needed for proper functionning.

Ok, thanks for the input.

New spec: http://people.redhat.com/jnovy/files/arpwatch.spec
New SRPM: http://people.redhat.com/jnovy/files/arpwatch-2.1a15-1.1.fc7.src.rpm

In general it is not needed, but I think it could be helpfull in the
case of the one liners of that package to have both versions
of the script to be able to do a gendiff to see what the one-liners
did. It would amount to changing the perl line to look like:

perl -pi.quotes -e "s/\'/\'\\\'\'/g" *.awk

and doing a cp after the sed one liner, something along:

....
                        > $RPM_BUILD_ROOT%{_sbindir}/$i.embedawk
                cp $RPM_BUILD_ROOT%{_sbindir}/$i.embedawk .
                mv $RPM_BUILD_ROOT%{_sbindir}/$i{.embedawk,}
....

After seing the result of the perl one-liner result, I really think 
it should be a patch... For the awk embedding it is certainly better
with the one-liner, but with a way to be able to easily see what was 
changed.

Miroslav, whether you will implement the comment #9 I'd leave on your decision.

Other nits:

As you require /usr/sbin/sendmail you should probably also BuildRequire it
because otherwise the /usr/lib/sendmail default from the configure script will
be used.

The snmpwalk non-requirement looks to me like a non issue because user running
the arpfetch command will get a message snmpwalk not found if it is not there.
And I wouldn't say that the script is non functional, it just requires
installation of another package. This is really only issue of aesthetics and I'd
like to leave that on Miroslav to decide.

The release number should be probably just a single number (+ disttag) for FC devel.

The scripts aren't one-to-one copy of the scriptlets from guidelines but I don't
think it is mandatory to have one-to-one copy if they work the same.

As the things above are only minor nits and comments and the package is OK
otherwise I think I can call it ACCEPTED.

(In reply to comment #10)


> The snmpwalk non-requirement looks to me like a non issue because user running
> the arpfetch command will get a message snmpwalk not found if it is not there.

That's what I call broken. A user running a script installed in the default
PATH by a package should not get any error. Or it should be documented 
prominently.

> And I wouldn't say that the script is non functional, it just requires
> installation of another package. This is really only issue of aesthetics and I'd
> like to leave that on Miroslav to decide.

It's not aesthetic, it's poor packaging. Packaged software should work 
out of the box, or have things that won't work out of the box 
documented.

> The release number should be probably just a single number (+ disttag) for FC
devel.

If it is a pre-release it should be named according to
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a

If it is a post release version, it is right as is, as seen here:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-18aa467fc6925455e44be682fd336667a17e8933

> The scripts aren't one-to-one copy of the scriptlets from guidelines but I don't
> think it is mandatory to have one-to-one copy if they work the same.

I agree on the principle, but I'd like to have some explanations.
Is it true that they work the same? Is it sure that the exit 0 is enough
to avoid any failure? Some snippets on the wiki page have ||:, is it 
unusefull?

> As the things above are only minor nits and comments and the package is OK
> otherwise I think I can call it ACCEPTED.

One of my questions hasn't been answered. It is certainly not a blocker,
but I think it also deserves an explanation (it may even be that it is
the other possibility, ie doing useradd only for the first install which
is wrong):

* why isn't the useradd only done for the first install?

(In reply to comment #11)
> > The snmpwalk non-requirement looks to me like a non issue because user running
> > the arpfetch command will get a message snmpwalk not found if it is not there.
> 
> That's what I call broken. A user running a script installed in the default
> PATH by a package should not get any error. Or it should be documented 
> prominently.
> 
> > And I wouldn't say that the script is non functional, it just requires
> > installation of another package. This is really only issue of aesthetics and I'd
> > like to leave that on Miroslav to decide.
> 
> It's not aesthetic, it's poor packaging. Packaged software should work 
> out of the box, or have things that won't work out of the box 
> documented.

OK, if README.fedora in %doc would satisfy you I think that Miroslav should add
it. Or arpfetch could be moved to %doc.

> > The release number should be probably just a single number (+ disttag) for FC
> devel.
> 
> If it is a pre-release it should be named according to
>
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a
> 
> If it is a post release version, it is right as is, as seen here:
>
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-18aa467fc6925455e44be682fd336667a17e8933

It is post release, I didn't comment on the version but on the release number
(1.1) - two part number shouldn't be used in devel without a reason.
 
> > The scripts aren't one-to-one copy of the scriptlets from guidelines but I don't
> > think it is mandatory to have one-to-one copy if they work the same.
> 
> I agree on the principle, but I'd like to have some explanations.
> Is it true that they work the same? Is it sure that the exit 0 is enough
> to avoid any failure? Some snippets on the wiki page have ||:, is it 
> unusefull?
I've actually tested the scriptlets and even if /sbin/service return nonzero
exit code the chkconfig runs OK so the || : is unnecessary.

> > As the things above are only minor nits and comments and the package is OK
> > otherwise I think I can call it ACCEPTED.
> 
> One of my questions hasn't been answered. It is certainly not a blocker,
> but I think it also deserves an explanation (it may even be that it is
> the other possibility, ie doing useradd only for the first install which
> is wrong):
> 
> * why isn't the useradd only done for the first install?
> 
I'll leave this one on Miroslav to answer.

(In reply to comment #12)

> OK, if README.fedora in %doc would satisfy you I think that Miroslav should add
> it. Or arpfetch could be moved to %doc.

Both solutions seem right to me. I have a slight personal preference 
for arpfetch in %doc.

> It is post release, I didn't comment on the version but on the release number
> (1.1) - two part number shouldn't be used in devel without a reason.

Ok, you're right, I misunderstood...

> I've actually tested the scriptlets and even if /sbin/service return nonzero
> exit code the chkconfig runs OK so the || : is unnecessary.

Ok.

(In reply to comment #10)
> As you require /usr/sbin/sendmail you should probably also BuildRequire it
> because otherwise the /usr/lib/sendmail default from the configure script will
> be used.

Ok.

> The snmpwalk non-requirement looks to me like a non issue because user running
> the arpfetch command will get a message snmpwalk not found if it is not there.
> And I wouldn't say that the script is non functional, it just requires
> installation of another package.

I moved the arpfetch to %doc in the 1.1 release.

> The release number should be probably just a single number (+ disttag) for FC
devel.

Yes, I have used the minor number just for this reviewing process.

(In reply to comment #11)
> > The scripts aren't one-to-one copy of the scriptlets from guidelines but I don't
> > think it is mandatory to have one-to-one copy if they work the same.
> 
> I agree on the principle, but I'd like to have some explanations.
> Is it true that they work the same? Is it sure that the exit 0 is enough
> to avoid any failure? Some snippets on the wiki page have ||:, is it 
> unusefull?

rpm checks return code of the program executing the scriptlet, /bin/sh in this
case, and exit 0 guarantees it will return 0. This is the same as adding "||:"
to the last command in the script.

> * why isn't the useradd only done for the first install?

- the user may get deleted after first install or there could be an upgrade from
a broken/ancient version of arpwatch and arpwatch doesn't even start without the
user (with default config)
- other packages do the same

> rpm checks return code of the program executing the scriptlet, /bin/sh in this
> case, and exit 0 guarantees it will return 0. This is the same as adding "||:"
> to the last command in the script.

Ok.

> > * why isn't the useradd only done for the first install?
> 
> - the user may get deleted after first install or 

Do we really want to readd the user even if the admin removed it?

> there could be an upgrade from
> a broken/ancient version of arpwatch and arpwatch doesn't even start without the
> user (with default config)

What arpwatch version could it be? Is it a real or a theoretical issue?

> - other packages do the same

That's not a reason to do the same. Other packages may be broken.

Added to dist-fc7.  Please close this bug when you've built for rawhide.