[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

--- Comment #12 from Gal Hammer <ghammer at redhat.com> 2012-02-05 04:06:40 EST ---
(In reply to comment #11)

> 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.

The reason I include the gdm src.rpm file is because I didn't find another way
to compile a gdm plugin outside the gdm's source tree. It is used only during
compilation and linkage time and it is not shipped with the plugin itself.

> [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

Fixed.

> [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)

As far as I understood this is allowed
(http://fedoraproject.org/wiki/Packaging/SourceURL).

> [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.

Probably a bug after fixes done after a review. I've updated a new version
(http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.1-1.fc16.src.rpm) which
compile on both 32 and 64 bits.

> [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?]

I'm not sure I understand this. I checked with the gdm spec file (as an
example) and it look pretty much like mine (using %macro expect for
$RPM_BUILD_ROOT).

> 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

Why do I need to include it? My build machine doesn't have the python2-devel
package installed.

> Some additional suggestions:
>  - defattr is not needed anymore, if I recall

"The %files section normally begins with a %defattr line which sets the default
file permissions."
(http://fedoraproject.org/wiki/How_to_create_an_RPM_package).

Maybe it is time to review/update the manual... :-)

>  - you should name your different targets appropriately like
> "ovirt-guest-agent-gdm-plugin" instead of just "gdm-plugin"

The rpmbuild does that for me. The rpm file name is called:
ovirt-guest-agent-gdm-plugin-1.0.1-1.fc16.x86_64.

>  - 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

This is an assigned uid.

-- 
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.