Bug 448491 - Review Request: libcgroup - A collection of tools and libraries to control and monitor control groups the associated controllers
Review Request: libcgroup - A collection of tools and libraries to control a...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-27 05:27 EDT by Dhaval Giani
Modified: 2008-07-30 16:09 EDT (History)
6 users (show)

See Also:
Fixed In Version: 0.1c-2.fc9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-30 16:09:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dhaval Giani 2008-05-27 05:27:59 EDT
Spec URL: http://jaist.dl.sourceforge.net/sourceforge/libcg/libcgroup.spec
SRPM URL: http://nchc.dl.sourceforge.net/sourceforge/libcg/cgroup-0.1b-1.fc8.src.rpm
Description:
Control groups infrastructure. The tools and library help manipulate, control,
administrate and monitor control groups and the associated controllers.

cgroup-lib
The libcgroup package contains the libraries to run control group
applications.

cgroup-lib-devel
It provides API to create/delete and modify cgroup nodes. It will also in the
future allow creation of persistent configuration for control groups and provide
scripts to manage that configuration.
Comment 1 Dhaval Giani 2008-05-27 05:37:49 EDT
This is my first package to the Fedora and I need a sponsor
Comment 2 Bill Nottingham 2008-05-27 10:19:09 EDT
Not a formal review, but the spec file name should match package name/upstream
name, as a start.
Comment 3 Dhaval Giani 2008-05-27 10:30:00 EDT
The upstream name is libcgroup itself.
Comment 4 Bill Nottingham 2008-05-27 10:36:33 EDT
That's not what the upstream tarball is named. If it is named libcgroup, then
the package names should match it as well (not cgroup-lib)
Comment 5 Dhaval Giani 2008-05-27 13:53:47 EDT
OK,I updated the filename.

spec file is now available at
http://nchc.dl.sourceforge.net/sourceforge/libcg/cgroup.spec
Comment 6 Dan Smith 2008-05-27 13:56:26 EDT
The name should be libcgroup, not cgroup.
Comment 7 Tom "spot" Callaway 2008-05-27 15:06:11 EDT
The point is this: The SRPM name should match the SPEC name and the generated
binary name.

So, if the name is libcgroup, then you should have:

libcgroup-0.1b-1.fc8.src.rpm
libcgroup.spec

and when built,
libcgroup-0.1b-1.fc8.x86_64.rpm
Comment 8 Balbir Singh 2008-05-27 22:17:56 EDT
Dhaval has renamed the spec to cgroup.spec and the RPMS built are

cgroup-lib-0.1b-*.rpm and cgroup-lib-devel-0.1b-*rpm

We decided to drop the lib prefix from our package since we in the long run
intend to provide initscripts and potentially a daemon for configuraiton.

Am I missing something?
Comment 9 Dan Smith 2008-05-28 09:50:53 EDT
Library packages are conventionally named "libfoo", with a "libfoo-devel"
package for the headers.  "foo-lib" is not conventional and I can't think of any
reason why you would want to break tradition here.  Your SF.net name is libcg,
your file release is libcg, but the rest of your files are different.  Now is
the time to settle on a single consistent name for all of it, and libcgroup
would be my suggestion.

Other library packages include daemons, scripts, etc in their "libfoo" package.
 Look at libvirt as a prime (and related) example.
Comment 10 Balbir Singh 2008-05-28 10:54:43 EDT
I don't intend to start a flame war here, but I did my changes based on the RPMS
I saw in F9

My F9 system shows

bluez-libs-3.30-1.fc9.i386
compat-libstdc++-296-2.96-140.i386
paps-libs-0.6.8-5.fc9.i386
nfs-utils-lib-1.1.1-3.fc9.i386
PackageKit-libs-0.1.12-10.20080505.fc9.i386
kaffeine-libs-0.8.6-4.fc9.i386
lame-libs-3.97-6.lvn8.i386
cdparanoia-libs-alpha9.8-30.i386
kdegraphics-libs-4.0.3-3.fc9.i386
and many more

Having said that, I'll go by what the majority users of cgroups would like the
library to be named as.
Comment 11 Dan Smith 2008-05-28 11:02:07 EDT
Those packages are libraries specific to a parent package, not ones meant for
general development consumption, AFAICT.

However, for comparison:

% rpm -qa | grep '^lib' -c     
235
Comment 12 Balbir Singh 2008-05-29 15:28:13 EDT
I've done the renaming of files. Run rpmlint (I had done that before as well) on
the generated RPMS. Please do review the following files. 

Spec file is at

http://downloads.sourceforge.net/libcg/libcgroup.spec?use_mirror=osdn

Source file is at

http://downloads.sourceforge.net/libcg/libcgroup-0.1b-1.fc9.src.rpm?use_mirror=osdn

We would really appreciate early feedback, so that we can fix the problems (if
any) and revert back with a good clean package. Thanks for your patience and help.
Comment 13 Dan Smith 2008-05-30 12:33:03 EDT
Scratch build passed:

  http://koji.fedoraproject.org/koji/taskinfo?taskID=636762
Comment 14 Tom "spot" Callaway 2008-05-30 12:45:30 EDT
One minor issue:

Please don't use %makeinstall, use make DESTDIR=$RPM_BUILD_ROOT install

See:
http://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used
Comment 15 Balbir Singh 2008-05-30 14:51:44 EDT
Dan, thanks for the build results

Tom, I've updated the spec file at the same location as mentioned earlier. In
case you see an older version of the spec file (still), sf.net does not update
all its mirrors immediately and hence the older file might still be around as
stale data.

