Bug 226105

I'd be happy to review this. Look for a full review in a bit... 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
See below - Should package latest version
5 open bugs - check for outstanding bugs on package.

Issues:

1. Should use a full URL in Source line.
What is the upstream for this package?

This is version 3.7.4, which I see no where on
http://www2.logwatch.org:81/tabs/download/
That site has version 7.3.2.

2. Should include COPYING file.

3. Buildroot should be standard buildroot.

4. rpmlint, out little pal says:

rpmlint on ./logrotate-debuginfo-3.7.4-11.fc7.i386.rpm
W: logrotate-debuginfo no-url-tag
rpmlint on ./logrotate-3.7.4-11.fc7.src.rpm
W: logrotate no-url-tag
rpmlint on ./logrotate-3.7.4-11.fc7.i386.rpm
W: logrotate no-url-tag

URL should be fixed. See point 1.

E: logrotate zero-length /var/lib/logrotate.status

Should this be set the same way.

5. Should look at the 5 open bugs pending.

I'm not sure whether you rewieved the last version of logwatch in fc
(logwatch-7.3.2-6.fc7)

Odd. I was pulling things from Matts mass rebuild at: 
http://linux.dell.com/files/fedora/FixBuildRequires/mock-results-core/i386/logrotate-3.7.4-11.fc7.src.rpm/

