Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=889546 Bug ID: 889546 Summary: Review Request: ovirt-guest-agent - oVirt Guest Agent Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Reporter: vfeenstr@xxxxxxxxxx Flags: fedora-review? 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. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=tHj4zwQ9ib&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review