[Bug 991531] Review Request: sartgraph - draw ASCII bargraph of sar stats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=991531



--- Comment #5 from Michael Schwendt <bugs.michael@xxxxxxx> ---
> 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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=WGNGrAFcZK&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]