Bug 421871 - Review Request: libvirt-cim - A CIM provider for libvirt
Review Request: libvirt-cim - A CIM provider for libvirt
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matt Domsch
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-12 11:23 EST by Dan Smith
Modified: 2009-10-06 14:25 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-08 11:17:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
matt_domsch: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dan Smith 2007-12-12 11:23:53 EST
Spec URL: http://static.danplanet.com/pkg/fedora-9/libvirt-cim.spec
SRPM URL: http://static.danplanet.com/pkg/fedora-9/libvirt-cim-0.1-1.fc9.src.rpm
Description: libvirt-cim is a CIM provider for virtualization platforms supported by libvirt.  It enables remote enterprise-class management of virtual machines.

Note: We have not made a formal code release yet.  We will do so soon and will update the spec with a URL at that point.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=289649
Comment 1 Matt Domsch 2008-01-06 00:42:06 EST
Release: what's extra_release there for?

BuildRoot: value acceptable, but two better choices are available.

Source: misssing full URL to the file.

Why explicit Requires for libxml2 and libvirt versions?  README lists
libvirt-0.2.3, while spec Requires libvirt >= 0.3.2.  typo? somewhere?

%pre needs a comment explaining what you're trying to do.
 It looks like the provider-register call in %pre is trying to
 unregister (e.g. -d is delete).  It took me a while to figure it out,
 because you are invoking scripts that are part of its own (not yet
 installed) package (which fails on first install, so you hit the
 ||true clause).

%post: don't automatically restart the service, use cond-restart.  You
 don't want it started on initial install from kickstart, for example.

$ rpmlint *.rpm
libvirt-cim.src: W: strange-permission libvirt-cim.spec 0600
otherwise file permissions look ok, no other lint warnings.


rpaths used, which need to be removed.  Drop files in
/etc/ld.so.conf.d/ adding /usr/lib*/cmpi/.

Remove *.so files.


Review criteria:
rpmlint attached, trivial spec permission fix at checkin time OK
package name OK
spec file name OK
with changes above, should meet packaging guidelines FIX+RECHECK
license LGPLv2.1+ OK
license field matches OK
license included in %doc OK
spec is English OK
spec mostly legible, modulo comments above. OK
can't presently judge source matches upstream, until formal upstream
  tarball is released.   FIX+RECHECK
package compiles on at least x86_64 F9(rawhide) in mock. OK
spec doesn't use locales OK
ldconfig called appropriately OK
package not relocatable OK 
package owns its directories OK
no duplicate files OK
defaddr line present, permissions sane OK
%clean OK
macros OK
contains code OK
no large docs
no header files OK
no static libs OK
no pkgconfig files OK
.so files present but not in a -devel package, delete
  them. FIX+RECHECK
no .la files OK
no GUI files OK
directory ownership OK
%install cleans first OK
all filesname UTF-8 OK

license included OK
translated summary & description, not present. SHOULD, but OK
package builds in mock on x86_64. OK
package not tested.
scriptlets sane, if commented.  FIX+RECHECK
no subpackages OK
no .pc files OK
no file deps OK


Please fix and repost.
Comment 2 Dan Smith 2008-01-07 13:02:23 EST
- Removed extra_release
- Source URL will be updated when the upstream release
  is made
- Removed explicit Requires
- Commented %pre.  Following convention here.
- Changed to cond-restart
- chrpath reports no rpath set on my .so's.  CIM
  providers *are* .so files and must remain as such.
  See sblim-cmpi-base (et al) for other examples.  These
  are not development libraries, they are plugin-type
  libraries.

SPEC: http://static.danplanet.com/pkg/fedora-9/libvirt-cim.spec
SRPM: http://static.danplanet.com/pkg/fedora-9/libvirt-cim-0.1-2.fc9.src.rpm

Thanks!
Comment 3 Matt Domsch 2008-01-07 13:56:21 EST
Thanks for the quick turn.

Building on x86_64, I do see RPATHs being used.

$ find . -name \*.so | xargs -n 1 chrpath -l | grep RPATH
./usr/lib64/cmpi/libVirt_HostedDependency.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_AllocationCapabilities.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_VSMigrationService.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_VirtualSystemManagementService.so: 
RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ComputerSystemIndication.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ElementCapabilities.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ResourcePoolConfigurationService.so: 
RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_SettingsDefineState.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ElementConformsToProfile.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_VSSDComponent.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_SystemDevice.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_HostedService.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_HostedResourcePool.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_VirtualSystemManagementCapabilities.so: 
RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_SettingsDefineCapabilities.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ElementAllocatedFromPool.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ResourceAllocationFromPool.so: RPATH=/usr/lib64/cmpi
./usr/lib64/cmpi/libVirt_ElementSettingData.so: RPATH=/usr/lib64/cmpi

other trivial things:

rpmlint warning: libvirt-cim.x86_64: W: percent-in-%pre.
This comes from %pre's comment header mentioning %post.  Just drop the %.

Be sure spec file permissions are 644 when you cvs checkin to get rid of the 
other rpmlint warning.

A comment about .so files being plugins would be helpful.
Comment 4 Dan Smith 2008-01-07 18:08:21 EST
Hmm, RPATH is only on some of them.  How weird.  I added the code to "fix"
libtool, and now they're gone.

Fixed the comment issue (that was dumb of me).

For reference, the fixed version:
SRPM: http://static.danplanet.com/pkg/fedora-9/libvirt-cim-0.1-3.fc9.src.rpm
SPEC: http://static.danplanet.com/pkg/fedora-9/libvirt-cim.spec

Thanks!
Comment 5 Dan Smith 2008-01-07 18:09:17 EST
New Package CVS Request
=======================
Package Name: libvirt-cim
Short Description: A CIM provider for libvirt
Owners: danms
Branches: F-9
InitialCC: 
Cvsextras Commits: No
Comment 6 Matt Domsch 2008-01-07 18:19:59 EST
great, thanks. APPROVED.
Comment 7 Kevin Fenzi 2008-01-07 22:27:39 EST
cvs done. 

Any particular reason to disallow cvsextras commits? 
Comment 8 Kaitlin Rupert 2008-10-13 19:54:46 EDT
Package Change Request
======================
Package Name: libvirt-cim
New Branches: dist-f10
Owners: danms

Add Fedora 10 branch.
Comment 9 Kevin Fenzi 2008-10-15 18:04:10 EDT
cvs done.
Comment 10 Kaitlin Rupert 2009-04-21 21:14:58 EDT
Package Change Request
======================
Package Name: libvirt-cim
New Branches: dist-f11
Owners: kaitlin, danms

Add Fedora 11 branch.
Comment 11 Kaitlin Rupert 2009-10-06 14:18:44 EDT
Package Change Request
======================
Package Name: libvirt-cim
Owners: danms kailtin

danms is the current owner and I am a co-maintainer.  Dan has not been maintaining the project for the past 7 months.  Would like to switch ownership over to Kaitlin and remove Dan as a co-maintainer.

Thanks!
Comment 12 Dan Smith 2009-10-06 14:21:54 EDT
Seconded :)
Comment 13 Kevin Fenzi 2009-10-06 14:25:23 EDT
You guys can do this yourself. ;) 

Dan, go to: 
https://admin.fedoraproject.org/pkgdb/packages/name/libvirt-cim

and orphan the package, and Kaitlin can take full ownership.

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