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

[bugzilla at redhat.com]

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


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

--- Comment #24 from Steven Dake <sdake at redhat.com> 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

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