But you are right, thats clearly old and not the current package. ;( 

I will try and sort it out after I get some sleep... Sorry for the incorrect
version. 

Sorry for the review on the old package. No idea how that got in there... 
I will ping mdomish and see if he can figure it out. 

In the mean time I have done a review of the real version... 
(7.3.2-6.fc7):

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (MIT)
See below - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
4beead011dda24741a30ebe60de054b2  logwatch-7.3.2.tar.gz
4beead011dda24741a30ebe60de054b2  logwatch-7.3.2.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version
0 outstanding bugs - check for outstanding bugs on package.

Issues:

1. 2 of the files seem to have a GPL license:
scripts/services/courier
and
scripts/services/dpkg

Can they really be distributed with the rest of the MIT licensed code?
Perhaps upstream should be contacted about them...

2. Should use the approved one true buildroot:
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

3. There is quite a bit of output from our pal rpmlint...

rpmlint on logwatch-7.3.2-5.fc7.src.rpm
W: logwatch no-%build-section

Since this package is noarch, I think you can ignore this...
If you like you could add a empty %build just for completeness.

rpmlint on logwatch-7.3.2-5.fc7.noarch.rpm

All of these look like they are perl modules, but they are executable,
even though they don't have a #!/usr/bin/perl. They aren't meant to be
called directly, so they should probibly all be mode 644?

E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/tac_acc
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/modprobe
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applybinddate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/mountd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/ipop3d
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/clamav
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/amavis
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/samba/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applystddate
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/zz-disk_space
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/sudo
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/zz-fortune
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/http
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/yum/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/onlyservice
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/ftpd-xferlog
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/xferlog/removeheaders
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/sshd2
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/dhcpd
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/emerge/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/stunnel
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/kernel
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/up2date/removeheaders
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/audit
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/portsentry
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/syslogd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/yum
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/qmail
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/netscreen
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/dpkg
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/fail2ban
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/removeheaders
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/onlycontains
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/netopia
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/resolver
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/secure
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/automount
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/qmail-smtpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/cisco
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/rt314
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/sshd
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/clamav-milter
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/cron
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/samba/removeheaders
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/hosthash
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/scsi
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/dovecot
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/dnssec
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applyhttpdate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pureftpd
: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applyhttpdate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pureftpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/in.qpopper
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/zz-network
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/qmail-pop3ds
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/remove
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/imapd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pam
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/sendmail-largeboxes 
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/cron/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pix
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/samba
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/openvpn
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/xntpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pound
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/sendmail
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applyeurodate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/vpopmail
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pam_unix
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/shared/applyvsftpddate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/expandrepeats
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/sonicwall
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/php
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/extreme-networks
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/identd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/emerge
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/exim
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/init
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pluto
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/saslauthd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/afpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/zz-sys
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applytaidate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/slon
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/oidentd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pam_pwdb
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/removeservice
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/hostlist
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/mailscanner
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/eximstats
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/xferlog/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/smartd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/denyhosts
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/qmail-send
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/postfix
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/named
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/proftpd-messages
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/up2date/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/shaperd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/raid
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/clam-update
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/up2date
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/services/ftpd-messages
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/onlyhost
E: logwatch script-without-shebang
/usr/share/logwatch/scripts/logfiles/autorpm/applydate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/qmail-pop3d
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/vsftpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/autorpm
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/vsftpd
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/autorpm
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/windows
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/pop3
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/courier
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/applyusdate
E: logwatch script-without-shebang /usr/share/logwatch/scripts/shared/multiservice
E: logwatch script-without-shebang /usr/share/logwatch/scripts/services/arpwatch
W: logwatch symlink-should-be-relative /etc/cron.daily/0logwatch
/usr/share/logwatch/scripts/logwatch.pl
W: logwatch symlink-should-be-relative /usr/sbin/logwatch
/usr/share/logwatch/scripts/logwatch.pl

Should change that link to ../share/logwatch/scripts/logwatch.pl?
This makes the package more usefull in chroot type setups.

E: logwatch wrong-script-end-of-line-encoding
/usr/share/logwatch/scripts/services/courier
E: logwatch wrong-script-end-of-line-encoding
/usr/share/logwatch/scripts/services/pix

Dos line endings? You can fix this with a call to sed...

%{__sed} -i 's/\r//' /usr/share/logwatch/scripts/services/courier

E: logwatch zero-length /usr/share/logwatch/scripts/services/zz-fortune
E: logwatch zero-length /usr/share/doc/logwatch-7.3.2/TODO

zero-length files are not really very useful. Should be removed?

Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back
to me, and change the 'fedora-review' flag back to ? for me to take action.

Thanks for your excellent review - I just discuss some points with logwatch
upstream - so it takes some time to fix all problems.

Fixed in logwatch-7.3.2-7.fc7.
The zero lenght files remains in logwatch (they are necessary - but they will be
nonempty soon).

1. Removing them I guess is ok.
Have you contacted upstream about distributing them at all?

2. good. ok.

3.1. %build added ok.

3.2. 644 on all the services looks good. ok.

3.3. The logwatch.pl links are still not relative...
Suggest:

Change:

ln -s %{_datadir}/logwatch/scripts/logwatch.pl
%{buildroot}%{_sysconfdir}/cron.daily/0logwatch
ln -s %{_datadir}/logwatch/scripts/logwatch.pl %{buildroot}%{_sbindir}/logwatch

to

ln -s ../../%{_datadir}logwatch/scripts/logwatch.pl
%{buildroot}%{_sysconfdir}/cron.daily/0logwatch
ln -s ../../%{_datadir}logwatch/scripts/logwatch.pl %{buildroot}%{_sbindir}/logwatch

3.4. The "pix" file still has dos CR/FE endings...
Suggest:

%{__sed} -i 's/\r//' %{buildroot}%{_datadir}/logwatch/scripts/services/pix

3.5. ok.

So, 3.3 and 3.4 are the only outstanding items I see...

Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back
to me, and change the 'fedora-review' flag back to ? for me to take action.

Fixed in logwatch-7.3.2-8.fc7.

Thanks for the fixes. 

3.3 looks fine. 
3.4 looks ok, but you might just use a: 
sed -i 's/\r//' <file_to_strip>
in the spec file instead of a patch that just replaces the entire file. 
Thats not a blocker, just a suggestion. 

I don't see any further issues here, this package is APPROVED. 
Thanks for the speedy fixes... 

Per the new offical review guidelines, I am reassigning this to me (the
reviewer) and closing it rawhide (since the changes are already available there).