Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: spice-vdagent - Agent for Spice guests
https://bugzilla.redhat.com/show_bug.cgi?id=648549
Summary: Review Request: spice-vdagent - Agent for Spice guests Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: hdegoede@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Target Release: ---
Spec URL: http://people.fedoraproject.org/~jwrdegoede/spice-vdagent.spec SRPM URL: http://people.fedoraproject.org/~jwrdegoede/spice-vdagent-0.6.3-1.fc14.src.r... Description: Spice agent for Linux guests offering the following features:
Features: * Client mouse mode (no need to grab mouse by client, no mouse lag) this is handled by the daemon by feeding mouse events into the kernel via uinput. This will only work if the active X-session is running a spice-vdagent process so that its resolution can be determined. * Automatic adjustment of the X-session resolution to the client resolution * Support of copy and paste (text and images) between the active X-session and the client
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=648549
Germán Racca gracca@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |gracca@gmail.com
--- Comment #1 from Germán Racca gracca@gmail.com 2010-11-05 15:38:37 EDT --- Hi Hans:
While trying to compile I got the following error:
+ make 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic' cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/spice-1 -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -D_GNU_SOURCE -c -o vdagentd.o vdagentd.c vdagentd.c: In function 'send_capabilities': vdagentd.c:87:21: error: 'VD_AGENT_CAP_CLIPBOARD_BY_DEMAND' undeclared (first use in this function) vdagentd.c:87:21: note: each undeclared identifier is reported only once for each function it appears in vdagentd.c: In function 'do_client_clipboard': vdagentd.c:167:10: error: 'VD_AGENT_CLIPBOARD_GRAB' undeclared (first use in this function) vdagentd.c:173:10: error: 'VD_AGENT_CLIPBOARD_REQUEST' undeclared (first use in this function) vdagentd.c:174:9: error: 'VDAgentClipboardRequest' undeclared (first use in this function) vdagentd.c:174:34: error: 'req' undeclared (first use in this function) vdagentd.c:174:66: error: expected expression before ')' token vdagentd.c:187:10: error: 'VD_AGENT_CLIPBOARD_RELEASE' undeclared (first use in this function) vdagentd.c: In function 'virtio_port_read_complete': vdagentd.c:241:10: error: 'VD_AGENT_CLIPBOARD_GRAB' undeclared (first use in this function) vdagentd.c:242:10: error: 'VD_AGENT_CLIPBOARD_REQUEST' undeclared (first use in this function) vdagentd.c:244:10: error: 'VD_AGENT_CLIPBOARD_RELEASE' undeclared (first use in this function) vdagentd.c:247:31: error: 'VDAgentClipboardGrab' undeclared (first use in this function) vdagentd.c:249:31: error: 'VDAgentClipboardRequest' undeclared (first use in this function) vdagentd.c: In function 'do_agent_clipboard': vdagentd.c:275:12: error: 'VD_AGENT_CAP_CLIPBOARD_BY_DEMAND' undeclared (first use in this function) vdagentd.c:295:35: error: 'VD_AGENT_CLIPBOARD_GRAB' undeclared (first use in this function) vdagentd.c:300:9: error: 'VDAgentClipboardRequest' undeclared (first use in this function) vdagentd.c:300:33: error: expected ';' before 'req' vdagentd.c:302:35: error: 'VD_AGENT_CLIPBOARD_REQUEST' undeclared (first use in this function) vdagentd.c:303:47: error: 'req' undeclared (first use in this function) vdagentd.c:327:35: error: 'VD_AGENT_CLIPBOARD_RELEASE' undeclared (first use in this function) vdagentd.c:338:21: error: 'VD_AGENT_CLIPBOARD_NONE' undeclared (first use in this function) vdagentd.c: In function 'update_active_session_connection': vdagentd.c:427:35: error: 'VD_AGENT_CLIPBOARD_RELEASE' undeclared (first use in this function) vdagentd.c: In function 'main': vdagentd.c:684:35: error: 'VD_AGENT_CLIPBOARD_RELEASE' undeclared (first use in this function) make: *** [vdagentd.o] Error 1
Please give me some hints so I can successfully build the package and start the review :)
Regards, Germán.
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=648549
--- Comment #2 from Hans de Goede hdegoede@redhat.com 2010-11-05 15:55:18 EDT --- Hi,
Oops I forgot about that. You need the spice-protocol from updates-testing (which moved to regular updates earlier today, so should show up their soon). It might be easiest to get it directly from koji: http://koji.fedoraproject.org/koji/buildinfo?buildID=200958
Regards,
Hans
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=648549
Germán Racca gracca@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
--- Comment #3 from Germán Racca gracca@gmail.com 2010-11-08 16:24:52 EST --- Hi Hans,
here follows the review:
Package Review ==============
Key: - = N/A x = Check ! = Problem ? = Not evaluated
=== REQUIRED ITEMS === [x] Package is named according to the Package Naming Guidelines. [x] Spec file name must match the base package %{name}, in the format %{name}.spec. [x] Package successfully compiles and builds into binary rpms on at least one supported architecture. Tested on: x86_64 [!] Rpmlint output: spice-vdagent.src: W: spelling-error %description -l en_US uinput -> input, u input, sinciput spice-vdagent.x86_64: W: spelling-error %description -l en_US uinput -> input, u input, sinciput spice-vdagent.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/spice-vdagent.desktop spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagentd spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagent spice-vdagent.x86_64: W: service-default-enabled /etc/rc.d/init.d/spice-vdagentd spice-vdagent.x86_64: E: malformed-line-in-lsb-comment-block # daemon enhances the spice guest user experience with client spice-vdagent.x86_64: E: malformed-line-in-lsb-comment-block # mouse mode, guest <-> client copy and paste support and more. spice-vdagent.x86_64: W: incoherent-subsys /etc/rc.d/init.d/spice-vdagentd $prog spice-vdagent.x86_64: W: service-default-enabled /etc/rc.d/init.d/spice-vdagentd [x] Package is not relocatable. [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)) [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] License field in the package spec file matches the actual license. License type: GPLv3+ [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] Spec file is legible and written in American English. [x] Sources used to build the package matches the upstream source, as provided in the spec URL. MD5SUM this package : dca976a6a92744462e1aed101f4ea467 MD5SUM upstream package: dca976a6a92744462e1aed101f4ea467 [x] Package is not known to require ExcludeArch, OR: Arches excluded: Why: [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [-] The spec file handles locales properly. [x] ldconfig called in %post and %postun if required. [x] Package must own all directories that it creates. [-] Package requires other packages for directories it uses. [x] Package does not contain duplicates in %files. [x] Permissions on files are set properly. [x] Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [x] Package consistently uses macros. [-] Package contains code, or permissable content. [-] Large documentation files are in a -doc subpackage, if required. [x] Package uses nothing in %doc for runtime. [-] Header files in -devel subpackage, if present. [-] Static libraries in -devel subpackage, if present. [-] Package requires pkgconfig, if .pc files are present. [-] Development .so files in -devel subpackage, if present. [-] Fully versioned dependency in subpackages, if present. [x] Package does not contain any libtool archives (.la). [-] Package contains a properly installed %{name}.desktop file if it is a GUI application. [-] Package does not own files or directories owned by other packages.
=== SUGGESTED ITEMS === [x] Latest version is packaged. [x] Package does not include license text files separate from upstream. [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [!] Reviewer should test that the package builds in mock. Tested on: x86_64 [?] Package should compile and build into binary rpms on all supported architectures. Tested on: [?] Package functions as described. [x] Scriptlets must be sane, if used. [-] The placement of pkgconfig(.pc) files are correct. [-] File based requires are sane.
=== Issues === 1. Most important for me in this review is the rpmlint output, as it has some errors and warnings. The first 4 warnings are acceptable for me. However, as you know this is my first review, and I've never seen these errors/warnings before. If you didn't see them before, please try to correct them. If they are acceptable, please explain why. 2. This is a should, but I was not able to successfully compile the package in mock because of the updates-testing requirement for one of the BRs.
Package is nice, but please clarify the marked issues.
Regards, Germán.
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=648549
--- Comment #4 from Hans de Goede hdegoede@redhat.com 2010-11-08 16:47:04 EST --- Hi,
Thanks for the review!
spice-vdagent.src: W: spelling-error %description -l en_US uinput ->input, u input, sinciput spice-vdagent.x86_64: W: spelling-error %description -l en_US uinput -> input, u input, sinciput
<sigh>, rpmlint is stupid wrt spelling errors, uinput is the correct term these can safely be ignored.
spice-vdagent.x86_64: W: non-conffile-in-etc/etc/xdg/autostart/spice-vdagent.desktop
And another favorite useless rpmlint warnings some files under /etc are not meant for user modification and thus not marked %config, files under /etc/xdg is one of the groups of files which should not be %config even though being under /etc.
spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagentd spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagent
Not really an issue as they are not meant to be directly started from the cmdline, let alone started with options on the cmdline. They are automatically started as service, resp. by .desktop files -> no need for manpage.
spice-vdagent.x86_64: W: service-default-enabled/etc/rc.d/init.d/spice-vdagentd
Yes, and intentionally so. The initscript is a no-op (exits immediately) when not running under spice.
spice-vdagent.x86_64: E: malformed-line-in-lsb-comment-block # daemonenhances the spice guest user experience with client spice-vdagent.x86_64: E: malformed-line-in-lsb-comment-block # mouse mode, guest <-> client copy and paste support and more.
Oh rpmlint found a real issue. I'll upload a fixed package.
spice-vdagent.x86_64: W: incoherent-subsys /etc/rc.d/init.d/spice-vdagentd
<sigh> and another useless rpmlint check
=== Issues ===
- Most important for me in this review is the rpmlint output, as it has some
errors and warnings. The first 4 warnings are acceptable for me. However, as you know this is my first review, and I've never seen these errors/warnings before. If you didn't see them before, please try to correct them. If they are acceptable, please explain why.
See above, please don't take it personal if I sound a bit grumpy above. Although rpmlint is a very useful tool (as shown by the one real issue did find) sometimes it makes me <sigh> a lot because some of its checks are useless (like the terrible newly added spelling checking failure).
But still it did find one real issue I'll upload a new version with this fixed soon.
- This is a should, but I was not able to successfully compile the package in
mock because of the updates-testing requirement for one of the BRs.
The needed spice-protocol package is in stable updates now :)
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=648549
--- Comment #5 from Hans de Goede hdegoede@redhat.com 2010-11-08 16:52:11 EST --- Here is a new version with the lsb header issue fixed: Spec URL: http://people.fedoraproject.org/~jwrdegoede/spice-vdagent.spec SRPM URL: http://people.fedoraproject.org/~jwrdegoede/spice-vdagent-0.6.3-2.fc14.src.r...
Once more thanks for the review!
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=648549
--- Comment #6 from Germán Racca gracca@gmail.com 2010-11-09 10:58:35 EST --- Hi Hans!
Thanks very much for your explanations about rpmlint output. As I said above, there were some warnings/errors I didn't see before so I needed to see your comments about them. Now it is all clear for me...and you didn't sound grumpy, so don't worry about that :)
Some items that remains to check now that you, as upstream, released a modified version. Some things like checksums didn't change because you only patched the original code.
=== REQUIRED ITEMS === [x] Package successfully compiles and builds into binary rpms on at least one supported architec ture. Tested on: x86_64 [x] Rpmlint output: spice-vdagent.src: W: spelling-error %description -l en_US uinput -> input, u input, sinciput spice-vdagent.x86_64: W: spelling-error %description -l en_US uinput -> input, u input, sinciput spice-vdagent.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/spice-vdagent.desktop spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagentd spice-vdagent.x86_64: W: no-manual-page-for-binary spice-vdagent spice-vdagent.x86_64: W: service-default-enabled /etc/rc.d/init.d/spice-vdagentd spice-vdagent.x86_64: W: incoherent-subsys /etc/rc.d/init.d/spice-vdagentd $prog spice-vdagent.x86_64: W: service-default-enabled /etc/rc.d/init.d/spice-vdagentd
=== SUGGESTED ITEMS === [x] Reviewer should test that the package builds in mock. Tested on: x86_64
=== Final Notes === 1. Now the real problem rpmlint detected is fixed and all those warnings were explained by you in a previous comment. 2. Now I was able to successfully build the package in mock.
Therefore, your package is...
================ *** APPROVED *** ================
I have tried to be as much clear as I could, but my English is poor, so if something remained obscure please let me know. Also, I would like to hear from you how I have been doing in my first review, so I can continue reviewing other packages also.
All the best, Germán.
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=648549
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #7 from Hans de Goede hdegoede@redhat.com 2010-11-09 15:21:40 EST --- Hi,
Thanks for the review. You forgot to set the fedora-review flag to +, can you please do that?
Regards,
Hans
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=648549
Germán Racca gracca@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #8 from Germán Racca gracca@gmail.com 2010-11-09 16:30:40 EST --- I'm sorry Hans. Now it's ok.
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=648549
--- Comment #9 from Hans de Goede hdegoede@redhat.com 2010-11-09 16:37:50 EST --- New Package SCM Request ======================= Package Name: spice-vdagent Short Description: Agent for Spice guests Owners: jwrdegoede Branches: f14 InitialCC:
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=648549
--- Comment #10 from Kevin Fenzi kevin@tummy.com 2010-11-09 22:10:07 EST --- Git done (by process-git-requests).
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=648549
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
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=648549
--- Comment #11 from Fedora Update System updates@fedoraproject.org 2010-11-10 03:38:59 EST --- spice-vdagent-0.6.3-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/spice-vdagent-0.6.3-2.fc14
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=648549
--- Comment #12 from Hans de Goede hdegoede@redhat.com 2010-11-10 03:40:28 EST --- Hi,
(In reply to comment #8)
I'm sorry Hans. Now it's ok.
No problem. Doing things and making mistakes is the only way to learn :)
Let me know if you want to swap another review.
Regards,
Hans
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=648549
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #13 from Fedora Update System updates@fedoraproject.org 2010-11-10 16:44:01 EST --- spice-vdagent-0.6.3-2.fc14 has been pushed to the Fedora 14 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update spice-vdagent'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/spice-vdagent-0.6.3-2.fc14
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=648549
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kraxel@redhat.com
--- Comment #14 from Hans de Goede hdegoede@redhat.com 2010-11-20 05:59:15 EST --- *** Bug 613654 has been marked as a duplicate of this bug. ***
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=648549
--- Comment #15 from Fedora Update System updates@fedoraproject.org 2010-11-25 20:08:22 EST --- spice-vdagent-0.6.3-2.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
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=648549
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |spice-vdagent-0.6.3-2.fc14 Resolution| |ERRATA Last Closed| |2010-11-25 20:08:27
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=648549
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs+ |fedora-cvs?
--- Comment #16 from Hans de Goede hdegoede@redhat.com 2012-01-06 07:34:21 EST --- Package Change Request ====================== Package Name: spice-vdagent New Branches: el5 Owners: jwrdegoede
I would like to introduce spice-vdagent as an EPEL-package for EL5.
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=648549
--- Comment #17 from Jon Ciesla limburgher@gmail.com 2012-01-06 09:20:42 EST --- Git done (by process-git-requests).
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla