Bug 889546

Originally Gal Hammer was the package maintainer, however I will take this over therefore I clone the bug and close the old one.

Please find the history here:

+++ This bug was initially created as a clone of Bug #772608 +++

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL: http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.0-1.fc16.src.rpm

Description:

This is the oVirt managment agent running inside the guest. The agent
interfaces with the oVirt manager, supplying heart-beat info as well as
runtime data from within the guest itself. The agent also accepts
control commands to be run executed within the OS (like: shutdown and
restart).

--- Additional comment from Pavel Zhukov on 2012-01-11 02:00:55 EST ---

Hello. I'm tring to use ovirt. Just few comments

There is no need to include the following packages: redhat-rpm-config or their dependencies as BuildRequires because they would occur too often. These packages are considered the minimum build environment. 
http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
rpm-python and dbus-python depend from python. So there is no need to include python to Requires. 
automake depends from autoconf etc.

--- Additional comment from Gal Hammer on 2012-01-17 08:04:49 EST ---

(In reply to comment #1)
> Hello. I'm tring to use ovirt. Just few comments
> 
> There is no need to include the following packages: redhat-rpm-config or their
> dependencies as BuildRequires because they would occur too often. These
> packages are considered the minimum build environment. 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
> rpm-python and dbus-python depend from python. So there is no need to include
> python to Requires. 
> automake depends from autoconf etc.

Thanks. I removed the relevant lines from the spec file.

--- Additional comment from Stephen Gordon on 2012-01-24 16:57:18 EST ---

- In the Requires and BuildRequires statements some use %define macros for the version and some hardcode them. It would probably be best to choose one method of specifying the versions and use it consistently.

- The Source0 line is expected to contain a URL pointing to the archive or, where applicable, just the name of the archive accompanied by a comment explaining where it was generated from:

http://fedoraproject.org/wiki/Packaging/SourceURL

- Given the ExclusiveArch directive I am not sure this macro is required?:

%ifnarch s390 s390x ppc64
BuildRequires: xorg-x11-server-Xorg
%endif  

- There are a few spelling errors in the description, as well as mixed use of tabs/spaces, detected by rpmlint.

Output of rpmlint on the SRPM (given oVirt is the project name I think it is probably fair to ignore the first warning):

ovirt-guest-agent.src: W: summary-not-capitalized C oVirt Guest Agent
ovirt-guest-agent.src: W: spelling-error %description -l en_US managment -> management, engagement, Mantegna
ovirt-guest-agent.src: W: spelling-error %description -l en_US runtime -> run time, run-time, rudiment
ovirt-guest-agent.src:185: W: mixed-use-of-spaces-and-tabs (spaces: line 185, tab: line 180)
ovirt-guest-agent.src: W: invalid-url Source0: ovirt-guest-agent-1.0.0.tar.bz2
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

--- Additional comment from Gal Hammer on 2012-01-25 03:12:36 EST ---

(In reply to comment #3)

> - In the Requires and BuildRequires statements some use %define macros for the
> version and some hardcode them. It would probably be best to choose one method
> of specifying the versions and use it consistently.

The %define macros were copied from the gdm.spec file. I moved them closer to the GDM's BuildRequires section to separate them from the guest's BuildRequires section.
 
> - The Source0 line is expected to contain a URL pointing to the archive or,
> where applicable, just the name of the archive accompanied by a comment
> explaining where it was generated from:
> 
> http://fedoraproject.org/wiki/Packaging/SourceURL

Fixed.
 
> - Given the ExclusiveArch directive I am not sure this macro is required?:
> 
> %ifnarch s390 s390x ppc64
> BuildRequires: xorg-x11-server-Xorg
> %endif  

It is just a left over from the GDM's spec file. I removed it.
 
> - There are a few spelling errors in the description, as well as mixed use of
> tabs/spaces, detected by rpmlint.

Fixed.

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.0-2.fc16.src.rpm

--- Additional comment from Stephen Gordon on 2012-01-25 09:47:54 EST ---

(In reply to comment #4)
> (In reply to comment #3)
> > %ifnarch s390 s390x ppc64
> > BuildRequires: xorg-x11-server-Xorg
> > %endif  
> 
> It is just a left over from the GDM's spec file. I removed it.

To be clear I was suggesting that:

%ifnarch s390 s390x ppc64
BuildRequires: xorg-x11-server-Xorg
%endif  

Become:

BuildRequires: xorg-x11-server-Xorg

AFAIK the package is required to build on the supported architectures (i686, x86_64 thanks to the ExclusiveArch)), certainly mock suggests this is the case:

error: Failed build dependencies:
        xorg-x11-server-Xorg is needed by gdm-1:3.2.1.1-6.fc16.x86_64

I'll admit I am a little confused as to why this package has to bundle the gdm SRPM rather than setting a BuildRequires on gdm but you have commented it and I will have to take your word for it for now. Another reviewer might want to look at this more closely :).

--- Additional comment from Stephen Gordon on 2012-01-25 10:08:54 EST ---

Additionally the following files appear to be installed but not packaged (this comes out of the rpmbuild check):

/etc/pam.d/ovirt-hibernate
/etc/security/console.apps/ovirt-hibernate
/usr/share/ovirt-guest-agent/hibernate

--- Additional comment from Gal Hammer on 2012-01-25 14:15:36 EST ---

(In reply to comment #5)

> To be clear I was suggesting that:
> 
> %ifnarch s390 s390x ppc64
> BuildRequires: xorg-x11-server-Xorg
> %endif  
> 
> Become:
> 
> BuildRequires: xorg-x11-server-Xorg

Fixed.
 
> I'll admit I am a little confused as to why this package has to bundle the gdm
> SRPM rather than setting a BuildRequires on gdm but you have commented it and I
> will have to take your word for it for now. Another reviewer might want to look
> at this more closely :).

I hope someone will find a better solution. The agent's build time will be much faster. :-)

The problem is that th gdm package doesn't include files which libtool requires. So I couldn't solve it with a BuildRequires on gdm.

--- Additional comment from Gal Hammer on 2012-01-25 14:17:04 EST ---

(In reply to comment #6)
> Additionally the following files appear to be installed but not packaged (this
> comes out of the rpmbuild check):
> 
> /etc/pam.d/ovirt-hibernate
> /etc/security/console.apps/ovirt-hibernate
> /usr/share/ovirt-guest-agent/hibernate

These are files required for a feature that is not included in the reviewed version.

Where did you see a reference to them?

--- Additional comment from Stephen Gordon on 2012-01-25 15:10:33 EST ---

When I added the "BuildRequires: xorg-x11-server-XorgWhen" entry back to this version and then attempted to build the package those files were listed in the output from rpmbuild. 

What it is (or was at least) effectively saying is that those files were detected as being deployed by the RPM but are not listed in the %files section (either explicitly or as part of the globs).

--- Additional comment from Steven Dake on 2012-01-30 09:06:17 EST ---

Gal,

I will sponsor you as a packager.  The first step is to review a Fedora contributor's package.  Please execute the review process on this bugzilla:

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

--- Additional comment from Jorge A Gallegos on 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@propane rpmbuild ]$ rpmlint SPECS/ovirt-guest-agent.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[kad@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

--- Additional comment from Gal Hammer on 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.

--- Additional comment from Steven Dake on 2012-02-05 12:00:31 EST ---

Gal,

A short recommendation when making updates to spec files.  After each revision of a spec file, please make a new bugzilla comment entry separate from questions for the reviewer that states the spec file location and src.rpm file location.

Also take care to increase the version numbers and update changelog in the spec file.

Example:

"
Updated spec file to fix previous reported problems: 
SPEC: link
SRPM: link
"

--- Additional comment from Jorge A Gallegos on 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?

--- Additional comment from Steven Dake on 2012-02-19 15:04:10 EST ---

Gal,

First please respond to questions in comment #14.

Second, I'd like you to act as a reviewer for another package on your way to making into the packagers group.  Please follow through with the entire process of getting the package into shape.

The bugzilla is:
https://bugzilla.redhat.com/show_bug.cgi?id=786860

--- Additional comment from Gal Hammer on 2012-02-22 04:40:38 EST ---

New version:

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.1-2.fc16.src.rpm

- updated required selinux-policy version (related to rhbz#791113).
- added a support to hibernate (s4) command.
- renamed user name to ovirtguest.
- reset version numbering after changing the package name.

--- Additional comment from Gal Hammer on 2012-02-22 04:50:41 EST ---

(In reply to comment #14)

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

AFAIK, there is no devel package to support GDM plugins development.
 
> 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

Since I'm providing the source as a tarball there is not reason to describe how to create that tarball. That's what the quote say.
 
> 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.

I'm not using koji at the moment. I've checked the new version (1.0.1-2) using a mock build on my local machine and it worked. I hope this is enough for koji too.
 
> I was just pointing out the python packaging guidelines.

Understood. However it would be nice to understand the rational behind it.

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

Check the file /usr/share/doc/setup-2.8.36/uidgid (from the setup pacakge). It include the "reserved" uids.

--- Additional comment from Steven Dake on 2012-03-06 13:07:45 EST ---

This package has a crazy number of build dependencies, but if you think you can keep it all working, more power to you :)

I would recommend changing 

BuildRequires: pkgconfig(libcanberra-gtk)

to 

BuildRequires: libcanberra-devel
The current mechanism is prone to error and hard to understand

Along those same lines 

Change 
BuildRequires: pkgconfig(accountsservices) to

to:
BuildRequires accountsservices-devel

# No gdm-devel package is available for plug-in development. So for now
# we build the gdm package.
Source1: gdm-3.2.1.1-6.fc16.src.rpm

Hacking around the problem by bundling a source rpm is not acceptable.  The proper solution is to fix the problem:

1. file a gdm bug
2a. if you want it fixed fast, sort out how to change gdm packaging to provide a devel package that exports what you need and feed it into the bugzilla
or
2b. block on gdm maintainer providing devel package

An alternative is not to build/provide the gdm plugin until this upstream problem is sorted out.  This is why mock is not building (gdm requires xorg server, which the build system can't sort out)

Even if this were permissible, I'm uncertain the build system could handle it properly.  Since it is not permissible, whether the build system could be hacked to make it work is not relevant ;)

The review is blocked on this issue.

--- Additional comment from Gal Hammer on 2012-03-28 06:22:42 EDT ---

New version:

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.2-1.fc16.src.rpm

- included a gpl-v2 copying file.
- build the gdm-plugin using the gdm-devel package.
- added a support for RHEL distribution.

--- Additional comment from Gal Hammer on 2012-03-28 06:29:25 EDT ---

(In reply to comment #18)
> This package has a crazy number of build dependencies, but if you think you can
> keep it all working, more power to you :)

That's all gdm staff :-).
 
> I would recommend changing 
> 
> BuildRequires: pkgconfig(libcanberra-gtk)
> 
> to 
> 
> BuildRequires: libcanberra-devel
> The current mechanism is prone to error and hard to understand
> 
> Along those same lines 
> 
> Change 
> BuildRequires: pkgconfig(accountsservices) to
> 
> to:
> BuildRequires accountsservices-devel
> 
> # No gdm-devel package is available for plug-in development. So for now
> # we build the gdm package.
> Source1: gdm-3.2.1.1-6.fc16.src.rpm
> 
> Hacking around the problem by bundling a source rpm is not acceptable.  The
> proper solution is to fix the problem:
> 
> 1. file a gdm bug
> 2a. if you want it fixed fast, sort out how to change gdm packaging to provide
> a devel package that exports what you need and feed it into the bugzilla
> or
> 2b. block on gdm maintainer providing devel package
> 
> An alternative is not to build/provide the gdm plugin until this upstream
> problem is sorted out.  This is why mock is not building (gdm requires xorg
> server, which the build system can't sort out)
> 
> Even if this were permissible, I'm uncertain the build system could handle it
> properly.  Since it is not permissible, whether the build system could be
> hacked to make it work is not relevant ;)
> 
> The review is blocked on this issue.

gdm-3 added a gdm-devel package, so I've fixed the build process to use it. It is no longer required to build the whole gdm source tree. The exception is for older distribution (e.g. rhel) which still use gdm-2.

--- Additional comment from Steven Dake on 2012-03-28 10:07:30 EDT ---

I am a bit ill with a cold and head hurts.  I'll execute a review when I'm feeling a bit better (couple days?).

Regards
-steve

--- Additional comment from Gal Hammer on 2012-03-28 11:31:45 EDT ---

(In reply to comment #21)
> I am a bit ill with a cold and head hurts.  I'll execute a review when I'm
> feeling a bit better (couple days?).

It sounds like the normal symptoms for someone who is working around the agent ;-).

Get well soon.

--- Additional comment from Gal Hammer on 2012-04-10 10:22:12 EDT ---

New version:

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.3-1.fc16.src.rpm

- package was renamed to rhevm-guest-agent in RHEL distribution.
- fixed gdm-plugin build requires.

--- Additional comment from Steven Dake on 2012-04-12 10:48:36 EDT ---

1)
The BuildRoot is not necessary, Fedora figures this out automatically:

BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

2)
This looks a little suspicious.  Especially in FC17+ the gdm release shouldn't be el6?

%define gdm_version gdm-2.30.4
%define gdm_release %{gdm_version}-14.el6

Are these still needed now that your using the gdm-devel package?
# The following requirements were copied from the gdm.spec file.
BuildRequires: pkgconfig(libcanberra-gtk)
BuildRequires: scrollkeeper >= 0:%{scrollkeeper_version}
BuildRequires: pango-devel >= 0:%{pango_version}
BuildRequires: gtk2-devel >= 0:%{gtk2_version}
BuildRequires: libglade2-devel >= 0:%{libglade2_version}
BuildRequires: libgnomeui-devel >= 0:%{libgnomeui_version}
BuildRequires: pam-devel >= 0:%{pam_version}
BuildRequires: fontconfig >= 0:%{fontconfig_version}
BuildRequires: desktop-file-utils >= %{desktop_file_utils_version}
BuildRequires: gail-devel >= 0:%{gail_version}

if they are still needed, please get rid of the defines:
%define libauditver 1.0.6
%define pango_version 1.2.0
%define gtk2_version 2.6.0
%define libglade2_version 2.0.0
%define libgnomeui_version 2.2.0
%define scrollkeeper_version 0.3.4
%define pam_version 0.99.8.1-11
%define desktop_file_utils_version 0.2.90
%define gail_version 1.2.0
%define nss_version 3.11.1
%define fontconfig_version 2.6.0

and put them directly in the buildrequires.  The current spec file is very difficult to read.

3)
Make spec files fedora specific please otherwise the packaging is very difficult to read
ie:
%if 0%{?rhel}
    --with-gdm-src-dir=%{_topdir}/BUILD/%{gdm_version} \
    --with-simple-greeter-plugins-dir=%{_libdir}/gdm/simple-greeter/plugins \
%endif
%if 0%{?rhel}
    sed -i "s~parent->setObjectName(\"welcome\");~parent->setObjectName(\"talker\");~" kdm-plugin/src/kgreet_ovirtcred.cpp
%endif

just assume systemd will be used
    # Install systemd script.
    install -Dm 0644 ovirt-guest-agent/ovirt-guest-agent.service $RPM_BUILD_ROOT%{_unitdir}/ovirt-guest-agent.service

%if 0%{?rhel}
    # No longer needed and is provided by the gdm package.
    rm -f $RPM_BUILD_ROOT%{_libdir}/libgdmsimplegreeter.so

    rm -f $RPM_BUILD_ROOT%{_libdir}/gdm/simple-greeter/plugins/ovirtcred.a
    rm -f $RPM_BUILD_ROOT%{_libdir}/gdm/simple-greeter/plugins/ovirtcred.la
%else

4)
/var/run is mounted dynamically as a tempfs.  As a result, you will not want it in the package.  If you need directories in /var/run, you will need to create them.  I don't like it either if it makes you feel better ;)

mkdir -p $RPM_BUILD_ROOT%{_localstatedir}/run/ovirt-guest-agent

5)
The clean section isn't needed in fedora since about 12ish or so.  It can be removed.

6) please do not use static IDS (such as 175) in useradd.  Also the proper thing is not being done here re handling useradd failures.
See http://fedoraproject.org/wiki/Packaging:UsersAndGroups for the proper mechanism.

