Bug 226448 - Merge Review: sysklogd
Merge Review: sysklogd
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:03 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-26 14:17:26 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:03:52 EST
Fedora Merge Review: sysklogd

http://cvs.fedora.redhat.com/viewcvs/devel/sysklogd/
Initial Owner: pvrabec@redhat.com
Comment 1 Kevin Fenzi 2007-02-15 21:42:35 EST
I'd be happy to review this package. Look for a full review in a bit. 
Comment 2 Kevin Fenzi 2007-02-16 19:53:36 EST
Sorry for the delay in reviewing this. My build machine had issues... 

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.
OK - 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.
See below - 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
See below - Should have dist tag
OK - Should package latest version
7 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Might include the COPYING file?

2. What is the upstream source for the sysklogd-1.4.1rh.tar.gz?
This looks like a locally modified version of the upstream sysklogd-1.4.1.tar.gz.
Why is this done instead of using the upstream source and applying patches to it?

3. Since this package has a logrotate file, shouldn't it 'Require: logrotate' ?

4. Our handy little scripty friend rpmlint says:

a)
W: sysklogd prereq-use fileutils /sbin/chkconfig /etc/init.d

The use of PreReq is deprecated. In the majority of cases, a plain Requires
is enough and the right thing to do. Sometimes Requires(pre), Requires(post),
Requires(preun) and/or Requires(postun) can also be used instead of PreReq.

b)
W: sysklogd unversioned-explicit-provides syslog

Is this needed for an old package? Or to smooth transition to something like
syslog-ng?

c)
W: sysklogd macro-in-%changelog postun
W: sysklogd macro-in-%changelog post
W: sysklogd macro-in-%changelog preun
W: sysklogd macro-in-%changelog preun
W: sysklogd macro-in-%changelog clean
W: sysklogd macro-in-%changelog post

Suggest: Change macros in changelog to use %% so rpm doesn't expand them.

d)
W: sysklogd mixed-use-of-spaces-and-tabs (spaces: line 165, tab: line 59)

Suggest: pick tabs or spaces, don't use both.

e)
E: sysklogd incoherent-logrotate-file /etc/logrotate.d/syslog

This is due to this package being called sysklogd, and the init
script being called syslog. Can we rename this to sysklogd? Or would
that end up breaking too much? As a side note, I have found myself
doing 'service sysklogd restart' and then having to go look and see
that it's called syslog. ;)

f)
W: sysklogd service-default-enabled /etc/rc.d/init.d/syslog

In this case we can ignore that, we want a syslog by default. ;)

5. You could add a dist tag, but this package appears dead upstream,
so it's unlikely that it would need to be rebased much if any.

6. There are 7 outstanding bugs. You might look through them and see
if any of them could be addressed while other items are being taken care
of for this review.
Comment 3 Jose Pedro Oliveira 2007-02-16 21:11:30 EST
(In reply to comment #2)
> ...
> b)
> W: sysklogd unversioned-explicit-provides syslog
> 
> Is this needed for an old package? Or to smooth transition to something like
> syslog-ng?
 
To smooth the transition to an alternative log daemon.  More details in bug #172885.

 * Bug 172885: syslog-ng gets removed when sysklod is updated
   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=172885

jpo
Comment 4 Peter Vrabec 2007-02-20 11:38:12 EST
fixed in sysklogd-1.4.1-47.fc7

2) I don't know, but upstream doesn't seem to be alive.
3) I think "no". I didn't find many "Require: logrotate" in spec files of 
packages, which rotate its files.
4e) I have renamed logrotate.d/syslog but not going to do the same for 
init.t/syslog. I'm worry it could make more problems.
5) What dist tag do you mean?
6) I'm not going to fix any of these.
Comment 5 Jose Pedro Oliveira 2007-02-20 11:53:07 EST
(In reply to comment #4)
> fixed in sysklogd-1.4.1-47.fc7

> 3) I think "no". I didn't find many "Require: logrotate" in spec files of 
> packages, which rotate its files.

In the syslog-ng.spec there is a requirement for logrotate: syslog-ng installs
a file in the /etc/logrotate.d directory which is owned by the logrotate package.

   $ rpm -qf /etc/logrotate.d/
   logrotate-3.7.4-12.fc6

> 4e) I have renamed logrotate.d/syslog but not going to do the same for 
> init.t/syslog. I'm worry it could make more problems.

