[Bug 947071] Review Request: monitorix - A free, open source, lightweight system monitoring tool

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=947071

Ken Dreyer <ktdreyer@xxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ktdreyer@xxxxxxxxxxxx

--- Comment #6 from Ken Dreyer <ktdreyer@xxxxxxxxxxxx> ---
Hi Christopher,

(This is just a nitpick: please offer links that provide direct downloads,
so that wget, or the fedora-review tool, can properly process them.) On to
the comments!

Please Require: logrotate, so we can ensure that /etc/logrotate.d will
exist before monitorix tries to drop its file in there.

Even though upstream sets 777 on /var/share/monitorix/imgs/ in order to
support multiple web servers, it's not acceptable for Fedora to ship a
directory wide open like that (particularly one that's web-accessible).
The easiest option is to just set "Requires: httpd" and give "apache"
write permissions on the directory. It's a nice goal to support multiple
web servers with our web apps, but that's going to require broader changes
in Fedora than what we have available today. In this case it's best to
choose "secure by default".

For Perl, please note that RPM is going to automatically add most of your
dependencies already. To verify this, run rpm -qp --requires on your
binary RPM. You'll see that RPM has already detected and added many of the
requirements, and in fact, when you manually specify them in the .spec
file, RPM is adding the requirement twice. For example:

  perl(MIME::Lite)
  perl(MIME::Lite)

or 

  perl-libwww-perl
  perl(LWP::UserAgent)

Both of those LWP lines are equivalent. It's much better to let RPM just
automatically determine the dependencies, so I recommend going through the
--provides list and removing any of these doubly-listed dependencies. For
the LWP example above, you can simply remove your "Requires:
perl-libwww-perl" line.

You can also remove the explicit requirement on rrdtool-perl, because RPM
is automatically adding a dependency on "perl(RRDs)".

On a similar note, RPM is erroneously concluding that your package
provides a lot of Perl modules.  (Run rpm -qp --provides on your binary
RPM.) You'll need to filter those out. Add the following (I like to put these
sort of definitions directly above %description):

  # We don't actually provide Perl modules for other packages to use.
  %global __provides_exclude perl\\(

You'll need to remove the #!/usr/bin/env shebangs and use #!/usr/bin/perl
instead.

sed -i 's|/usr/bin/env perl|/usr/bin/perl|g' monitorix
sed -i 's|/usr/bin/env perl|/usr/bin/perl|g' monitorix.cgi

I'm surprised that the .pm files have shebangs at all, and I would ask
upstream if the .pm files' shebangs could be removed altogether. Ideally you
should not see "/usr/bin/env" in the --requires at all.

I'm not sure perl-MailTools is really a requirement, because I could find no
reference to the Mail:: modules in the code. I recommend checking with
upstream about whether that dependency could be dropped.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=DhdTdZaYHa&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]