After correcting the above, I'll go through an official review

--- Additional comment from Gal Hammer on 2012-04-15 04:30:21 EDT ---

New version:

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.3-2.fc16.src.rpm

- removed the RHEL distribution support for the review process.
- removed BuildRoot header and %clean section.
- fixed user creation.

--- Additional comment from Gal Hammer on 2012-04-15 04:37:45 EDT ---

(In reply to comment #24)

> 1)
> The BuildRoot is not necessary, Fedora figures this out automatically:

Removed BuildRoot.
 
> 2)
> This looks a little suspicious.  Especially in FC17+ the gdm release shouldn't
> be el6?

Fixed by removing support for RHEL distribution for now.

> 3)
> Make spec files fedora specific please otherwise the packaging is very

Fixed.

> 4)
> /var/run is mounted dynamically as a tempfs.  As a result, you will not want 

Fixed. It was a leftover from RHEL 6. The /var/run path is not tempfs there.

> 5)
> The clean section isn't needed in fedora since about 12ish or so.  It can be
> removed.

Removed %clean section.

> 6) please do not use static IDS (such as 175) in useradd.  Also the proper
> thing is not being done here re handling useradd failures.
> See http://fedoraproject.org/wiki/Packaging:UsersAndGroups for the proper
> mechanism.

