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

Tue Feb 7 06:27:32 UTC 2012

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 #14 from Jorge A Gallegos <kad at blegh.net> 2012-02-07 01:27:30 EST ---
(In reply to comment #12)

See my responses inline

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

That sounds weird, and actually sounds like a shortcoming of the GDM plugin
framework and/or the gdm-devel package. I'd still say this is not permisible,
but I'll leave the decision to a more seasoned packager (Steven?)

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

Cool!

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

>From that wiki page: "There are several cases where upstream is not providing
the source to you in an upstream tarball. In these cases you must document how
to generate the tarball used in the rpm either through a spec file comment or a
script included as a separate SourceX"

Check that URL I pasted above again, you should actually describe how to create
that tarball from git, not point to the tarball you created

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

Try following Steven's advice and bump the rpm changelog every time you make
changes (hint: rpmdev-bumpspec foo.spec). Also try submitting a koji scratch
build (https://fedoraproject.org/wiki/Using_the_Koji_build_system) and paste
the URL each time. In this case I just submitted it and the results are here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3767959
As you can see, there are still some unmet build dependencies:
error: Failed build dependencies:
 xorg-x11-server-Xorg is needed by gdm-1:3.2.1.1-6.fc17.x86_64
If you don't want to use koji you can use mock in your machine like this: mock
-r fedora-16-x86_64 rebuild package-1.2-3.src.rpm. This should do a scratch
build in a sanboxed chroot so you can see if all dependencies are met.

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

Fair enough, I suppose if an existing accepted package follows the same tone is
fair to assume this is OK.

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

I was just pointing out the python packaging guidelines.

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

Yeah, the defattr thing I got from another review somewhere... let me see, here
it is https://bugzilla.redhat.com/show_bug.cgi?id=725292#c1 and a couple of
other reviews I've seen it (and perhaps you are correct, packaging docs *could*
be outdated) 

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

Cool

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

I don't think I quite understand this last one... what does "assigned" mean? If
I do in my system (before installing this package) `/usr/sbin/useradd -u 175 -o
-r haproxy -c "High Availability Proxy Daemon User" -d / -s /sbin/nologin` and
then try installing this package, the %prep step would likely fail, I think?

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