Spec URL: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage.spec SRPM URL: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage-2.5-3.el5.src.rpm Description: SysUsage is a tool used to continuously monitor a system and generate a daily/weekly/monthly/yearly graphical report using rrdtool and sar.
there has been some discussion as to whether or not this package should instead be named perl-SysUsage-Sar. if perl-SysUsage-Sar is a preferred or more correct package name, let me know and i'll be happy to respin it that way.
It seems to me that this is more of an application that happens to have a Perl library rather than a Perl library that happens to include an executable, so I don't think it needs to be named like a Perl module. What matters is the upstream name of the project, which seems to be SysUsage. It's perfectly OK to downcase that, so "sysusage" is fine. I note that there's a 2.6 out now; would you like to update? It would be best to do any fixups like removing the hardcoded /usr/local/sysusage bits in %prep, I think. %install shouldn't be modifying things. There's no need for the explicit Requires: perl; rpm will figure it out. But it doesn't hurt to have it there if you don't want to remove it. Otherwise this package looks good to me.
> It would be best to do any fixups like removing the hardcoded > /usr/local/sysusage bits in %prep, I think. %install shouldn't be modifying > things. You should probably be aware of what limitations exist in your own products before making suggestions: https://bugzilla.redhat.com/show_bug.cgi?id=232602
Pardon me? "my own products"? Sorry, I'm just a volunteer here, and the fact that the kernel package does something has no bearing on this review. So I find the above comment completely bewildering and frankly completely inappropriate.
A patch in %prep will create a different source rpm as compared to a patch in %install if you do rpmbuild -bs (vs. -ba). That is the bug I'm referencing, and given that the version of RPM hasn't changed in a Red Hat or Fedora distro in quite some time, it will still affect any and every package. Read the bug again, it shouldn't be as bewildering as you've made it out to be, even if it is assigned to the kernel.
rob: I'm sorry that this known troll has decided to disrupt your review ticket. I can only urge you to ignore him, since he apparently has his own axe to grind and is not interested in helping you or this package. If you prefer to mail me privately to continue to work on this package and avoid this troll, please feel free to do so. jason.corely: Unpacking the source tarball in %prep and then modifying the files therein with sed is no different from modifying them with %patch. Both are allowed, and often using sed is simpler. None of this has anything to do with whatever it seems you're spouting off about. Please let us get on with the business of reviewing this package.
Spec: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage.spec SRPM: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage-2.6-1.el5.src.rpm * Thu Nov 8 2007 Rob Myers <rob.myers.edu> 0:2.6-1 - move seds to build section - remove perl requires - update to 2.6
Let's take a step back for a moment, as I realized that my wording is misleading at best and I have perhaps read a bit much into your original suggestion. If you are telling Rob to do the following: sed -i -e '/foo/bar/g' $RPM_BUILD_DIR/some_file then you are 100% correct, that is exactly the same as a %patch directive. However if you are telling him to do the following: sed -i -e /foo/bar/g' $RPM_SOURCE_DIR/some_file then you would be introducing a bug where none existed previously (and indeed this is what you'd have to do for %SOURCE1 as it is not part of the tarball). Before you say how unlikely that situation is, please re-read the bug report I pointed you towards, where exactly that ill-advised task was being done for the RHEL5 kernel. jason.tibbbbets: I find it comical that you would immediately jump to name calling someone you don't know on the exact same day as Max Spevack's call for civility. Granted his email specifically targeted towards IRC, but you're reading comprehension skills are already in question. If you had looked at the spec in any detail you would have noticed my name in the changelog, and logical inference might have led one to believe that my commenting on this package is not to troll but because I wrote it and thus have a vested interest in seeing it not screwed up by rigid adherence to dogma for nothing more than the sake of aesthetics. Moving the sed from %install to %prep has absolutely no bearing on the final result. Rob has done a personal favor for me by submitting this package, one which I appreciate (I will not sign a CLA, in case you are wondering). Since you see fit to label me a troll when I did not call you any names I now feel happy to oblige. What gets your blood boiling, insulting your nation of origin, family history, genitalia size, significant other? I can certainly call into question your education as logical thinking and reading skills are clearly devoid in whatever degree you've managed to get from that mail-in program.
jason.corley: 1. can you test the 2.6 package and make sure it works for you? 2. let's focus on getting a good package into fedora rather than a pissing contest. :) Jason Tibbitts: i agree that the seds probably do not belong in the install section. i'm starting to think they don't belong in build either- i should have gone with prep as suggested. :) Spec: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage.spec SRPM: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage-2.6-2.el5.src.rpm * Fri Nov 9 2007 Rob Myers <rob.myers.edu> 0:2.6-2 - seds really belong in prep
Found some time to get back to this. There are a few issues: rpmlint says: sysusage.noarch: W: invalid-license Artistic sysusage.noarch: W: invalid-license GPL This package has the usual "same terms as Perl" clause, so the License: tag should contain "GPL+ or Artistic" (http://fedoraproject.org/wiki/Licensing). This package assumes /var/www exists but doesn't require anything which would provide it. My understanding is that this isn't really a webapp, just a script which produces some web-visible output, but you still need to require httpd if you're going to assume /var/www is there. And that would mean that by default this package makes its output visible to the world, which isn't generally a good thing. A .htaccess or /etc/httpd/conf.d/sysusage.conf which restricts access to localhost is necessary in this case. It would be helpful to include some Fedora-specific documentation on how to actually make this package do something. I suppose it needs a crontab, and it would be nice to provide one although it mustn't do anything by default. The cacti package puts a sample crontab in /etc/cron.d/cacti which is commented out, many packages include a README.Fedora file with instructions on getting them to run and opening up any default access restrictions. Review checklist: * source files match upstream: 0486abce90c072bb2cd66abf999f08ed662e55be2f1d53e098f04ff9cf6b5de4 SysUsage-Sar-2.6.tar.gz * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * summary is OK. * description is OK. * dist tag is present. * build root is OK. X license field needs a tweak. * license is open source-compatible. * license text not included upstream. * latest version is being packaged. * BuildRequires are proper (none) * %clean is present. * package builds in mock (rawhide, x86_64). * package installs properly X rpmlint has a valid complaint (license tag) X final provides and requires: config(sysusage) = 0:2.6-2.fc9 perl(SysUsage::Sar) = 2.6 sysusage = 0:2.6-2.fc9 = /usr/bin/perl config(sysusage) = 0:2.6-2.fc9 perl(Exporter) perl(File::Find) perl(FileHandle) perl(Getopt::Long) perl(Net::SMTP) perl(POSIX) perl(RRDs) perl(SysUsage::Sar) perl(strict) perl(vars) rrdtool sysstat (needs httpd for /var/www) * %check is not present; manual testing shows that things at least seem to run and produce proper output. X does not own /var/www/html, and depends on nothing which owns it. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. * %docs are not necessary for the proper functioning of the package.
(In reply to comment #10) > Found some time to get back to this. There are a few issues: thanks for your thorough review and suggestions! hopefully the new revision below addresses each of them. Spec: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage.spec SRPM: http://www.stl.gtri.gatech.edu/rmyers/sysusage/sysusage-2.6-3.el5.src.rpm * Thu Nov 15 2007 Rob Myers <rob.myers.edu> 0:2.6-3 - fix minor license issue - add a default crontab entry - add a default httpd configuration - add README.Fedora
Looks much better now. License: field is good, rpmlint is clean, httpd is a dependency so that /var/www is there, a sample crontab and fedora-specific documentation is in place, and access is closed by default. So no more complaints from me. APPROVED
New Package CVS Request ======================= Package Name: sysusage Short Description: System monitoring based on perl, rrdtool, and sysstat Owners: rmyers Branches: devel EL-4 EL-5
cvs done.
Package Change Request ====================== Package Name: sysusage New Branches: el6 Owners: cicku
There already seems to be a el6 branch, so you should be all set.