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
Clean [mindruv@localhost rpmbuild]$ rpmlint SPECS/sartgraph.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. [mindruv@localhost rpmbuild]$
Created attachment 782068 [details] screenshot a screen-shot
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.
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
> 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
renewed SPEC file. https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec
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. :(
updated SPEC file https://sourceforge.net/p/sargraph/code/ci/master/tree/sartgraph.spec Will submit a new Review request for Text::BarGraph
New review request submitted for Text::BarGraph https://bugzilla.redhat.com/show_bug.cgi?id=993636
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.