Spec URL: https://www.dropbox.com/s/juxvii8r7aapar3/monitorix.spec SRPM URL: https://www.dropbox.com/s/ci4gapzmk3gs8tf/monitorix-3.1.0-1.fc20.src.rpm Description: Monitorix is a free, open source, lightweight system monitoring tool designed to monitor as many services and system resources as possible. It has been created to be used under production Linux/UNIX servers, but due to its simplicity and small size may also be used on embedded devices as well. Fedora Account System Username: cicku
According to the files, the license is GPLv2+. This seems to be a noarch package and should be labeled accordingly. Use the name macro for your other sources as well. You must BR systemd, according to http://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations Take a look at the Perl guidelines on how and if to require Perl modules manually. I think you should create a monitorix system user. The logrotate file doesn't match the actual file and directory layout. It should also have a su line. Your package should either own %{_sysconfdir}/logrotate.d/ or require logrotate "rm -rf %{buildroot}" in the install section should be dropped, as it is no longer necessary. %{_localstatedir}/lib/monitorix/reports/*.html -- You're not owning this directory. I also wonder why html should be configuration. Replace the ".gz" from the manpages with an asterisk, to allow for the compression method to change. What's the point of 777 permissions for that one directory? Your changelog entry has an invalid form. See http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs As a sidenote, you could shorten your install section by using the -D option of the install command. You must also use -p to preserve the timestamps.
HTML should be modified by users because html is a monthly report file which contains: <a href="you">you</a><br> <a href="http://www.example.com">http://www.example.com</a> This section must be changed by users in order to match their personal use. For 777 permissions problem, I'll contact upstream. Thnaks for the review! I'll upload a 2nd version later.
(In reply to comment #1) > According to the files, the license is GPLv2+. Fixed. > > This seems to be a noarch package and should be labeled accordingly. > Added. > Use the name macro for your other sources as well. > I've changed all "monitorix" strings to %{name},correct? > You must BR systemd, according to > http://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations > Fixed. > Take a look at the Perl guidelines on how and if to require Perl modules > manually. > Still have question with: perl-libwww-perl Should I changed it to "perl(LWP)" or keep current status? > I think you should create a monitorix system user. The logrotate file > doesn't match the actual file and directory layout. It should also have a su > line. Your package should either own %{_sysconfdir}/logrotate.d/ or require > logrotate > Upstream said: "I don't know if we really need the 'monitorix' user, if so it will be only used for the built-in HTTP server, which is currently using the 'nobody' user by default. For the rest of task, Monitorix needs the 'root' user in order to have enough permissions to collect data. " Well, I forgot to bring a logrotate file written by myself into package as source2... If there still has problems, please tell me. > "rm -rf %{buildroot}" in the install section should be dropped, as it is no > longer necessary. Fixed. > %{_localstatedir}/lib/monitorix/reports/*.html -- You're not owning this > directory. I also wonder why html should be configuration. Removed. > Replace the ".gz" from the manpages with an asterisk, to allow for the > compression method to change. Replaced. > What's the point of 777 permissions for that one directory? Upstream: "The only directory that needs 777 permissions is /var/share/monitorix/imgs/. This is because the user in which will run the web server will have to write there to create the graphs (.png files). Since we cannot know which web server will be used to see the graphs (the built-in HTTP server that comes with Monitorix, or Apache, or Nginx, or Lighttpd, etc.) then we cannot know which user will try to write inside the directory. That's the reason why I used 777 for that directory. " > Your changelog entry has an invalid form. See > http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs I don't know the current one is ok or not. Please have a look again. I remember a tool help generate changelog but I think copy paste is just ok with no syntax changes. > As a sidenote, you could shorten your install section by using the -D option > of the install command. You must also use -p to preserve the timestamps. Used.I tried as possible as can to shorten my install section, if there still has shortenable space, please tell me. Thanks again!
2nd spec:https://www.dropbox.com/s/ftg190kzw7b7ol8/monitorix.spec 2nd SRPM:https://www.dropbox.com/s/4q4igq9y2g85aze/monitorix-3.1.0-2.fc20.src.rpm
Sorry, I've written a wrong dependency Require. Fixed. New spec:https://www.dropbox.com/s/juxvii8r7aapar3/monitorix.spec New SRPM:https://www.dropbox.com/s/p33pscgg4uyemrs/monitorix-3.1.0-2.fc20.src.rpm
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.
(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
Koji success: http://koji.fedoraproject.org/koji/taskinfo?taskID=5316665
MUSTFIX: This package installs its perl modules under %{_libdir}/%{name}. This is problematic twice: - Noarch packages must not install files under %{_libdir}/%{name} - /usr/bin/monitorix searches for its modules under /usr/lib/monitorix: /usr/bin/monitorix: ... use lib $Bin . "/lib", "/usr/lib/monitorix"; ... => Change your spec to install the perl-modules into /usr/lib/%{name} instead of %{_libdir}/%{name} Alternatively, modify the package to install the perl modules into %{perl_vendorlib}/%{name}.
(In reply to comment #9) Fixed by changing to /usr/lib New SRPM URL: http://cicku.me/monitorix-3.1.0-4.fc20.src.rpm
I've sponsored Christopher.
(In reply to comment #11) > I've sponsored Christopher. Thanks.
Upstream has released a newer version: New SPEC URL: http://cicku.me/monitorix.spec New SRPM URL: http://cicku.me/monitorix-3.2.0-1.fc20.src.rpm
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== MUST items ===== Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package requires other packages for directories it uses. [x]: Package uses nothing in %doc for runtime. [x]: Package is not known to require ExcludeArch. [x]: Package complies to the Packaging Guidelines [x]: License field in the package spec file matches the actual license. [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [x]: If the package is a rename of another package, proper Obsoletes and Provides are present. [!]: Package must own all directories that it creates. Note: Some directories are not owned: /usr/lib/%{name}/ %{_localstatedir}/lib/%{name}/ %{_localstatedir}/lib/%{name}/reports/ %{_datadir}/%{name}/ %{_datadir}/%{name}/cgi/ [x]: Package does not own files or directories owned by other packages. [!]: Requires correct, justified where necessary. Note: * No need to explicitly require rrdtool * Requires perl(HTTP::Server::Simple) is false, it actually uses perl(HTTP::Server::Simple::CGI) which has been added to Requires by rpmbuild automatically. * Requires perl(DBD::mysql) is needed in mysql.pm, just Requires perl(DBI) is not enough [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Large documentation must go in a -doc subpackage. [!]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. Note: missed BuildRequires perl [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: %config files are marked noreplace or the reason is justified. [x]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: No %config files under /usr. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). Perl: [x]: Package contains the mandatory BuildRequires and Requires:. ===== SHOULD items ===== Generic: [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [!]: Final provides and requires are sane (see attachments). Note: The Perl modules provided should be filtered. The Requires perl(Monitorix) and perl(HTTPServer) should be filtered. https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [-]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SourceX tarball generation or download is documented. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: monitorix-3.2.0-1.fc18.noarch.rpm monitorix.noarch: W: only-non-binary-in-usr-lib 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint monitorix monitorix.noarch: W: only-non-binary-in-usr-lib 1 packages and 0 specfiles checked; 0 errors, 1 warnings. # echo 'rpmlint-done:' Requires -------- monitorix (rpmlib, GLIBC filtered): /bin/sh /usr/bin/perl config(monitorix) logrotate perl perl(:MODULE_COMPAT_5.16.3) perl(CGI) perl(Config::General) perl(Cwd) perl(DBI) perl(Exporter) perl(File::Basename) perl(FindBin) perl(Getopt::Std) perl(HTTP::Server::Simple) perl(HTTP::Server::Simple::CGI) perl(HTTPServer) perl(LWP::UserAgent) perl(MIME::Lite) perl(Monitorix) perl(POSIX) perl(RRDs) perl(Socket) perl(XML::Simple) perl(base) perl(constant) perl(lib) perl(strict) perl(warnings) rrdtool systemd Provides -------- monitorix: config(monitorix) monitorix perl(HTTPServer) perl(Monitorix) perl(apache) perl(bind) perl(disk) perl(fail2ban) perl(fs) perl(ftp) perl(hptemp) perl(icecast) perl(int) perl(kern) perl(lighttpd) perl(lmsens) perl(mail) perl(mysql) perl(net) perl(nfsc) perl(nfss) perl(nginx) perl(ntp) perl(nvidia) perl(port) perl(proc) perl(raspberrypi) perl(serv) perl(squid) perl(system) perl(traffacct) perl(user) Source checksums ---------------- http://www.monitorix.org/monitorix-3.2.0.tar.gz : CHECKSUM(SHA256) this package : 512a145431aba68a59f79e75e9d8400f878c5b6050c03be5c5542f6590e3d1d0 CHECKSUM(SHA256) upstream package : 512a145431aba68a59f79e75e9d8400f878c5b6050c03be5c5542f6590e3d1d0 Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review -b 947071 ===== Manual review items ===== * Use upstream provided systemd unit file and logrotate file in docs/ * Patch shebang with stricter regex: sed -i 's|#!/usr/bin/env perl|#!/usr/bin/perl|' monitorix * Consider packaging Apache and Lighttpd config files in sub-packages, follow the way like the package 'anyterm'
Hi Robin, 1)Upstream provide a not good system unit file IMO, and logrotate file also. 2)Fixed. 3)I don't know if I create the sub packages I should Add Requires for httpd or lighthttpd?
(In reply to Christopher Meng from comment #15) > Hi Robin, > > 1)Upstream provide a not good system unit file IMO, and logrotate file also. Since upstream provides equivalent files, you should provide patches instead of replacing files. And then patches must be sent upstream and each come with a comment in the specfile. https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment > 3)I don't know if I create the sub packages I should Add Requires for httpd > or lighthttpd? I don't fill splitting sub-packages is a necessary and perfect enhancement for this package. Just go ahead with other issues. After all, you must check the [!] items in the review.
OK. I dropped the %{S:1,2} and use the default one. Add %dir for directories ownership Fix filtering. New SPEC URL: http://cicku.me/monitorix.spec New SRPM URL: http://cicku.me/monitorix-3.2.0-2.fc20.src.rpm
Approved by cheeselee. Some additional remind: * Write more serious and meaningful changelog. * Write a line of comment for why hard coding /usr/lib instead of %{_libdir} for this package.
(In reply to Robin Lee from comment #18) > Approved by cheeselee. > > Some additional remind: > * Write more serious and meaningful changelog. Will be done in SCM. > * Write a line of comment for why hard coding /usr/lib instead of %{_libdir} > for this package. Oh, I should ask the author......
New Package SCM Request ======================= Package Name: monitorix Short Description: A free, open source, lightweight system monitoring tool Owners: cicku Branches: f18 f19 InitialCC:
Git setup complete.
(In reply to Jon Ciesla from comment #21) > Git setup complete. Jon, it seems you used to change fedora-cvs to '+', not just remove the flag. Any mistake?
(In reply to Robin Lee from comment #22) > (In reply to Jon Ciesla from comment #21) > > Git setup complete. > > Jon, it seems you used to change fedora-cvs to '+', not just remove the > flag. Any mistake? It doesn't matter because bugzilla had some problem yesterday so Jon manually added the SCM.
Package Change Request ====================== Package Name: monitorix New Branches: el6 Owners: cicku
monitorix-3.2.0-2.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/monitorix-3.2.0-2.fc19
monitorix-3.2.0-2.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/monitorix-3.2.0-2.fc18
Git done (by process-git-requests).
monitorix-3.2.0-2.fc19 has been pushed to the Fedora 19 testing repository.
monitorix-3.2.0-2.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/monitorix-3.2.0-2.el6
monitorix-3.2.0-2.fc19 has been pushed to the Fedora 19 stable repository.
monitorix-3.2.0-2.fc18 has been pushed to the Fedora 18 stable repository.
monitorix-3.2.1-1.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/monitorix-3.2.1-1.el6
monitorix-3.2.1-1.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.
Package Change Request ====================== Package Name: monitorix New Branches: epel7 Owners: cicku mikaku