The rename broke the usage of syslog-ng as a sysklogd replacement. Syslog-ng
needs to ship the same file as sysklogd (same MD5 digest to avoid file
conflicts): logrotate doesn't like that two different configuration scripts
rotate the same log files.

jpo
Comment 6 Jose Pedro Oliveira 2007-02-20 12:01:58 EST
Another improvement would be to preserve the files timestamps during
installation, i.e., use install -p .

jpo

Comment 7 Peter Vrabec 2007-02-20 13:09:39 EST
thnx. Jose, all issues you metioned are fixed in sysklogd-1.4.1-48.fc7.
Comment 8 Kevin Fenzi 2007-02-20 22:44:03 EST
Thanks Peter. 

1. good. ok. 
2. Not sure what to do here... The package review guidelines say: 
"- MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task."

Is there anyone you might be able to ask to find out the reason for the
sysklogd-<sersion>rh.tar.gz source? It looks like notting used to maintain this
package around that time, perhaps we could ask him?

3. good. ok. 

4. a) b) c) d) good. ok. 
e) ok. I understand your reasoning, although I am not sure what problems
renaming the init file would cause. 
f) ok. 

5. You can read about the dist tag here: 
http://www.fedoraproject.org/wiki/Packaging/DistTag
basically this allows you to ship the same version in diffrent branches, and
have the fc6 version have a .fc6, and so on, so upgrades work. 

6. ok, none of them seem package related to me... 

Sorry for the breakage in syslog-ng Jose. ;( 

So, the only blocker I see left is point #2. Perhaps notting can enlighten us. 
Comment 9 Bill Nottingham 2007-02-20 23:12:26 EST
2 - Too many patches, so it was maintained in CVS (upstream sysklogd imported on
a vendor branch.)

See :pserver:rhlinux.redhat.com:/usr/local/CVS sysklogd

Comment 10 Kevin Fenzi 2007-02-21 00:06:41 EST
Humm...ok.
The differences between the upstream and the rh version seems to be around 26k
once you remove the sysklogd.spec and the redhat/Changelog files (which was last
updated in 2004). many of the remaning changes are pretty simple logical files
(like the files under the redhat/ dir, which are config, logrotate, etc, files).

For comparison, there is a ipv6 patch thats 21k.  

Peter: Would you be willing to split out the changes as logical patches and then
use the base upstream source?
Comment 11 Peter Vrabec 2007-02-21 07:15:31 EST
2) We can do nothing. I don't want to use upstream sources, because upstream 
is dead and some other distros are using our sources. I'd like to rather merge 
patches with RH sources, but need to be sure the sysklogd is stable as much as 
possible. I can remove URL(it doesn't point to our sources), if you don't like 
it.

5) There was a dist tag in spec file, wasn't it?
Comment 12 Kevin Fenzi 2007-02-21 18:50:07 EST
2. ok. 

Checking out a copy from the above cvs, making a .tar.gz and diffing between
that version and whats shipped in your src rpm shows one diffrence... 
you have a sysklogd.spec file in your tar.gz, which doesn't exist in cvs. 
I assume thats generated from the sysklogd.spec.in, so it can be ignored. 

Can you add a comment to the spec before the Source line indicating how someone 
could check out the source from rhlinux cvs? something like: 

# The source for this package was pulled from cvs.
# Use the following commands to generate the tarball:
#  export CVSROOT=:pserver:anonymous@rhlinux.redhat.com:/usr/local/CVS
#  cvs login (hit return)
#  cvs co sysklogd
#  mv sysklogd sysklogd-%{version}rh
#  tar -czvf sysklogd-%{version}rh.tar.gz sysklogd-%{version}rh

5. There is indeed a dist tag. That was an old/outdated comment. 

I see no further blockers here. If you could make that comment change, we can
call this package APPROVED. 
Comment 13 Bill Nottingham 2007-02-21 18:58:30 EST
For generating the tarball, see the tag-archive/create-archive sections in the
Makefile.
Comment 14 Kevin Fenzi 2007-02-23 22:27:38 EST
Per the new review guidelines I am reassigning to me (reviewer). 
Please close this rawhide once you have made the change from comment #12 and 
pushed it out to rawhide/devel. 
https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html
Comment 15 Peter Vrabec 2007-02-26 05:12:59 EST
comment #12 is fixed in sysklogd-1.4.2-1.fc7, which is a new upstream(RH) 
release.
Comment 16 Kevin Fenzi 2007-02-26 14:17:26 EST
Yep. Looks good from here... I'll go ahead and close this review rawhide. 
Thanks again for all your patience. 

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