Fixed the user creation to match the guide lines in the link.

I'm still using the static id. It is an assign id (see the setup package).
 
> After correcting the above, I'll go through an official review

Fingers crossed! :-)

--- Additional comment from David Jaša on 2012-05-10 15:49:27 EDT ---

When building on .fc17, I got this error:

Processing files: ovirt-guest-agent-1.0.3-2.fc17.x86_64
error: File not found: /home/djasa/rpmbuild/BUILDROOT/ovirt-guest-agent-1.0.3-2.fc17.x86_64/lib/systemd/system/ovirt-guest-agent.service

Changing hardcoded path of said file in %files to %{_unitdir}/ovirt-guest-agent.service fixed it for me:

--- rpmbuild/SPECS/ovirt-guest-agent.spec.orig	2012-04-15 09:42:39.000000000 +0200
+++ rpmbuild/SPECS/ovirt-guest-agent.spec	2012-05-10 21:45:16.498458778 +0200
@@ -178,7 +178,7 @@
 %{_datadir}/ovirt-guest-agent/GuestAgentLinux2.py*
 %attr (755,root,root) %{_datadir}/ovirt-guest-agent/LockActiveSession.py*
 %attr (755,root,root) %{_datadir}/ovirt-guest-agent/hibernate
-/lib/systemd/system/ovirt-guest-agent.service
+%{_unitdir}/ovirt-guest-agent.service
 
 %doc AUTHORS COPYING NEWS README
 

