Bug 513541 - (cpulimit) Review Request: cpulimit - CPU Usage Limiter for Linux
Review Request: cpulimit - CPU Usage Limiter for Linux
Status: CLOSED DUPLICATE of bug 772406
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
http://cpulimit.sourceforge.net/
:
: 513663 524006 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-24 00:46 EDT by Ashay Humane
Modified: 2012-01-11 17:03 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-21 00:13:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ashay Humane 2009-07-24 00:46:43 EDT
Spec URL: http://ashay.info/rpm/cpulimit.spec
SRPM URL: http://ashay.info/rpm/cpulimit-1.1-1.fc11.src.rpm
Description: 

cpulimit is a simple program that attempts to limit the cpu usage of a process (expressed in percentage, not in cpu time). This is useful to control batch jobs, when you don't want them to eat too much cpu. 

It does not act on the nice value or other scheduling priority stuff, but on the real cpu usage. Also, it is able to adapt itself to the overall system load, dynamically and quickly. 

More info here: http://cpulimit.sourceforge.net/

The original source did not use autotools and had a simple Makefile. I converted the source to use autotools. 

Thank you
Comment 1 Fabian Affolter 2009-07-24 03:24:44 EDT
Just some quick comments on your spec file:

You wrote in your sec file...
----%>------
#The source on sourceforge does not use autotools. Source1 does
Source0:        http://downloads.sourceforge.net/project/cpulimit/cpulimit/cpulimit/cpulimit-1.1.tar.gz 
Source1:        http://ashay.info/rpm/cpulimit-1.1.tar.gz
----<%------

Have you make a patch for the usage of autotools? If yes, send this patch upstream and apply the patch in the spec file.

https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.25patch_commands

There are more details available about 'Source0'

https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Spec_file_pieces_explained look for 'Source0'

- 'Source0' seems to be wrong, shouldn't it be 'http://downloads.sourceforge.net/cpulimit/%{name}-%{version}.tar.gz' ?

- For valid groups you can use 'less /usr/share/doc/rpm-*/GROUPS'