I've replace %makeinstall with make DESTDIR=$RPM_BUILD_ROOT install as suggested.

The file sizes are
-rw-r--r-- 1 balbir balbir 1638 2008-05-30 23:29 libcgroup.spec
-rw-r--r-- 1 balbir balbir 64201 2008-05-30 23:30 libcgroup-0.1b-1.fc9.src.rpm


Comment 16 Tom "spot" Callaway 2008-05-30 16:28:23 EDT
In general, any time you make a change to the spec file, you need to increment
the release, and add a new entry to the %changelog section. I'll ignore that for
now, but please be sure to do it in the future.

In the spec, you've got:

URL: http://downloads.sourceforge.net/libcg/%{name}-%{version}.tar.bz2
Source0: libcgroup-0.1b.tar.bz2

URL is intended to point to the libcgroup website.
Source0 should be the URL path to download the tarball.

So, you need to change it to:

URL: http://libcg.sourceforge.net/
Source0: http://downloads.sourceforge.net/libcg/%{name}-%{version}.tar.bz2

Please make a new SRPM, and I'll finish this review.
Comment 17 Balbir Singh 2008-05-30 17:50:17 EDT
Thanks for your review comments and patience Tom.

I was aware of editing the changelog, but I thought it was for software changes
rather then the spec file itself. Sorry about missing that part. What I've done
this time is updated Source0 and URL as per your suggestion (in fact just copied
them over)

I've updated the release to 2 and updated the changelog as well

The spec file is at

http://downloads.sourceforge.net/libcg/libcgroup.spec?use_mirror=osdn
(it is 1771 bytes in size)

The source RPM is at
http://downloads.sourceforge.net/libcg/libcgroup-0.1b-2.fc9.src.rpm?use_mirror=osdn
(it's 64333 bytes in size)

I hope the new files will get sync'ed across mirrors soon

Hopefully, I've got it right this time :-)
Comment 18 Tom "spot" Callaway 2008-06-02 17:37:56 EDT
OK, two more issues (sorry I didn't catch them last time).

1. You need to make sure that the devel package Requires the main package
explicitly. Currently, you have:

Requires: libcgroup >= 0.1b

This should be :

Requires: libcgroup = %{version}-%{release}

2. You need to have %post and %postun sections which call ldconfig. This way,
the shared library will get added to the system cache and be immediately
available. Just add these lines below your %clean section

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

I'll go ahead and do the rest of the review, but please make a -3 package and
upload it.

REVIEW
=======
Good:

- rpmlint checks return: Nothing
- package meets naming guidelines
- package meets packaging guidelines
- license (LGPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (5eebc14487c4e7c05808d60b86151d9881828eb3)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR (well, coreutils is questionable, but harmless)
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- no .la files
X post/postun ldconfig missing
X missing proper devel requires of base n-v-r
Comment 19 Dhaval Giani 2008-06-03 01:14:03 EDT
thanks for the comments. I have uploaded the changed specfile and src RPM

http://downloads.sourceforge.net/libcg/libcgroup.spec?use_mirror=osdn
(1971 bytes)
http://downloads.sourceforge.net/libcg/libcgroup-0.1b-3.fc8.src.rpm?use_mirror=osdn
(64646 bytes)
Comment 20 Tom "spot" Callaway 2008-06-03 14:32:33 EDT
Looks good, package is approved. Thanks for your patience here, you guys did a
wonderful job of resolving the issues. I will sponsor you, Dhaval.

You need to continue in the process here:

https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account
Comment 21 Balbir Singh 2008-06-03 15:29:13 EDT
Thanks! That's great news Tom.

Could you sponsor me as well, I intend to co-maintain this package with Dhaval.
Comment 22 Dhaval Giani 2008-06-03 15:53:17 EDT
New Package CVS Request
=======================
Package Name: libcgroup
Short Description: A collection of tools and libraries to control and monitor
control groups the associated controllers
Owners: dhavalgiani, balbirsingh
Branches: F-9, devel
InitialCC: spot
Cvsextras Commits: yes
Comment 23 Dhaval Giani 2008-06-03 16:52:08 EDT
New Package CVS Request
=======================
Package Name: libcgroup
Short Description: A collection of tools and libraries to control and monitor
control groups the associated controllers
Owners: dhavalgiani, balbirsingh
Branches: F-9 devel
InitialCC: spot
Cvsextras Commits: yes
Comment 24 Dhaval Giani 2008-06-03 17:21:28 EDT
Balbir still does not have cvsextras membership, so removing his name from
owners for now.

New Package CVS Request
=======================
Package Name: libcgroup
Short Description: A collection of tools and libraries to control and monitor
control groups the associated controllers
Owners: dhavalgiani
Branches: F-9 devel
InitialCC:
Cvsextras Commits: yes

Comment 25 Kevin Fenzi 2008-06-04 12:23:03 EDT
cvs done.
Comment 26 Fedora Update System 2008-06-11 05:13:44 EDT
libcgroup-0.1c-2.fc9 has been submitted as an update for Fedora 9
Comment 27 Fedora Update System 2008-06-11 19:35:52 EDT
libcgroup-0.1c-2.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libcgroup'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-5252
Comment 28 Tom "spot" Callaway 2008-07-02 17:53:01 EDT
In rawhide, closing this one out.
Comment 29 Fedora Update System 2008-07-30 16:09:37 EDT
libcgroup-0.1c-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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