Bug 226536 - Merge Review: webalizer
Merge Review: webalizer
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: webalizer (Show other bugs)
23
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Sergio Monteiro Basto
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:16 EST by Nobody's working on this, feel free to take it
Modified: 2016-12-20 15:18 EST (History)
6 users (show)

See Also:
Fixed In Version: webalizer-2.23_08-4.el7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-12-20 07:00:38 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
conditionnally run the cron script (2.12 KB, patch)
2008-03-16 14:19 EDT, Patrice Dumas
no flags Details | Diff
sysconfig file (107 bytes, text/plain)
2008-03-16 14:20 EDT, Patrice Dumas
no flags Details

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

http://cvs.fedora.redhat.com/viewcvs/devel/webalizer/
Initial Owner: jorton@redhat.com
Comment 1 Ruben Kerkhof 2007-03-18 06:53:59 EDT
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)
Comment 2 Joe Orton 2007-03-19 06:16:26 EDT
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
Comment 3 Patrice Dumas 2007-03-19 06:32:57 EDT
(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.
Comment 4 Patrice Dumas 2007-03-19 08:00:05 EDT
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.
Comment 5 Joe Orton 2007-03-19 08:52:32 EDT
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.
Comment 6 Patrice Dumas 2007-03-19 15:50:59 EDT
(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.
Comment 7 Ruben Kerkhof 2007-03-19 16:17:53 EDT
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)




 
Comment 8 Ruben Kerkhof 2007-03-19 16:22:10 EDT
Can you also fix?
W: webalizer mixed-use-of-spaces-and-tabs (spaces: line 73, tab: line 94)
Comment 9 Patrice Dumas 2007-03-19 17:07:15 EDT
(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.
Comment 10 Ruben Kerkhof 2007-06-17 03:16:51 EDT
Ping?
Comment 11 Joe Orton 2008-02-07 11:26:15 EST
* Thu Feb  7 2008 Joe Orton <jorton@redhat.com> 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.
Comment 12 Patrice Dumas 2008-02-09 06:18:44 EST
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.
Comment 13 Ruben Kerkhof 2008-02-21 14:23:59 EST
Hi Patrice,

Do you want to take over this review?

Thanks, Ruben
Comment 14 Patrice Dumas 2008-02-23 08:01:44 EST
(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.
Comment 15 Ruben Kerkhof 2008-03-08 12:24:24 EST
Ok, Joe, what do you think about Patrice's comments?
Comment 16 Joe Orton 2008-03-10 06:39:18 EDT
Patches to add & use a sysconfig file to allow the cron script to be disabled
are welcome.
Comment 17 Patrice Dumas 2008-03-10 06:49:41 EDT
I'll do that, but not now, since I am quite busy. Maybe by the
end of the week.
Comment 18 Patrice Dumas 2008-03-16 14:19:21 EDT
Created attachment 298203 [details]
conditionnally run the cron script
Comment 19 Patrice Dumas 2008-03-16 14:20:33 EDT
Created attachment 298204 [details]
sysconfig file
Comment 20 Patrice Dumas 2008-03-16 14:22:08 EDT
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?
Comment 21 Patrice Dumas 2008-05-15 07:21:32 EDT
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}
Comment 22 Patrice Dumas 2008-05-15 09:22:42 EDT
As usual I can do patch for these issues too, but please apply
or reject the other patch before.
Comment 23 Ruben Kerkhof 2008-10-19 08:46:29 EDT
Joe, can you have a look?
Comment 24 Robert Scheck 2008-12-20 09:12:20 EST
Joe, ping?
Comment 25 Ruben Kerkhof 2010-01-31 09:33:46 EST
Joe, ping?
Comment 26 Joe Orton 2010-01-31 11:38:45 EST
The patches look fine, please go ahead and commit.  Thanks!
Comment 27 Ruben Kerkhof 2010-10-17 14:33:21 EDT
Ok, I've applied the patches and have taken the liberty to do a build for rawhide.
Comment 28 Ruben Kerkhof 2011-02-11 11:48:58 EST
Joe, would you mind looking at Patrice's comments in #21?
Comment 29 Joe Orton 2011-02-11 12:12:03 EST
Please feel free to implement improvements as you see fit.
Comment 30 Ruben Kerkhof 2011-02-11 12:32:33 EST
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 :-)
Comment 31 Jason Tibbitts 2011-02-15 16:04:01 EST
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.
Comment 32 Ruben Kerkhof 2011-02-15 16:08:21 EST
Yes, I agree it's weird :) But if I (as reviewer and provenpackager) make some changes, who's gonna review those?...
Comment 33 Stephen John Smoogen 2011-02-15 16:42:01 EST
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]
Comment 34 Joe Orton 2011-02-16 03:46:32 EST
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.
Comment 35 Ruben Kerkhof 2011-02-16 08:44:34 EST
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.
Comment 36 Cole Robinson 2015-02-11 15:39:22 EST
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.
Comment 37 Jan Kurik 2015-07-15 11:23:13 EDT
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
Comment 38 Sergio Monteiro Basto 2016-11-23 09:50:52 EST
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.
Comment 39 Fedora End Of Life 2016-11-24 05:22:05 EST
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.
Comment 40 Fedora Update System 2016-12-01 18:41:55 EST
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
Comment 41 Fedora Update System 2016-12-03 06:18:46 EST
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
Comment 42 Fedora End Of Life 2016-12-20 07:00:38 EST
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.
Comment 43 Fedora Update System 2016-12-20 15:18:03 EST
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.

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