/mine fingers crossed, too! :)

--- Additional comment from Gal Hammer on 2012-05-13 06:54:10 EDT ---

(In reply to comment #27)

> Changing hardcoded path of said file in %files to
> %{_unitdir}/ovirt-guest-agent.service fixed it for me:

Thanks. Applied.

--- Additional comment from Steven Dake on 2012-05-13 11:54:21 EDT ---

Gal,

I'll review today.  Need bit of time to get an F17 system in order as this doesn't build on F16.

--- Additional comment from Steven Dake on 2012-05-14 13:18:55 EDT ---

Gal,

Trying to build your srpm in comment #25, I run into the following problem:

RPM build errors:
    File not found: /root/rpmbuild/BUILDROOT/ovirt-guest-agent-1.0.3-2.fc17.x86_64/lib/systemd/system/ovirt-guest-agent.service


Comment #27 may fix the problem, but I didn't try as the srpm must come from an upstream location to be merged.  Please create a new spec file and rpm with the appropriate changes.

Regards
-steve

--- Additional comment from Gal Hammer on 2012-05-15 05:19:05 EDT ---

New version:

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.4-1.fc16.src.rpm

- replaced "with" usage with a python 2.4 compatible way.
- added files to support RHEL-5 distribution.
- added more detailed memory statistics.
- fixed build on fc-17 (use the %{_unitdir} macro).

--- Additional comment from Gal Hammer on 2012-05-15 05:21:58 EDT ---

(In reply to comment #30)
> Gal,
> 
> Trying to build your srpm in comment #25, I run into the following problem:
> 
> RPM build errors:
>     File not found:
> /root/rpmbuild/BUILDROOT/ovirt-guest-agent-1.0.3-2.fc17.x86_64/lib/systemd/system/ovirt-guest-agent.service
> 
> 
> Comment #27 may fix the problem, but I didn't try as the srpm must come from an
> upstream location to be merged.  Please create a new spec file and rpm with the
> appropriate changes.

Fixed. See Comment #31.
 
> Regards
> -steve

--- Additional comment from Steven Dake on 2012-05-17 16:04:45 EDT ---

rpmlint spits out a bunch of errors:

Please review:
https://fedoraproject.org/wiki/ParagNemade/CommonRpmlintErrors

and fix as appropriate:

[root@f17 x86_64]# rpmlint ovirt-guest-agent-1.0.4-1.fc17.x86_64.rpm
ovirt-guest-agent.x86_64: W: summary-not-capitalized C oVirt Guest Agent
ovirt-guest-agent.x86_64: E: no-binary
ovirt-guest-agent.x86_64: W: only-non-binary-in-usr-lib
ovirt-guest-agent.x86_64: W: conffile-without-noreplace-flag /etc/ovirt-guest-agent.conf
ovirt-guest-agent.x86_64: E: non-executable-script /usr/share/ovirt-guest-agent/VirtIoChannel.py 0644L /usr/bin/python
ovirt-guest-agent.x86_64: E: wrong-script-end-of-line-encoding /usr/share/ovirt-guest-agent/VirtIoChannel.py
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-locksession
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-hibernate
ovirt-guest-agent.x86_64: W: non-standard-uid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent.x86_64: W: non-standard-gid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent.x86_64: E: non-executable-script /usr/share/ovirt-guest-agent/OVirtAgentLogic.py 0644L /usr/bin/python
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-shutdown
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/55-ovirt-guest-agent.rules
ovirt-guest-agent.x86_64: E: non-executable-script /usr/share/ovirt-guest-agent/GuestAgentLinux2.py 0644L /usr/bin/python
ovirt-guest-agent.x86_64: E: wrong-script-end-of-line-encoding /usr/share/ovirt-guest-agent/GuestAgentLinux2.py
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-shutdown
ovirt-guest-agent.x86_64: E: non-executable-script /usr/share/ovirt-guest-agent/CredServer.py 0644L /usr/bin/python
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-hibernate
ovirt-guest-agent.x86_64: E: zero-length /usr/share/doc/ovirt-guest-agent-1.0.4/NEWS
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-locksession
ovirt-guest-agent.x86_64: W: log-files-without-logrotate /var/log/ovirt-guest-agent
ovirt-guest-agent.x86_64: W: dangerous-command-in-%post ln
ovirt-guest-agent.x86_64: W: dangerous-command-in-%postun rm
1 packages and 0 specfiles checked; 8 errors, 16 warnings.

[root@f17 x86_64]# rpmlint ovirt-guest-agent-debuginfo-1.0.4-1.fc17.x86_64.rpm

ovirt-guest-agent-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/ovirt-guest-agent-1.0.4/gdm-plugin/gdm-ovirtcred-extension.c
ovirt-guest-agent-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/ovirt-guest-agent-1.0.4/gdm-plugin/gdm-ovirtcred-extension.h
1 packages and 0 specfiles checked; 2 errors, 0 warnings.

[root@f17 x86_64]# rpmlint ovirt-guest-agent-gdm-plugin-1.0.4-1.fc17.x86_64.rpm
ovirt-guest-agent-gdm-plugin.x86_64: W: spelling-error %description -l en_US login -> loin, logic, lo gin
ovirt-guest-agent-gdm-plugin.x86_64: W: conffile-without-noreplace-flag /etc/pam.d/gdm-ovirtcred
ovirt-guest-agent-gdm-plugin.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


[root@f17 x86_64]# rpmlint ovirt-guest-agent-kdm-plugin-1.0.4-1.fc17.x86_64.rpm

ovirt-guest-agent-kdm-plugin.x86_64: W: spelling-error %description -l en_US login -> loin, logic, lo gin
ovirt-guest-agent-kdm-plugin.x86_64: W: conffile-without-noreplace-flag /etc/pam.d/kdm-ovirtcred
ovirt-guest-agent-kdm-plugin.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

[root@f17 x86_64]# rpmlint ovirt-guest-agent-pam-module-1.0.4-1.fc17.x86_64.rpm
ovirt-guest-agent-pam-module.x86_64: W: summary-not-capitalized C oVirt Guest Agent PAM module
ovirt-guest-agent-pam-module.x86_64: W: spelling-error %description -l en_US login -> loin, logic, lo gin
ovirt-guest-agent-pam-module.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

[root@f17 SRPMS]# rpmlint ovirt-guest-agent-1.0.4-1.fc17.src.rpm
ovirt-guest-agent.src: W: summary-not-capitalized C oVirt Guest Agent
ovirt-guest-agent.src:208: W: macro-in-%changelog %{_unitdir}
ovirt-guest-agent.src:212: W: macro-in-%changelog %clean
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

--- Additional comment from Gal Hammer on 2012-05-20 09:34:36 EDT ---

New version (1.0.5-1):

Spec URL: http://ghammer.fedorapeople.org/ovirt-guest-agent.spec
SRPM URL:
http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.5-1.fc16.src.rpm

- fixed 'udevadm trigger' command line (bz#819945).
- fixed various rpmlint errors and warnings.

--- Additional comment from Gal Hammer on 2012-05-20 09:45:31 EDT ---

(In reply to comment #33)

I handled all the errors but one and reduce the number of warnings.

> [root@f17 x86_64]# rpmlint ovirt-guest-agent-1.0.4-1.fc17.x86_64.rpm
> ovirt-guest-agent.x86_64: E: no-binary

Although the agent is no-arch (Python) the sub-packages are compiled (arch depended). AFAIK there is no way to create an arch depended sub-package with a main no-arch package.

> ovirt-guest-agent.x86_64: W: non-standard-uid /var/log/ovirt-guest-agent
> ovirtagent
> ovirt-guest-agent.x86_64: W: non-standard-gid /var/log/ovirt-guest-agent
> ovirtagent

These uid/gid are assigned to this package.

> ovirt-guest-agent.x86_64: W: log-files-without-logrotate
> /var/log/ovirt-guest-agent

I'm using the Python's build in log rotation.

> ovirt-guest-agent-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/ovirt-guest-agent-1.0.4/gdm-plugin/gdm-ovirtcred-extension.c
> ovirt-guest-agent-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/ovirt-guest-agent-1.0.4/gdm-plugin/gdm-ovirtcred-extension.h

Fixed.

The current rpmlint output is:

]$ rpmlint /var/lib/mock/fedora-16-x86_64/result/*.rpm
ovirt-guest-agent.src: W: summary-not-capitalized C oVirt Guest Agent
ovirt-guest-agent.x86_64: W: summary-not-capitalized C oVirt Guest Agent
ovirt-guest-agent.x86_64: E: no-binary
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-locksession
ovirt-guest-agent.x86_64: W: non-standard-uid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent.x86_64: W: non-standard-gid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-shutdown
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/55-ovirt-guest-agent.rules
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-hibernate
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-shutdown
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/pam.d/ovirt-hibernate
ovirt-guest-agent.x86_64: W: non-conffile-in-etc /etc/security/console.apps/ovirt-locksession
ovirt-guest-agent.x86_64: W: log-files-without-logrotate /var/log/ovirt-guest-agent
ovirt-guest-agent.x86_64: W: dangerous-command-in-%post ln
ovirt-guest-agent.x86_64: W: dangerous-command-in-%postun rm
ovirt-guest-agent-gdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-gdm-plugin.x86_64: W: non-conffile-in-etc /etc/pam.d/gdm-ovirtcred
ovirt-guest-agent-kdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-kdm-plugin.x86_64: W: non-conffile-in-etc /etc/pam.d/kdm-ovirtcred
ovirt-guest-agent-pam-module.x86_64: W: summary-not-capitalized C oVirt Guest Agent PAM module
ovirt-guest-agent-pam-module.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 1 errors, 21 warnings.