Fedora Merge Review: webalizer http://cvs.fedora.redhat.com/viewcvs/devel/webalizer/ Initial Owner: jorton
Hi Joe, I'm happy to review your package. Review for release 31: * RPM name is OK * Source webalizer-2.01-10-src.tar.bz2 is the same as upstream * This is the latest version Needs work: * BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) (wiki: PackagingGuidelines#BuildRoot) * Encoding of spec should be UTF-8. iconv -f iso8859-1 -t utf-8 should do it. * Missing SMP flags. If it doesn't build with it, please add a comment (wiki: PackagingGuidelines#parallelmake) * Spec file: some paths are not replaced with RPM macros (wiki: QAChecklist item 7) * The %makeinstall macro should not be used (wiki: PackagingGuidelines#MakeInstall) * Please use the ?{dist} tag * Remove the dot at the end of the Summary * Replace BuildPreReq with BuildRequires * shadow-utils and fileutils can be dropped, since they're always there * Preserve timestamps when installing files * Consider installing content in /usr/share/webalizer instead of /var/www Minor: * Duplicate BuildRequires: zlib-devel (by gd-devel), libpng-devel (by gd-devel)
Thanks a lot for the review! In -32: - spec file cleanups (#226536): * convert to UTF-8 * fix BuildRoot, Summary * add Requires(pre) for shadow-utils, remove Prereqs * trim BuildRequires to png-devel, db4-devel * use smp_mflags in make * use sysconfdir macro throughout * preserve file timestamps on installation Won't fix: - makeinstall is needed, the Makefile doesn't support DESTDIR - dist tag use is not mandatory - /var/www/user is appropriate for webalizer, since it generates content there
(In reply to comment #2) > - /var/www/user is appropriate for webalizer, since it generates content there It depends on the configuration. And there is a rule not to touch /var/www/, because user may already have content in it: http://fedoraproject.org/wiki/Packaging/Guidelines#head-5d1681fa7cf3714ad490fbf7c095a0cfe16da27f Moreover those images don't seem to be used in the output? Maybe the best would be not to ship the images, or ship them in %doc. And don't own the directory either.
Also webalizer is enabled by default, with the /etc/cron.daily/00webalizer file. I am not convinced that it is right. Nothing seems to depend on webalizer, but still I think it is wrong to enable it without admin action. For example the admin should have the possibility to change the directory webalizer writes to before it is run first. Having webalizer enabled by default allows to have it working off-the-box, but in that case I am not sure that it is right. There is a guideline (may actually be only through rpmlint) that daemons should not be enabled in the default case, I think it is sane, and somehow apply here. Another possibility would be to have a separate package containing the bits that enable having it run in by default. For example webalizer-cron. That package only would depend on webserver and crontabs. Also webserver may not be the right dependency, since the webserver bits seems to be apache (httpd) specific.
cron scripts are not daemons, it's certainly standard to enable cron scripts by default. If you want some way to be able to disable the webalizer cron script but keep the package installed intact please file a separate RFE, this is not something required by the packaging guidelines. s/webserver/httpd seems reasonable. usage.png *is* used in the generated content.
(In reply to comment #5) > cron scripts are not daemons, it's certainly standard to enable cron scripts by > default. If you want some way to be able to disable the webalizer cron script > but keep the package installed intact please file a separate RFE, this is not > something required by the packaging guidelines. The submission review is certainly not only to check packaging guidelines. Every issue found should be reported, the objective is to have the best possible package. It also avoids submitting a lot of bugs and do things globally. The issue of integration in fedora, for example is as important as the guidelines, though less easy to formalize. > usage.png *is* used in the generated content. Sorry, I missed it. The issue about /var/www is still there. Although I don't know exactly how it should be solved.
I reply to comment #2: %makeinstall can be dropped and the rpm still builds. make in the %build section already installs the binaries. In reply to comment #3: Patrice, isn't /var/www/usage a better place for generated content, since /usr might be on a read-only filesystem? A few more comments: - Replace /var with %{_localstatedir} and /etc with %{_sysconfdir} everywhere - Use %{SOURCE1} and %{SOURCE3} - Mark /etc/webalizer.conf as %config(noreplace)
Can you also fix? W: webalizer mixed-use-of-spaces-and-tabs (spaces: line 73, tab: line 94)
(In reply to comment #7) > In reply to comment #3: Patrice, isn't /var/www/usage a better place for generated content, since > /usr might be on a read-only filesystem? Yes, that's why I said that I don't know it should be solved. If there was no need to install the icon the simplest way would be not to create the /var/www/usage directory and let the admin do it, but since there is a need for that image file I don't know what should be done. Still something should be done. Maybe the best thing would be to ask on the maintainer list? I guess other packages have similar issues.
Ping?
* Thu Feb 7 2008 Joe Orton <jorton> 2.01_10-35 - use %{_localestatedir}, remove tabs, require httpd not webserver, mark webalizer.conf config(noreplace) (#226536) I think this addresses all outstanding issues.
Not really. I had a more precise look at the code/output, and it seems that the 2 images shipped in the package are not used. webalizer.png is not used at all, while msfree.png is used as an example in a commented out section of the config file. So I think that they should definitively be in %doc if shipped at all. The issue of webalizer running as soon as it is installed is still an issue in my opinion. Just like webalizer installing /var/www/usage and filling it as soon as it is installed. There are many ways to fix that. I see 2 possibilities: * have a separate webalizer-cron package which owns /var/www/usage and /etc/cron.daily/00webalizer /etc/httpd/conf.d/webalizer.conf with a very clear %description stating that it modifies httpd configuration and starts automatically webalizer. * have /etc/cron.daily/00webalizer read /etc/sysconfig/webalizer and have a variable in /etc/sysconfig/webalizer that enables webalizer, and ship /etc/httpd/conf.d/webalizer.conf in %doc.
Hi Patrice, Do you want to take over this review? Thanks, Ruben
(In reply to comment #13) > Hi Patrice, > > Do you want to take over this review? As you want. I don't care about being the formal reviewer or not, in any case I consider that my comments have the same value whether or not I am the formal reviewer, and since you take them into account, no problem.
Ok, Joe, what do you think about Patrice's comments?
Patches to add & use a sysconfig file to allow the cron script to be disabled are welcome.
I'll do that, but not now, since I am quite busy. Maybe by the end of the week.
Created attachment 298203 [details] conditionnally run the cron script
Created attachment 298204 [details] sysconfig file
I removed the cnoditional if [ -s /var/log/httpd/access_log ]; then since the logs may be in another directory. What about the images shipped only as %doc since they are not used?
Other issues/suggestions: * use %{SOURCEX} instead of RPM_SOURCE_DIR. Not a must but it is the usual practice. * use rpm macros to substitute hard-coded paths in cron and conf file * don't have /var/www/usage in the main package but in an additional package. * use current add user script http://fedoraproject.org/wiki/Packaging/UsersAndGroups * --with-dblib looks wrong * --with-etcdir=%{_sysconfdir}
As usual I can do patch for these issues too, but please apply or reject the other patch before.
Joe, can you have a look?
Joe, ping?
The patches look fine, please go ahead and commit. Thanks!
Ok, I've applied the patches and have taken the liberty to do a build for rawhide.
Joe, would you mind looking at Patrice's comments in #21?
Please feel free to implement improvements as you see fit.
Uh sorry, but unless I'm mistaken how this is supposed to work is that the reviewer reviews the package, and the maintainer does the improvements :-)
Well, it's a merge review so it's kind of weird. Given that the package is already in the distribution, the maintainer has no incentive to actually fix problems. You can get a provenpackager to do it but the problem then is that many are unwilling to do things that they can't easily test, since if things to wrong that just makes more work for the maintainer.
Yes, I agree it's weird :) But if I (as reviewer and provenpackager) make some changes, who's gonna review those?...
Make a patch, have a second reviewer look at that patch, have them ok/fail the patch in this ticket, and apply the patch? [Just suggestions trying to help]
If you dump random feature requests in a "Merge Review" bug and think that will inspire me to stop working on *bugs which actually affect our users* and jump to it, you are sadly mistaken. In fact, it only winds me up. If you want to improve the package, improve the package. If you are not a provenpackager, request commit privs and I'll ack them. If you commit changes I will get mailed the diffs and can review them. This stuff is not rocket science.
Ok, that's not nice. I've been very patient for about 4 years now, and I jusked asked you to look at some comments. I never "dumped random feature requests" on you, and just like you I rather spend time working on real bugs in my own packages. If you don't have time to maintain webalizer, just orphan it and let someone pick it up who actually cares. Anyway, find yourself another reviewer. I have more important things to do.
Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket: https://fedorahosted.org/fesco/ticket/1269 If you don't know what merge reviews are about, please see: https://fedoraproject.org/wiki/Merge_Reviews How to handle this bug is left to the discretion of the package maintainer.
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle. Changing version to '23'. (As we did not run this process for some time, it could affect also pre-Fedora 23 development cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.) More information and reason for this action is here: https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23
Hello , I'm the new maintainer of webalizer, can we finish fix this ? (In reply to Ruben Kerkhof from comment #35) > and I > jusked asked you to look at some comments. I never "dumped random feature > requests" on you what is pending ? (In reply to Patrice Dumas from comment #22) > As usual I can do patch for these issues too, but please apply > or reject the other patch before. yes, I'd love have patch to just apply ... Best regards.
This message is a reminder that Fedora 23 is nearing its end of life. Approximately 4 (four) weeks from now Fedora will stop maintaining and issuing updates for Fedora 23. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a Fedora 'version' of '23'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, simply change the 'version' to a later Fedora version. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora 23 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora, you are encouraged change the 'version' to a later Fedora version prior this bug is closed as described in the policy above. Although we aim to fix as many bugs as possible during every release's lifetime, sometimes those efforts are overtaken by events. Often a more recent Fedora release includes newer upstream software that fixes bugs or makes them obsolete.
webalizer-2.23_08-4.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-c2db08003c
webalizer-2.23_08-4.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-c2db08003c
Fedora 23 changed to end-of-life (EOL) status on 2016-12-20. Fedora 23 is no longer maintained, which means that it will not receive any further security or bug fix updates. As a result we are closing this bug. If you can reproduce this bug against a currently maintained version of Fedora please feel free to reopen this bug against that version. If you are unable to reopen this bug, please file a new report against the current release. If you experience problems, please add a comment to this bug. Thank you for reporting this bug and we are sorry it could not be fixed.
webalizer-2.23_08-4.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.