This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 889546 - Review Request: ovirt-guest-agent - oVirt Guest Agent
Review Request: ovirt-guest-agent - oVirt Guest Agent
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Stanislav Ochotnicky
Fedora Extras Quality Assurance
:
: 772608 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-21 11:56 EST by Vinzenz Feenstra [evilissimo]
Modified: 2014-05-19 08:01 EDT (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 772608
Environment:
Last Closed: 2013-02-24 03:34:48 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
sochotni: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Vinzenz Feenstra [evilissimo] 2012-12-21 11:56:49 EST
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.
Comment 1 Vinzenz Feenstra [evilissimo] 2012-12-21 12:06:55 EST
New version (1.0.6-2):

Spec URL: http://evilissimo.fedorapeople.org/specs/ovirt-guest-agent/ovirt-guest-agent.spec
SRPM URL: http://evilissimo.fedorapeople.org/specs/ovirt-guest-agent/ovirt-guest-agent-1.0.6-2.fc17.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).


Koji Scratch Builds:
f17: http://koji.fedoraproject.org/koji/taskinfo?taskID=4811628
f18: http://koji.fedoraproject.org/koji/taskinfo?taskID=4811638
rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=4811644

RPMLint warnings:
# No need for documentation
ovirt-guest-agent-kdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-gdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-pam-module.x86_64: W: no-documentation

# service unit
ovirt-guest-agent-common.noarch: W: only-non-binary-in-usr-lib

# These are intentionally NOT noreplace. We DO want them to be replaced if there's a higher version or anything which needs it to be fixed
ovirt-guest-agent-kdm-plugin.x86_64: W: conffile-without-noreplace-flag /etc/pam.d/kdm-ovirtcred
ovirt-guest-agent-gdm-plugin.x86_64: W: conffile-without-noreplace-flag /etc/pam.d/gdm-ovirtcred
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-locksession
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/udev/rules.d/55-ovirt-guest-agent.rules
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-locksession

# ovirtagent is the user gid for this package (uid/gid 175)
ovirt-guest-agent-common.noarch: W: non-standard-uid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent-common.noarch: W: non-standard-gid /var/log/ovirt-guest-agent ovirtagent

# We have our own log rotate
ovirt-guest-agent-common.noarch: W: log-files-without-logrotate /var/log/ovirt-guest-agent
5 packages and 0 specfiles checked; 0 errors, 17 warnings.
Comment 2 Vinzenz Feenstra [evilissimo] 2012-12-21 12:08:16 EST
*** Bug 772608 has been marked as a duplicate of this bug. ***
Comment 3 Stanislav Ochotnicky 2013-01-21 12:30:17 EST
Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- gtk-update-icon-cache is invoked when required
  Note: icons in ovirt-guest-agent-gdm-plugin
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
- post/postun scriptlets of kdm-plugin
- noreplace on some things, just because user *can* break stuff by configuring something doesn't mean it should be forbidden. Especially the pam configuration. Most likely also /etc/security/ and dbus files. There are no simple reasons for admins to touch them, but if they do-> they obviously had their reasons.


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config /etc/pam.d/ovirt-locksession %config
     /etc/pam.d/ovirt-shutdown %config /etc/pam.d/ovirt-hibernate %config
     %attr (644,root,root) /etc/udev/rules.d/55-ovirt-guest-agent.rules
     %config /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf %config
     /etc/security/console.apps/ovirt-locksession %config
     /etc/security/console.apps/ovirt-shutdown %config
     /etc/security/console.apps/ovirt-hibernate %config /etc/pam.d/gdm-
     ovirtcred %config /etc/pam.d/kdm-ovirtcred

I'd prefer if this was discussed on packaging mailing list. From my POV, I'd say
it should be possible that local administrator will want to modify security
policies or pam settings and you should not overwrite his changes blindly on updates.

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ovirt-
     guest-agent-common , ovirt-guest-agent-pam-module , ovirt-guest-agent-
     gdm-plugin , ovirt-guest-agent-kdm-plugin
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[?]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Scriptlets must be sane, if used.

I *really* don't like post/postun of kdm-plugin subpackage. You are modifying
configuration of another package. IMO that's road to hell.

[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: ovirt-guest-agent-common-1.0.6-2.fc19.noarch.rpm
          ovirt-guest-agent-pam-module-1.0.6-2.fc19.x86_64.rpm
          ovirt-guest-agent-gdm-plugin-1.0.6-2.fc19.x86_64.rpm
          ovirt-guest-agent-kdm-plugin-1.0.6-2.fc19.x86_64.rpm
ovirt-guest-agent-common.noarch: W: only-non-binary-in-usr-lib
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-locksession
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/udev/rules.d/55-ovirt-guest-agent.rules
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-locksession
ovirt-guest-agent-common.noarch: W: non-standard-uid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent-common.noarch: W: non-standard-gid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent-common.noarch: W: log-files-without-logrotate /var/log/ovirt-guest-agent
ovirt-guest-agent-pam-module.x86_64: W: no-documentation
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
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
4 packages and 0 specfiles checked; 0 errors, 17 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint ovirt-guest-agent-kdm-plugin ovirt-guest-agent-common ovirt-guest-agent-gdm-plugi 
n ovirt-guest-agent-pam-module
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
ovirt-guest-agent-common.noarch: W: only-non-binary-in-usr-lib
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-locksession
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/udev/rules.d/55-ovirt-guest-agent.rules
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-shutdown
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/pam.d/ovirt-hibernate
ovirt-guest-agent-common.noarch: W: conffile-without-noreplace-flag /etc/security/console.apps/ovirt-locksession
ovirt-guest-agent-common.noarch: W: log-files-without-logrotate /var/log/ovirt-guest-agent
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
ovirt-guest-agent-pam-module.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 15 warnings.
# echo 'rpmlint-done:'



Requires
--------
ovirt-guest-agent-kdm-plugin (rpmlib, GLIBC filtered):
    /bin/sh
    config(ovirt-guest-agent-kdm-plugin)
    kdm
    libQtCore.so.4()(64bit)
    libQtDBus.so.4()(64bit)
    libQtGui.so.4()(64bit)
    libQtSvg.so.4()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libkdecore.so.5()(64bit)
    libkdeui.so.5()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    ovirt-guest-agent
    ovirt-guest-agent-pam-module
    rtld(GNU_HASH)

ovirt-guest-agent-common (rpmlib, GLIBC filtered):
    /bin/sh
    config(ovirt-guest-agent-common)
    dbus-python
    kernel
    python-ethtool
    rpm-python
    udev
    usermode

ovirt-guest-agent-gdm-plugin (rpmlib, GLIBC filtered):
    config(ovirt-guest-agent-gdm-plugin)
    gdm
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libdbus-1.so.3()(64bit)
    libdbus-glib-1.so.2()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgdk-x11-2.0.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgdmsimplegreeter.so.1()(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgtk-x11-2.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpam.so.0()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    ovirt-guest-agent
    ovirt-guest-agent-pam-module
    rtld(GNU_HASH)

ovirt-guest-agent-pam-module (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpam.so.0()(64bit)
    libpam.so.0(LIBPAM_1.0)(64bit)
    ovirt-guest-agent
    pam
    rtld(GNU_HASH)



Provides
--------
ovirt-guest-agent-kdm-plugin:
    config(ovirt-guest-agent-kdm-plugin)
    kgreet_ovirtcred.so()(64bit)
    ovirt-guest-agent-kdm-plugin
    ovirt-guest-agent-kdm-plugin(x86-64)

ovirt-guest-agent-common:
    config(ovirt-guest-agent-common)
    ovirt-guest-agent
    ovirt-guest-agent-common

ovirt-guest-agent-gdm-plugin:
    config(ovirt-guest-agent-gdm-plugin)
    libovirtcred.so()(64bit)
    ovirt-guest-agent-gdm-plugin
    ovirt-guest-agent-gdm-plugin(x86-64)

ovirt-guest-agent-pam-module:
    ovirt-guest-agent-pam-module
    ovirt-guest-agent-pam-module(x86-64)
    pam_ovirt_cred.so()(64bit)



Unversioned so-files
--------------------
ovirt-guest-agent-pam-module: /lib64/security/pam_ovirt_cred.so
ovirt-guest-agent-gdm-plugin: /usr/lib64/gdm/simple-greeter/extensions/libovirtcred.so
ovirt-guest-agent-kdm-plugin: /usr/lib64/kde4/kgreet_ovirtcred.so

MD5-sum check
-------------
http://resources.ovirt.org/releases/3.2/src/ovirt-guest-agent-1.0.6.tar.bz2 :
  CHECKSUM(SHA256) this package     : ab801cbee7b89251814988843b40dde65c40fc93554ceca9261889b2084f1c22
  CHECKSUM(SHA256) upstream package : ab801cbee7b89251814988843b40dde65c40fc93554ceca9261889b2084f1c22


Generated by fedora-review 0.3.1 (903b443) last change: 2012-12-20
Buildroot used: fedora-raw-x86_64
Command line :/home/w0rm/work/projects/fedora-review/try-fedora-review -b 889546
Comment 4 Vinzenz Feenstra [evilissimo] 2013-01-22 05:29:09 EST
(In reply to comment #3)
> Issues:
> =======
> - gtk-update-icon-cache is invoked when required
>   Note: icons in ovirt-guest-agent-gdm-plugin
>   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
OK Will be done

> - post/postun scriptlets of kdm-plugin
See below

> - noreplace on some things, just because user *can* break stuff by
> configuring something doesn't mean it should be forbidden. Especially the
> pam configuration. Most likely also /etc/security/ and dbus files. There are
> no simple reasons for admins to touch them, but if they do-> they obviously
> had their reasons.
See below

> [!]: %config files are marked noreplace or the reason is justified.
>      Note: No (noreplace) in %config /etc/pam.d/ovirt-locksession %config
>      /etc/pam.d/ovirt-shutdown %config /etc/pam.d/ovirt-hibernate %config
>      %attr (644,root,root) /etc/udev/rules.d/55-ovirt-guest-agent.rules
>      %config /etc/dbus-1/system.d/org.ovirt.vdsm.Credentials.conf %config
>      /etc/security/console.apps/ovirt-locksession %config
>      /etc/security/console.apps/ovirt-shutdown %config
>      /etc/security/console.apps/ovirt-hibernate %config /etc/pam.d/gdm-
>      ovirtcred %config /etc/pam.d/kdm-ovirtcred
> 
> I'd prefer if this was discussed on packaging mailing list. From my POV, I'd
> say
> it should be possible that local administrator will want to modify security
> policies or pam settings and you should not overwrite his changes blindly on
> updates.

Well I actually thought so myself, however I was asked to put it like this. But I'd rather obey fedora rules. If someone messes with this files and doesn't know what they are doing they have to fix it by themselves.

-> Will be changed


> [!]: Scriptlets must be sane, if used.
> 
> I *really* don't like post/postun of kdm-plugin subpackage. You are modifying
> configuration of another package. IMO that's road to hell.
> 

OK I'll remove it. This has to be solved then differently.
Comment 5 Vinzenz Feenstra [evilissimo] 2013-02-06 09:59:05 EST
New version (1.0.6-3):

Spec URL: http://evilissimo.fedorapeople.org/specs/ovirt-guest-agent/ovirt-guest-agent.spec
SRPM URL: http://evilissimo.fedorapeople.org/specs/ovirt-guest-agent/ovirt-guest-agent-1.0.6-3.fc17.src.rpm

Changes:
* Tue Jan 22 2013 Vinzenz Feenstra <vfeenstr@redhat.com> - 1.0.6-3              
- All config files are now 'noreplace'                                          
- Refreshing the gtk icon cache during installation                             
- The package is not modifying the kdmrc any longer                             
- Using new systemd macros where appropriate                                    

Koji Scratch Builds:
f17: http://koji.fedoraproject.org/koji/taskinfo?taskID=4933162
f18: http://koji.fedoraproject.org/koji/taskinfo?taskID=4933163
rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=4933161

RPM lint warnings:
~/rpmbuild/RPMS$ rpmlint `find -name ovirt-guest-agent*-1.0.6-3*`

# No need for documentation
ovirt-guest-agent-gdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-kdm-plugin.x86_64: W: no-documentation
ovirt-guest-agent-pam-module.x86_64: W: no-documentation

# service unit
ovirt-guest-agent-common.noarch: W: only-non-binary-in-usr-lib

# ovirtagent is the user gid for this package (uid/gid 175)
ovirt-guest-agent-common.noarch: W: non-standard-uid /var/log/ovirt-guest-agent ovirtagent
ovirt-guest-agent-common.noarch: W: non-standard-gid /var/log/ovirt-guest-agent ovirtagent

# We have our own log rotate
ovirt-guest-agent-common.noarch: W: log-files-without-logrotate /var/log/ovirt-guest-agent
5 packages and 0 specfiles checked; 0 errors, 7 warnings.
Comment 6 Stanislav Ochotnicky 2013-02-07 09:33:11 EST
The package looks sane to me now. You still have %global _kdmrc /etc/kde/kdm/kdmrc so you probably want to remove it before commit/build. Other than that I am fine with explanations/fixes. 

APPROVED
Comment 7 Vinzenz Feenstra [evilissimo] 2013-02-07 12:11:20 EST
New Package SCM Request
=======================
Package Name: ovirt-guest-agent 
Short Description: The oVirt Guest Agent
Owners: evilissimo
Branches: f17 f18 f19
InitialCC: sochotni
Comment 8 Jon Ciesla 2013-02-07 13:29:52 EST
Git done (by process-git-requests).
Comment 9 Fedora Update System 2013-02-08 02:54:38 EST
ovirt-guest-agent-1.0.6-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ovirt-guest-agent-1.0.6-4.fc17
Comment 10 Fedora Update System 2013-02-08 02:55:06 EST
ovirt-guest-agent-1.0.6-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ovirt-guest-agent-1.0.6-4.fc18
Comment 11 Fedora Update System 2013-02-09 06:24:36 EST
ovirt-guest-agent-1.0.6-4.fc18 has been pushed to the Fedora 18 testing repository.
Comment 12 Fedora Update System 2013-02-14 10:29:48 EST
ovirt-guest-agent-1.0.6-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ovirt-guest-agent-1.0.6-5.fc17
Comment 13 Fedora Update System 2013-02-14 10:30:12 EST
ovirt-guest-agent-1.0.6-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ovirt-guest-agent-1.0.6-5.fc18
Comment 14 Fedora Update System 2013-02-24 03:34:50 EST
ovirt-guest-agent-1.0.6-5.fc17 has been pushed to the Fedora 17 stable repository.
Comment 15 Fedora Update System 2013-02-24 03:51:59 EST
ovirt-guest-agent-1.0.6-5.fc18 has been pushed to the Fedora 18 stable repository.
Comment 16 Vinzenz Feenstra [evilissimo] 2013-07-11 05:48:58 EDT
Package Change Request
======================
Package Name: ovirt-guest-agent
New Branches: el5 el6
Owners: evilissimo
Comment 17 Jon Ciesla 2013-07-11 06:44:56 EDT
Git done (by process-git-requests).
Comment 18 Vinzenz Feenstra [evilissimo] 2014-05-19 07:37:32 EDT
Package Change Request
======================
Package Name: ovirt-guest-agent
New Branches: el7
Owners: evilissimo
Comment 19 Jon Ciesla 2014-05-19 08:01:35 EDT
Git done (by process-git-requests).

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