Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226105 - Merge Review: logwatch
Summary: Merge Review: logwatch
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 19:35 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-24 03:22:45 UTC
kevin: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 19:35:11 UTC
Fedora Merge Review: logwatch

http://cvs.fedora.redhat.com/viewcvs/devel/logwatch/
Initial Owner: varekova@redhat.com

Comment 1 Kevin Fenzi 2007-02-03 22:43:08 UTC
I'd be happy to review this. Look for a full review in a bit... 


Comment 2 Kevin Fenzi 2007-02-03 23:19:19 UTC
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.

Comment 3 Ivana Varekova 2007-02-05 14:18:53 UTC
I'm not sure whether you rewieved the last version of logwatch in fc
(logwatch-7.3.2-6.fc7)

Comment 4 Kevin Fenzi 2007-02-05 23:44:33 UTC
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. 


Comment 5 Kevin Fenzi 2007-02-07 03:35:33 UTC
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.


Comment 6 Ivana Varekova 2007-02-07 09:52:37 UTC
Thanks for your excellent review - I just discuss some points with logwatch
upstream - so it takes some time to fix all problems.

Comment 7 Ivana Varekova 2007-02-08 15:12:18 UTC
Fixed in logwatch-7.3.2-7.fc7.
The zero lenght files remains in logwatch (they are necessary - but they will be
nonempty soon).

Comment 8 Kevin Fenzi 2007-02-09 04:20:34 UTC
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.


Comment 9 Ivana Varekova 2007-02-09 12:58:23 UTC
Fixed in logwatch-7.3.2-8.fc7.

Comment 10 Kevin Fenzi 2007-02-11 20:55:30 UTC
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... 

Comment 11 Kevin Fenzi 2007-02-24 03:22:45 UTC
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). 


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