Fedora Merge Review: sysklogd http://cvs.fedora.redhat.com/viewcvs/devel/sysklogd/ Initial Owner: pvrabec
I'd be happy to review this package. Look for a full review in a bit.
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.
(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
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.
(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
Another improvement would be to preserve the files timestamps during installation, i.e., use install -p . jpo
thnx. Jose, all issues you metioned are fixed in sysklogd-1.4.1-48.fc7.
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.
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
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?
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?
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.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.
For generating the tarball, see the tag-archive/create-archive sections in the Makefile.
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 #12 is fixed in sysklogd-1.4.2-1.fc7, which is a new upstream(RH) release.
Yep. Looks good from here... I'll go ahead and close this review rawhide. Thanks again for all your patience.