[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

--- Comment #7 from Christopher Meng <cickumqt@xxxxxxxxx> ---
(In reply to comment #6)
> Please Require: logrotate, so we can ensure that /etc/logrotate.d will
> exist before monitorix tries to drop its file in there.

Fixed...(Oh....Damn...)

> 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".

Monitorix enables its own built-in HTTP server by default, so upstream thought
that it's better to rely on it instead obligating to the user to install a
third party web server like Apache. Upstream recommended me that setting
 /var/share/monitorix/imgs/ as 755 and give the user 'nobody' the write
permissions.

So is it well enough?


> 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)".

These two Removed.

> 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\\(

I just added it but this time yum said:

Error: Package: monitorix-3.1.0-3.fc20.noarch (/monitorix-3.1.0-3.fc20.noarch)
           Requires: perl(Monitorix)
Error: Package: monitorix-3.1.0-3.fc20.noarch (/monitorix-3.1.0-3.fc20.noarch)
           Requires: perl(HTTPServer)

So what should I do?

> 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

Added in %prep.

> 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.

Upstream said it's a typo and will be removed in the next release.I've added a
sed line in %prep.

> 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.

Upstream said that this may be helpful on some CentOS systems, so I just
removed it. Fixed.

New place:

SPEC: http://cicku.me/monitorix.spec
SRPM: http://cicku.me/monitorix-3.1.0-3.fc20.noarch.rpm

-- 
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=ryQCtwFTRe&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]