-  %attr (-,root,root) can be left away, it sufficient only to use %{_mandir}/*/* 

- Isn't rpmlint complaining about the line length of the %description?
Comment 2 Ashay Humane 2009-07-24 12:53:23 EDT
*** Bug 513663 has been marked as a duplicate of this bug. ***
Comment 3 Itamar Reis Peixoto 2009-07-24 13:13:02 EDT
replace these 2 lines

mkdir -p $RPM_BUILD_ROOT%{_mandir}/man1
install -m 644 man/cpulimit.1    $RPM_BUILD_ROOT%{_mandir}/man1/cpulimit.1


with
install -Dp -m (only one line do the job)
Comment 4 Itamar Reis Peixoto 2009-07-24 13:16:08 EDT
is this your first package ?

what's your fedora account (FAS) username ?
Comment 5 Fabian Affolter 2009-07-24 14:21:09 EDT
(In reply to comment #1)
> - Isn't rpmlint complaining about the line length of the %description?  

rpmlint output

[fab@laptop09 SRPMS]$ rpmlint cpulimit-1.1-1.fc11.src.rpm 
cpulimit.src: E: description-line-too-long cpulimit is a simple program that attempts to limit the cpu usage of a process (expressed in percentage, not in cpu time).
cpulimit.src: E: description-line-too-long This is useful to control batch jobs, when you don't want them to eat too much cpu.
cpulimit.src: E: description-line-too-long It does not act on the nice value or other scheduling priority stuff, but on the real cpu usage.
cpulimit.src: E: description-line-too-long Also, it is able to adapt itself to the overall system load, dynamically and quickly.
cpulimit.src: W: non-standard-group System Tools
cpulimit.src: W: invalid-license GPL
1 packages and 0 specfiles checked; 4 errors, 2 warnings.
Comment 6 Ashay Humane 2009-07-24 18:07:18 EDT
Thank you for your comments...

I have updated the spec file as suggested.
The "convert to autotools" portion is now a patch, which I also sent upstream.

rpmlint errors/warnings are fixed.

Yes, this is my first package. My FAS account name is ashayh. 

Thank you
Comment 7 Itamar Reis Peixoto 2009-07-24 18:16:22 EDT
(In reply to comment #6)
please submit the new spec and src.rpm file, remember to bump version to avoid confusion.
Comment 8 Ashay Humane 2009-07-24 18:28:34 EDT
(In reply to comment #7)
> (In reply to comment #6)
> please submit the new spec and src.rpm file, remember to bump version to avoid
> confusion.  

Here is the new srpm:

http://ashay.info/rpm/cpulimit-1.1-2.fc11.src.rpm
http://ashay.info/rpm/cpulimit.spec
Comment 9 Fabian Affolter 2009-07-24 18:55:33 EDT
Your patch add several files.  The AUTHORS, COPYING, and all other documentation files should go in %doc.

There is a rpmlint warning.  This is easy to fix ;-)

[root@laptop09 i586]# rpmlint cpulimit*
cpulimit.i586: W: incoherent-version-in-changelog 1.1 ['1.1-2.fc11', '1.1-2']
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

A blank line between the changelog entry blocks make the whole changelog easier to ready.

One suggestion: Would it make the life of upstream easier if the man page is installed within the Makefile?

The time stamps are not preserved (https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps).  Almost all files are generated with the patch so this will affect after a new upstream release.
Comment 10 Fabian Affolter 2009-07-24 18:59:07 EDT
Upstream has a patch (an empty one)

http://sourceforge.net/tracker/?group_id=174425&atid=869186
Comment 11 Ashay Humane 2009-07-24 20:36:04 EDT
Added man file in make.
Should I bump the version each and every time I fix something here? (I bumped it)

I have no idea how to fix the time stamps issue...please advise if you do.

http://ashay.info/rpm/cpulimit.spec
http://ashay.info/rpm/cpulimit-1.1-3.fc11.src.rpm
Comment 12 Fabian Affolter 2009-07-25 04:34:00 EDT
(In reply to comment #11)
> Added man file in make.

To the Makefile? Is in this case the line 'install -Dp -m 644 man/cpulimit.1 $RPM_BUILD_ROOT%{_mandir}/man1/cpulimit.1' still needed?

> Should I bump the version each and every time I fix something here? (I bumped
> it)

Everytime you make a change, bump the release.  As Itamar already said, this avoid
confusion.

> I have no idea how to fix the time stamps issue...please advise if you do.

You can use 'make install INSTDIR=$RPM_BUILD_ROOT INSTALL="install -p"' instead of 'make install DESTDIR=$RPM_BUILD_ROOT '. You can change this later when everything is upstream but often it don't harm when it's added before it takes effect.
Comment 13 Ashay Humane 2009-07-25 15:13:32 EDT
I removed the "install -Dp -m 644 man/cpulimit.1 $RPM_BUILD_ROOT%{_mandir}/man1/cpulimit.1" " line.

' make install INSTDIR=$RPM_BUILD_ROOT INSTALL="install -p" ' does not work.
( configure and make do not recognize any variable called INSTDIR, and rpmbiuld fails because of that. )

So I added "install -p" to ./configure. Will this work as expected to preserve timestamps?

http://ashay.info/rpm/cpulimit.spec
http://ashay.info/rpm/cpulimit-1.1-4.fc11.src.rpm  

Thanks
Comment 14 Fabian Affolter 2009-07-25 17:31:56 EDT
Sorry, my fault... INSTDIR should be DESTDIR.  

make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25install_section

As I already said as long the doc files and the man page aren't upstream this is only cosmetically.

(In reply to comment #13)
> So I added "install -p" to ./configure. Will this work as expected to preserve
> timestamps?

No, I was misleading you.  Can you please fix this? Then I will make a full review.

BTW, I'm not a sponsor.  You can get some hints on that process at the following page https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored . The long story short: Make some informal reviews of other packages and make some more packages to show that you understand the guidelines and are familiar with them.
Comment 15 Ashay Humane 2009-07-25 18:46:27 EDT
Thanks, I will follow the sponsorship process.

Fixed files here:

http://ashay.info/rpm/cpulimit.spec
http://ashay.info/rpm/cpulimit-1.1-5.fc11.src.rpm  

Any issues now?
Comment 16 Susi Lehtola 2009-07-29 05:16:17 EDT
Don't bother with autotools. There's nothing wrong with a package that builds without them, especially when the build process is this simple.

In fact, you don't even need to use the makefile (since it doesn't support optflags). I recommend that you drop Patch0 and

 %setup -q

 %build
 gcc $RPM_OPT_FLAGS -lrt -o cpulimit cpulimit.c

 %install
 rm -rf $RPM_BUILD_ROOT
 install -p -m 755 cpulimit $RPM_BUILD_ROOT/%{_bindir}/cpulimit

**

Besides, your Patch is *creating* the files
AUTHORS COPYING README INSTALL ChangeLog

If you want to ship these, list them as extra Source lines. Where did you get these, anyway..? The tarball only contains the source code file and the simple Makefile.

**

I am willing to sponsor you, if you show me you know the Fedora guidelines; the most important of which are
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

To do this you need to make another submission, and perform two informal reviews of packages of other people. Please review only packages not marked with the FE-NEEDSPONSOR blocker as I will have to do the full formal review after you to check that you have got everything right.

After being sponsored, you are able to do reviews of your own.
Comment 17 Susi Lehtola 2009-07-29 05:18:53 EDT
(In reply to comment #16)
>  install -p -m 755 cpulimit $RPM_BUILD_ROOT/%{_bindir}/cpulimit
.. should be, of course, install -D -p -m 755 (I thought something was missing).

Fabian: as you've already started, you can do the review on this one.

I'll throw in extra comments if necessary :)
Comment 18 Itamar Reis Peixoto 2009-07-29 08:00:06 EDT
(In reply to comment #16)
> Don't bother with autotools. There's nothing wrong with a package that builds
> without them, especially when the build process is this simple.

I agree, but my recommendation is a small patch to makefile without auto-tools.
and sending this small patch to upstream of cpulimit.
Comment 19 Susi Lehtola 2009-07-29 08:26:24 EDT
(In reply to comment #18)
> (In reply to comment #16)
> > Don't bother with autotools. There's nothing wrong with a package that builds
> > without them, especially when the build process is this simple.
> 
> I agree, but my recommendation is a small patch to makefile without auto-tools.
> and sending this small patch to upstream of cpulimit.  

Yes, that is fine too. Then the makefile should look like


 BINDIR=/usr/local/bin
 MANDIR=/usr/local/man
 INSTALL="install"
 CFLAGS="-Wall -O2"

 all::   cpulimit

 cpulimit:       cpulimit.c
         gcc ${CFLAGS} -o cpulimit cpulimit.c -lrt

 clean:
         rm -f cpulimit

 install:
         ${INSTALL} -D -m 755 cpulimit ${DESTDIR}${BINDIR}
         ${INSTALL} -D -m 644 cpulimit.1 ${DESTDIR}${MANDIR}/man1/cpulimit.1


By default this will compile with "-Wall -O2" and install to /usr/local/bin. In the spec file one would use

 %build
 make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
to override the CFLAGS variable in Makefile and

 %install
 make DESTDIR=$RPM_BUILD_ROOT BINDIR=%{_bindir} MANDIR=%{_mandir} INSTALL="install -p" install
to override the other variables. One can also do without DESTDIR support, then one just prepends $RPM_BUILD_ROOT to MANDIR and BINDIR.
Comment 20 Itamar Reis Peixoto 2009-07-29 08:38:30 EDT
(In reply to comment #19)
the auto-tools previous patch was very intrusive.

patching makefile sounds perfect for me;

you could include CC, something like this.


>  BINDIR=/usr/local/bin
>  MANDIR=/usr/local/man
>  INSTALL="install"
>  CFLAGS="-Wall -O2"
   CC="gcc"
>  all::   cpulimit
> 
>  cpulimit:       cpulimit.c
>          ${CC} ${CFLAGS} -o cpulimit cpulimit.c -lrt
>
Comment 21 Fabian Affolter 2009-07-29 19:38:34 EDT
(In reply to comment #17)
> Fabian: as you've already started, you can do the review on this one.
> 
> I'll throw in extra comments if necessary :)  

Ok, I will.  It good to know are you are watching over my shoulder ;-)
Comment 22 Ashay Humane 2009-07-29 20:14:28 EDT
Ok, so as suggested, I got rid of autotools and made a patch for the Makefile.

The man page was not included by upstream, I got that from the debian package.
If I do not include the man page, rpmlint warns about no documentation.
So I decided to include the man page as Source1. (and sent it upstream on
sf.net)

If that's not a good idea, I will remove the man page and let the warning be.

Thanks everyone for their comments.

Jussi, thanks for the sponsorship offer.
I'll make a few more packages and review some packages soon.
I intend to make/maintain packages on a regular basis.

http://ashay.info/rpm/cpulimit.spec
http://ashay.info/rpm/cpulimit-1.1-6.fc11.src.rpm
Comment 23 Susi Lehtola 2009-07-29 20:26:43 EDT
(In reply to comment #22)
> Ok, so as suggested, I got rid of autotools and made a patch for the Makefile.
> 
> The man page was not included by upstream, I got that from the debian package.
> If I do not include the man page, rpmlint warns about no documentation.
> So I decided to include the man page as Source1. (and sent it upstream on
> sf.net)
> 
> If that's not a good idea, I will remove the man page and let the warning be.

OK. Debian considers not having a man file a packaging error, so they have the tendency of generating them if they do not already exist.

In Fedora we do not do so; we try to stay as close to upstream as possible and thus if there is no documentation in the tarball we don't ship any, either. I wouldn't ship the man page (generally there's no guarantee that it isn't obsolete), but you should send it upstream to see if they could include it in the next release. Also ask them to add the relevant license file, i.e.
http://www.gnu.org/licenses/gpl-2.0.txt

Some times you don't have to care about rpmlint warnings. The no-doc warning is the most common of these cases; you just have to check if there is something that should be put there.

> Thanks everyone for their comments.
> 
> Jussi, thanks for the sponsorship offer.
> I'll make a few more packages and review some packages soon.
> I intend to make/maintain packages on a regular basis.

Good, since packaging shouldn't be a one-off activity :)

Paste the numbers of the BZ entries here so I can check them out.
Comment 24 Ashay Humane 2009-07-29 20:46:46 EDT
Ok, I removed the man page.
Here are the new files:

http://ashay.info/rpm/cpulimit.spec
http://ashay.info/rpm/cpulimit-1.1-7.fc11.src.rpm
Comment 25 Fabian Affolter 2009-08-02 09:23:58 EDT
After the rebuild of the source RPM rpmlint says:

[fab@laptop09 i586]$ rpmlint cpulimit*
cpulimit.i586: W: no-documentation
cpulimit-debuginfo.i586: E: debuginfo-without-sources
2 packages and 0 specfiles checked; 1 errors, 1 warnings.

For it seems that the compiler flags aren't honored.  Comment #19 said what to do ;-)
Comment 26 Susi Lehtola 2009-08-02 09:53:43 EDT
(In reply to comment #25)
> After the rebuild of the source RPM rpmlint says:
> 
> [fab@laptop09 i586]$ rpmlint cpulimit*
> cpulimit.i586: W: no-documentation
> cpulimit-debuginfo.i586: E: debuginfo-without-sources
> 2 packages and 0 specfiles checked; 1 errors, 1 warnings.
> 
> For it seems that the compiler flags aren't honored.  Comment #19 said what to
> do ;-)  

i.e. you need
 make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
instead of
 make %{?_smp_mflags}
Comment 28 Susi Lehtola 2009-08-06 07:47:17 EDT
Have you made any other submissions of performed informal reviews?
Comment 29 Fabian Affolter 2009-09-17 14:05:46 EDT
*** Bug 524006 has been marked as a duplicate of this bug. ***
Comment 30 Christopher X.S. Zee 2009-09-17 20:51:16 EDT
Is this review request still ongoing?
Comment 31 Fabian Affolter 2009-09-29 03:44:55 EDT
Ashay, did you make some informal reviews or other packages?
Comment 32 Fabian Affolter 2009-10-23 09:58:40 EDT
ping?
Comment 33 Ashay Humane 2009-10-23 11:32:36 EDT
Hi Fabian

Sorry for the delay.
I've been on vacation for a long time with no practical access to computers/internet.

I've done one informal review so far.
And created another package (adtool)

I'll be back in a few weeks and continue...
Comment 34 Fabian Affolter 2010-01-04 05:09:04 EST
Ashay, is there any progress?  There was another review request by Xia submitted #524006, if you have no time it would be an option to close this review and go with the one of Xia.  What do you think?
Comment 35 Ashay Humane 2010-01-04 12:34:20 EST
Please close this and go ahead with Xias review.
Thanks
Comment 36 Jason Tibbitts 2010-01-21 00:13:03 EST
Closing as requested.
Comment 37 Thomas Spura 2012-01-11 17:02:41 EST

*** This bug has been marked as a duplicate of bug 772406 ***

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