Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 991531 (sartgraph) - Review Request: sartgraph - draw ASCII bargraph of sar stats
Summary: Review Request: sartgraph - draw ASCII bargraph of sar stats
Keywords:
Status: CLOSED NOTABUG
Alias: sartgraph
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: perl-Text-BarGraph
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2013-08-02 16:37 UTC by Veaceslav Mindru
Modified: 2015-01-26 16:38 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-07-23 15:34:23 UTC
rdieter: fedora-review-


Attachments (Terms of Use)
screenshot (deleted)
2013-08-02 16:40 UTC, Veaceslav Mindru
no flags Details

Description Veaceslav Mindru 2013-08-02 16:37:33 UTC
Spec URL: https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec
SRPM URL: https://downloads.sourceforge.net/project/sargraph/EPEL6/sartgraph-0.2-2.fc18.src.rpm?r=&ts=1375461316&use_mirror=master  
Description: use available sar stats and draw ASCII bargraph directly in terminal 
Fedora Account System Username: mindruv

Comment 1 Veaceslav Mindru 2013-08-02 16:38:29 UTC
Clean [mindruv@localhost rpmbuild]$ rpmlint SPECS/sartgraph.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[mindruv@localhost rpmbuild]$

Comment 2 Veaceslav Mindru 2013-08-02 16:40:02 UTC
Created attachment 782068 [details]
screenshot

a screen-shot

Comment 3 Joshua Small 2013-08-03 10:07:09 UTC
I am not a sponsor but would like to offer an informal review.

URLs: Both of these URLs given linked to website related to the SPEC and SRPM, however, nothing we would easily use a wget on. It would assist reviewers if a direct link could be given.

rpmlint: For maximum effect, this should be run against your spec file, the SRPM and RPM files. Ignoring spelling warnings, it presents these complaints:

sartgraph.src: W: no-version-in-last-changelog
sartgraph.noarch: W: no-version-in-last-changelog
sartgraph.noarch: W: no-manual-page-for-binary sartgraph


I believe updating your spec file with a version number as per the guidelines document would solve two of these complaints:

https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

It appears that the only documentation is in the form of the license, and you probably need at least a basic document added as a %doc. I appreciate that as a Perl script you pretty much "just run it", but you should say so.

I've run a koji build for you, which you can see here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5712574

[fedora@ip-172-31-20-108 noarch]$ rpmls sartgraph-0.2-2.fc19.noarch.rpm
-rwxr-xr-x  /usr/bin/sartgraph
drwxr-xr-x  /usr/share/doc/sartgraph-0.2
-rw-r--r--  /usr/share/doc/sartgraph-0.2/LICENSE

Permissions look correct. There are no libraries or special files in this package and thus it avoids many pitfalls by default.

Comment 4 Veaceslav Mindru 2013-08-03 10:52:15 UTC
Hello,

I was going to write a man page and a Readme file. This perl script has a help message when run with -h no/or wrong flags.

Thank you for your comments Josh :) 

VM

Comment 5 Michael Schwendt 2013-08-04 09:38:55 UTC
> Requires: sysstat,perl(Text::BarGraph)  

If you specified such requirements on separate lines

  Requires: sysstat
  Requires: perl(Text::BarGraph)  

you could add comments more easily. Interesting here would be to comment on what exactly from "sysstat" is needed. That's just a package name, and that package contains multiple tools. If the Perl script executes individual tools from sysstat, it would be a sign of packaging quality to mention that. And if you fear that an executable might move from one package to another, you could depend on its full path even (with no metadata lookup penalty, because file paths are stored in primary metadata).


> Requires: perl(Text::BarGraph)  

When it's strictly needed at run-time, it's clever to also require it at build-time as some sort of existance-check:

  BuildRequires: perl(Text::BarGraph)  


> %build
> %prep
> %setup -q

It's okay to have an empty %build section. It would even be okay to have no such section at all (if rpmlint didn't warn about it). Better yet is to keep these sections sorted in the order they are executed during build, i.e. %prep -> %build -> %install [-> %check]


> mkdir -p %{buildroot}%{_bindir}
> chmod a+x %{name}
> cp -p %{name}  %{buildroot}%{_bindir}

From the Hints Department, the "install" tool is very popular among packagers, because it can replace those three mkdir/chmod/cp lines:

  install -D -p -m0755 %{name} %{buildroot}%{_bindir}/%{name}


> %changelog

Not so important during review, but practising %changelog entry maintenance would be good:
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Comment 6 Veaceslav Mindru 2013-08-05 08:33:14 UTC
renewed SPEC file.

https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec

Comment 7 Rex Dieter 2013-08-05 14:38:46 UTC
As mentioned on irc, the latest sourceforce .spec link still:
* puts Requires on one line
* uses 'cp' instead of 'install' in %install section

I think the manpage name is not right either, I think it needs to have a .1 or .1.gz suffix.

hrm, on my f19 box, 
$ sudo yum install 'perl(Text::BarGraph)'
...
No package perl(Text::BarGraph) available.

Looks like we have a missing dependency too. :(

Comment 8 Veaceslav Mindru 2013-08-05 14:56:10 UTC
updated SPEC file https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec 

Will submit a new Review request for Text::BarGraph

Comment 9 Veaceslav Mindru 2013-08-06 10:46:20 UTC
New review request submitted for Text::BarGraph  https://bugzilla.redhat.com/show_bug.cgi?id=993636

Comment 10 Rex Dieter 2014-07-23 15:34:23 UTC
closing per stalled review process,
http://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

Veaceslav, if you're ever interested in picking this back up, please do let us know, and I'll be happy to help move things along again.


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