Bug 248395 - Review Request: sysusage - System monitoring based on perl, rrdtool, and sysstat
Review Request: sysusage - System monitoring based on perl, rrdtool, and sysstat
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-16 13:02 EDT by rob
Modified: 2013-08-18 15:08 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-19 08:37:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description rob 2007-07-16 13:02:32 EDT
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.
Comment 1 rob 2007-07-16 13:07:19 EDT
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.
Comment 2 Jason Tibbitts 2007-11-07 22:27:03 EST
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.
Comment 3 Jason Corley 2007-11-08 10:51:39 EST
> 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
Comment 4 Jason Tibbitts 2007-11-08 11:05:22 EST
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.
Comment 5 Jason Corley 2007-11-08 13:39:07 EST
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.
Comment 6 Jason Tibbitts 2007-11-08 14:11:49 EST
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.
Comment 7 rob 2007-11-08 17:21:19 EST
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@gtri.gatech.edu> 0:2.6-1
- move seds to build section
- remove perl requires
- update to 2.6
Comment 8 Jason Corley 2007-11-09 10:42:46 EST
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.
Comment 9 rob 2007-11-09 11:02:09 EST
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@gtri.gatech.edu> 0:2.6-2
- seds really belong in prep

Comment 10 Jason Tibbitts 2007-11-14 15:13:24 EST
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.
Comment 11 rob 2007-11-15 09:34:35 EST
(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@gtri.gatech.edu> 0:2.6-3
- fix minor license issue
- add a default crontab entry
- add a default httpd configuration
- add README.Fedora
Comment 12 Jason Tibbitts 2007-11-16 11:36:58 EST
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
Comment 13 rob 2007-11-16 13:58:40 EST
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
Comment 14 Kevin Fenzi 2007-11-16 23:24:49 EST
cvs done. 
Comment 15 Christopher Meng 2013-08-18 07:52:55 EDT
Package Change Request
======================
Package Name: sysusage
New Branches: el6
Owners: cicku
Comment 16 Kevin Fenzi 2013-08-18 15:08:18 EDT
There already seems to be a el6 branch, so you should be all set.

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