[Bug 772608] Review Request: ovirt-guest-agent - oVirt Guest Agent

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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

Jorge A Gallegos <kad at blegh.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kad at blegh.net

--- Comment #11 from Jorge A Gallegos <kad at blegh.net> 2012-02-01 03:19:25 EST ---
** I AM NOT A SPONSOR **

However, I'll try and submit a review as per Steven's request.

First, I can see a glaring issue with your package, you are shipping GDM's
.src.rpm as part of your own source. That is probably not going to fly, even
though I can't really find anything in the packaging guidelines that prevents
you from doing it (closest would be
http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
but there's no mention about .src.rpm). I see some problems with your approach:

 - You are shipping a specific version of source that is already present in
fedora
 - You will have to ship newer, static .src.rpm whenever GDM gets a newer
version
 - You may end up fighting duplicated libraries

The approach, in my opinion, would  be to get in touch with the GDM packagers
and ask for your stuff to be included if it's not already. It is not entirely
apparent why requiring gdm-devel wouldn't work right now.

Now, on with the revision proper:

[kad at propane rpmbuild ]$ rpmlint SPECS/ovirt-guest-agent.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[kad at propane rpmbuild ]$ rpmlint SRPMS/ovirt-guest-agent-1.0.0-2.fc16.src.rpm 
ovirt-guest-agent.src: W: summary-not-capitalized C oVirt Guest Agent
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

("oVirt" is not capitalized in the main oVirt package, so I guess that warning
is ok)

[BLOCKER] The License field in the package spec file must match the actual
license - the COPYING file in the source tarball has GPLv3, you have GPLv2+ in
the spec file
[BLOCKER] The sources used to build the package must match the upstream source,
as provided in the spec URL - You should take a look at
http://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control and
not use a self-hosted source tarball (Source0:
http://ghammer.fedorapeople.org/%{name}-%{version}.tar.bz2)
[BLOCKER] The package MUST successfully compile and build into binary rpms on
at least one primary architecture - I got this error when trying a mock build:
xorg-x11-server-Xorg is needed by gdm-1:3.2.1.1-6.fc16.x86_64.
[BLOCKER] Each package must consistently use macros - I see a mix of $MACRO and
%{macro}, use one style and stick with. Also, if you don't plan on supporting
EPEL/RH, you should do away with the whole RPM_BUILD_ROOT [citation needed,
maybe?]

Additionally, it seems you include some python code in there, so, using
http://fedoraproject.org/wiki/Packaging:Python as guide:

[BLOCKER] To build a package containing python2 files, you need to have
BuildRequires: python2-devel

Some additional suggestions:
 - defattr is not needed anymore, if I recall
 - you should name your different targets appropriately like
"ovirt-guest-agent-gdm-plugin" instead of just "gdm-plugin"
 - in your %pre step, you add a user rhevagent if it doesn't exist, but you are
also hardcoding the UID. You should leave that to the system to figure out and
just use -r for system users

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.