Gal Hammer has posted comments on this change.
Change subject: Updated spec file to follow packaging guide lines (BZ#772608)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File ovirt-guest-agent.spec
Line 1:
Line 2: %global release_version 2
Line 3:
Line 4: %global _moduledir /%{_lib}/security
Line 5: %global _kdmrc /etc/kde/kdm/kdmrc
Is there a benefit from using %global over %define in this case?
Line 6:
Line 7: Name: ovirt-guest-agent
Line 8: Version: 1.0.5
Line 9: Release: %{release_version}%{?dist}
Line 6:
Line 7: Name: ovirt-guest-agent
Line 8: Version: 1.0.5
Line 9: Release: %{release_version}%{?dist}
Line 10: Summary: The oVirt Guest Agent
I don't like the "The" prefix. I know the guide line say that the name should
start with a capital letter but that's the project name, oVirt.
Line 11: Group: Applications/System
Line 12: License: GPLv2+
Line 13: URL: http://gerrit.ovirt.org/p/ovirt-guest-agent.git
Line 14: Source0: http://ghammer.fedorapeople.org/%{name}-%{version}.tar.bz2
Line 15: BuildRequires: libtool
Line 16: BuildRequires: pam-devel
Line 17: BuildRequires: python2-devel
Line 18: %if 0%{?fedora} >= 18
Line 19: BuildRequires: systemd
Is this needed for the build process?
Line 20: %else
Line 21: BuildRequires: systemd-units
Line 22: %endif
Line 23:
Line 31: Requires: kernel > 2.6.18-238.5.0
Line 32: Requires: usermode
Line 33: Provides: %{name} = %{version}-%{release}
Line 34: %if 0%{?fc16}
Line 35: Requires: selinux-policy >= 3.10.0-77
Don't do that. You are now forcing the user to use selinux.
Line 36: %endif
Line 37: %if 0%{?fedora} >= 17
Line 38: Requires: selinux-policy >= 3.10.0-89
Line 39: %endif
Line 69: run-time data from within the guest itself. The agent also accepts
Line 70: control commands to be run executed within the OS (like: shutdown and
Line 71: restart).
Line 72:
Line 73: %description service
I don't like that either. It is nice that you solved the noarch problem, but
changing the package name to ovirt-guest-agent-service makes the name too long.
Line 74: This is the oVirt management agent running inside the guest. The agent
Line 75: interfaces with the oVirt manager, supplying heart-beat info as well as
Line 76: run-time data from within the guest itself. The agent also accepts
Line 77: control commands to be run executed within the OS (like: shutdown and
Line 90
Line 91
Line 92
Line 93
Line 94
Why did you remove this comment?
Line 109: # Update timestamps on Python files in order to avoid differences
between
Line 110: # .pyc/.pyo files.
Line 111: touch -r %{SOURCE0} %{buildroot}%{_datadir}/ovirt-guest-agent/*.py
Line 112:
Line 113: # Create needed folders
Isn't this a bit useless? A comment that answer the question "what" (what does
the command do?) and not the "why" (why do you "touch" the *.py files?) is
usually redundant.
Line 114: mkdir -p %{buildroot}%{_localstatedir}/log/ovirt-guest-agent
Line 115: mkdir -p %{buildroot}%{_localstatedir}/lock/subsys/ovirt-guest-agent
Line 116:
Line 117: ln -sf /usr/bin/consolehelper
%{buildroot}%{_datadir}/ovirt-guest-agent/ovirt-locksession
Line 177: %config(noreplace) %{_sysconfdir}/security/console.apps/ovirt-shutdown
Line 178: %config(noreplace)
%{_sysconfdir}/security/console.apps/ovirt-hibernate
Line 179: %config(noreplace) %{_sysconfdir}/pam.d/ovirt-locksession
Line 180: %config(noreplace) %{_sysconfdir}/pam.d/ovirt-shutdown
Line 181: %config(noreplace) %{_sysconfdir}/pam.d/ovirt-hibernate
I'm sure not if you should use the "noreplace" directive on all files. The
comment below, regarding break-something might also apply to most of these
files.
Line 182:
Line 183: #This is intentionally NOT 'noreplace' it's a required udev rule. If
this is
Line 184: #modified by an user this actually might break it.
Line 185: %config %attr (644,root,root)
%{_sysconfdir}/udev/rules.d/55-ovirt-guest-agent.rules
Line 180: %config(noreplace) %{_sysconfdir}/pam.d/ovirt-shutdown
Line 181: %config(noreplace) %{_sysconfdir}/pam.d/ovirt-hibernate
Line 182:
Line 183: #This is intentionally NOT 'noreplace' it's a required udev rule. If
this is
Line 184: #modified by an user this actually might break it.
A space after the # sign in a comment.
Line 185: %config %attr (644,root,root)
%{_sysconfdir}/udev/rules.d/55-ovirt-guest-agent.rules
Line 186:
Line 187: %attr (755,root,root)
%{_datadir}/ovirt-guest-agent/ovirt-guest-agent.py*
Line 188:
Line 209: %doc AUTHORS COPYING NEWS README
Line 210:
Line 211: %files pam-module
Line 212: %{_moduledir}/pam_ovirt_cred.so
Line 213: %doc AUTHORS COPYING NEWS README
Is it necessary? Duplicate these file in all sub packages?
Line 214:
Line 215: %files gdm-plugin
Line 216: %config(noreplace) %{_sysconfdir}/pam.d/gdm-ovirtcred
Line 217: %{_datadir}/icons/hicolor/*/*/*.png
Line 226: %doc AUTHORS COPYING NEWS README
Line 227:
Line 228: %changelog
Line 229: * Fri Oct 19 2012 Vinzenz Feenstra <vfeen...@redhat.com> - 1.0.5-2
Line 230: - introduced ovirt-guest-agent-service noarch package which provides
White space at EOL.
Line 231: ovirt-guest-agent and avoids duplication of the same package content
Line 232: - fixed various rpmlint errors and warnings
Line 233: - added AUTHORS, COPYING, NEWS and README to all subpackages
Line 234: - changed conflicts to be requires, which is more accurate
--
To view, visit http://gerrit.ovirt.org/8715
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec350b4027bffff8dff73db7986020fdc48261cd
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-guest-agent
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Gal Hammer <gham...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
