Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 175748 - Review Request: cacti
Summary: Review Request: cacti
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Aurelien Bompard
QA Contact: David Lawrence
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-12-14 16:06 UTC by Mike McGrath
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-02-08 15:30:26 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Mike McGrath 2005-12-14 16:06:48 UTC
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-2.src.rpm
Description:  
Cacti is a complete frontend to RRDTool. It stores all of the
necessary information to create graphs and populate them with
data in a MySQL database. The frontend is completely PHP
driven. Along with being able to maintain graphs, data
sources, and round robin archives in a database, Cacti also
handles the data gathering. There is SNMP support for those
used to creating traffic graphs with MRTG.

Misc: This is my first package and I would like a sponsor.

Comment 1 Aurelien Bompard 2005-12-17 10:24:51 UTC
Needs work:
* Use of buildroot is not consistant
  (wiki: PackagingGuidelines#UsingBuildRootOptFlags)
* No downloadable source. Please give the full URL in the Source tag.
* Group Application/System is not valid, it's "Applications/System" (with an s)
* /usr/share/cacti/rra/.placeholder and the log file are chmod 750 because of
the %attr definition in %files. The files should not be executable, so you
should change the lines in %files to :
%dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/log/
%attr(0644,cacti,cacti) %{_datadir}/%{name}/log/*
%dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/rra/
%attr(0644,cacti,cacti) %{_datadir}/%{name}/rra/.placeholder
* Is is necessary to make the log dir non world-readable ? I don't see important
information there. Plus, the log file has to be readable by the webserver for
the "View Log" function to work (in "System Utilities"). In the above example,
I've set the dir 755 and the log file 644.
* This is not FHS complient :
 - move include/config.php in /etc/cacti/config.php
 - move log/ to /var/log/cacti
 - move rra to /var/lib/cacti/rra/
And symlink the files back in /usr/share/cacti
I would also move scripts/ to /var/lib/cacti/scripts, since it is very common
for cacti users to add scripts. What do you think ?
* In %prep, use %setup -q to avoid file listing
* Please add a logrotate file for the log. Example:
/var/log/cacti/cacti.log {
    monthly
    notifempty
    missingok
    compress
    create 0644 cacti cacti
}
* File listed twice: /usr/share/cacti/include/config.php
* Missing dependancy on /sbin/service for %postun (package initscripts)
* /usr/bin should be changed to %{_bindir}
* Why "cp -avx" ? cp -a is enough
* Don't hardcode uid 329 in useradd, it may be already used. Just let useradd
find an available uid
* You need mailx and MTA as Requires(post) because of the mail in %post
* Changed Prereq to Requires(pre)
* No need for php in %pre, you can remove it from Requires(pre)

Comment 2 Mike McGrath 2005-12-17 18:25:05 UTC
I don't disagree with any of these.  I'll post the changes soon.  I guess I was
being a little loverly parrinoid about the log files.  Going through my own I
haven't seen anything too important.

Comment 3 Mike McGrath 2005-12-18 05:59:41 UTC
I've made these updates.  Couple of issues:

Simply running config.php out of /etc/ does not work as there are includes that
fail, and copying the entire include directory seems inapproperate.  I could
write a patch?  In the meantime I made a link from the config.php to
/etc/cacti/config.php so it can at least be found there. Any thoughts?

I'm also curious about others thoughts on running this as a Cacti user.  Would
it be more approperate to just run it as Apache?

Either way the spec and srpm are both updated and in their original spots:

Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-2.src.rpm

Comment 4 Warren Togami 2005-12-18 06:07:11 UTC
When you update a SRPM, please always increment the Release number, do not keep
it the same.

Comment 5 Aurelien Bompard 2005-12-18 15:20:12 UTC
> Simply running config.php out of /etc/ does not work as there are includes 
> that fail, and copying the entire include directory seems inapproperate.  I 
> could write a patch?

If it's very easy and obvious, yes, write a patch please. Users should not have
to edit configuration files in /usr/share

> I'm also curious about others thoughts on running this as a Cacti user.  Would
> it be more approperate to just run it as Apache?

You mean the cron job ? I think it's best not to run it as the apache user, so
that the apache user does not have write access to the RRA files in case of
compromise (and there have been such vulnerabilities in cacti's history)

* in %pre : /usr/sbin -> %{_sbindir}
* /var -> %{_localstatedir} in the logrotate file
* mailx and MTA should be in Requires(post)
* /sbin/service should be in Requires(postun)
* Using %doc in %files does the copying of files, so you don't have to do it in
%install

Minor:
* the scripts don't have to be owned by the cacti user, but that's not a problem
(it just spits out more warnings in rpmlint)
* you don't _have_ to use %{__install}, %{__mkdir}, %{__cp} and %{__chmod},
using install, mkdir, etc... is enough. Do what you prefer, it's up to you.

Comment 6 Mike McGrath 2005-12-18 17:23:13 UTC
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-3.noarch.rpm

I've updated many of those changes (including minor).  I ended up taking the
mail out of the RPM all together, I don't remember getting emails from other
packages saying "hey configure me here"

I've taken the database config portion out of the config.php file and placed it
in /etc/cacti/db.php  It seems like nothing else in config.php would need to be
edited for normal use.  

Thanks for all your help on this, I plan on submitting more packages in the
future so feel free to straighten me out on things that are not best practice.

Comment 7 Aurelien Bompard 2005-12-18 18:03:46 UTC
> I ended up taking the mail out of the RPM all together, I don't
> remember getting emails from other packages saying "hey configure me here"

Agreed. Seemed "unnatural" to me too.

> I've taken the database config portion out of the config.php file and placed
> it in /etc/cacti/db.php  It seems like nothing else in config.php would need
> to be edited for normal use.

Very nice. There are a few additional config vars for some HTML colors in
config.php, but we can ignore this. Actually, I wish upstream had splitted the
db config out of config.php. Could you submit your patch upstream (with the
/etc/cacti/db.php paths replaced by include/db.php) ?

A few last things :
* remove /sbin/service from Requires (it's already in Requires(postun))
* in %install, first you %__install the .placeholder file, then you empty it
  with echo. Why ? I think that is file is here just to prevent winzip from not
  including the rra dir, so you should not bother about it (I think you can even
  remove it entirely).


Comment 8 Mike McGrath 2005-12-18 21:41:07 UTC
These changes are ready, I've submitted an include/db.php patch to the Cacti
team.  Also while looking for their bug system I found 4 patches for 0.8.6g and
have included them in this release.  

Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-4.src.rpm



Comment 9 Aurelien Bompard 2005-12-19 07:54:44 UTC
* in the cron job, replace /usr/bin by %{_bindir}
* to make it clear that the patches come from upstream, please give the full url
  (ex:
http://www.cacti.net/downloads/patches/0.8.6g/short_open_tag_parse_error.patch)
  They won't start with "cacti-0.8.6g-", but it is more important to be able to
  verify them against upstream.
* drop the empty %build section
* concerning the apache restart in the scriptlets :
  1. we want to condrestart apache on install, in order for our new cacti.conf
     to be taken into account.
  2. we want to condrestart apache on upgrade, in case we changed cacti.conf
  3. we want to condrestart apache on uninstall, so that apache sees that the
     cacti.conf file has been removed.
  For 1 we need a post scriptlet like this :
%post
if [ $1 == 1 ]; then
  /sbin/service httpd condrestart >/dev/null 2>&1
fi
  We can combine 2 and 3 into a simple %postun scriptlet without tests :
%postun
/sbin/service httpd condrestart >/dev/null 2>&1
  See http://fedoraproject.org/wiki/ScriptletSnippets for more info on which
  tests to run

We are having a larger problem now. SELinux blocks httpd from accessing /var/log
and /var/lib. The RRAs are correctly created by the poller script, but can't be
seen in the web interface. I'm posting in fedora-selinux-list to get advice.

Comment 10 Mike McGrath 2005-12-19 16:08:44 UTC
I've got these changes in.  I'll wait to post them until we can figure out what
to do about SELinux.  I'll be monitoring the thread:

https://www.redhat.com/archives/fedora-selinux-list/2005-December/msg00124.html

Comment 11 Mike McGrath 2005-12-21 16:16:27 UTC
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-6.src.rpm

I've made the changes above and used the "quick hack" from the SELinux thread. 
I'm sure there's got to be a more approperate way to do this.  Any ideas anyone?



Comment 12 Mike McGrath 2006-01-05 00:44:10 UTC
Welp, selinux and FHS don't always get along :-D  I've moved all the log files
and associated web files to /var/www/cacti/ until we come up with a better way
to do it.

SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6g-7.src.rpm

Comment 13 Mike McGrath 2006-01-17 23:33:25 UTC
SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-3.src.rpm

Changes:
-Moved log files back to /var/log/cacti
-Moved Scripts back to /var/lib/cacti/scripts
-Moved rra files back to /var/lib/cacti/rra
-Changed logfile to (664,cacti,apache) so the "clear log file" will work
-Upstream had a new version come out
-Upstream already had 4 patches for that version

SELinux suggestion made by Paul Howarth sounds very reasonable:
https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html

If Cacti gets approved I'll submit a bug report to SELinux and try to get it
incorperated into the policy.  I'll use the FC5 modules once FC5 comes out (if
it applies)


Comment 14 Aurelien Bompard 2006-02-04 10:14:28 UTC
Sorry for not getting back earlier. In you last spec file, seem to have moved
files back to /usr/share and /var/lib.
In order to avoid the SELinux problem, I think we should keep everything in
/var/www, as you said in comment 12.

Comment 15 Mike McGrath 2006-02-04 15:16:18 UTC
I can move it back but do you see any reason Paul's suggestion won't work? 
(From Comment 13)

https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html


Comment 16 Aurelien Bompard 2006-02-06 15:19:01 UTC
OK, it looks good. Minor improvements:
- add " || : " at the end of the scriptlets to make sure they always return 0
- the logrotate file contains a "//" in the logfile path
- please add a README.Fedora with something like that :
If you have SELinux enabled, you have to run the following commands :
chcon -R -t httpd_sys_content_t /var/log/cacti/
chcon -R -t httpd_sys_content_t /var/lib/cacti/rra/

This README could be removed when cacti is added into the policy.

If you agree with the above, make the changes and I'll approve it

Comment 17 Mike McGrath 2006-02-06 15:47:19 UTC
Sounds good to me, all things added:

%changelog
* Mon Feb 6 2006 Mike McGrath <imlinux> - 0.8.6h-4
- Fixed some scriptlets to always return 0
- Fixed extra '/' in logrotate
- Added README.Fedora

SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-4.src.rpm

Comment 18 Aurelien Bompard 2006-02-06 22:38:20 UTC
Review for release 4:
* RPM name is OK
* Source cacti-0.8.6h.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK
* Works fine
APPROVED

Again, sorry for keeping you waiting.

Please post here the bug number for SELinux integration when you file it.


Comment 19 Mike McGrath 2006-02-08 15:30:26 UTC
I've created an SELinux bug: 

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=180482

Also cacti is now available in devel.  (Soon FC3 and FC4)


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