Bug 991531 - (sartgraph) Review Request: sartgraph - draw ASCII bargraph of sar stats
Review Request: sartgraph - draw ASCII bargraph of sar stats
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On: perl-Text-BarGraph
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-08-02 12:37 EDT by Veaceslav Mindru
Modified: 2015-01-26 11:38 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-07-23 11:34:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review-


Attachments (Terms of Use)
screenshot (185.17 KB, image/jpeg)
2013-08-02 12:40 EDT, Veaceslav Mindru
no flags Details

  None (edit)
Description Veaceslav Mindru 2013-08-02 12:37:33 EDT
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 12:38:29 EDT
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 12:40:02 EDT
Created attachment 782068 [details]
screenshot

a screen-shot
Comment 3 Joshua Small 2013-08-03 06:07:09 EDT
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 06:52:15 EDT
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 05:38:55 EDT
> 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 04:33:14 EDT
renewed SPEC file.

https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec
Comment 7 Rex Dieter 2013-08-05 10:38:46 EDT
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 10:56:10 EDT
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 06:46:20 EDT
New review request submitted for Text::BarGraph  https://bugzilla.redhat.com/show_bug.cgi?id=993636
Comment 10 Rex Dieter 2014-07-23 11:34:23 EDT
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.