Bug 648549 - Review Request: spice-vdagent - Agent for Spice guests
Review Request: spice-vdagent - Agent for Spice guests
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 613654 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-01 11:37 EDT by Hans de Goede
Modified: 2012-01-06 09:20 EST (History)
4 users (show)

See Also:
Fixed In Version: spice-vdagent-0.6.3-2.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-11-25 20:08:27 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
gracca: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2010-11-01 11:37:58 EDT
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.rpm
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
Comment 1 Germán Racca 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.
Comment 2 Hans de Goede 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
Comment 3 Germán Racca 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.
Comment 4 Hans de Goede 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 # 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.

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 ===
> 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. 

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.

> 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.

The needed spice-protocol package is in stable updates now :)
Comment 5 Hans de Goede 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.rpm

Once more thanks for the review!
Comment 6 Germán Racca 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.
Comment 7 Hans de Goede 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
Comment 8 Germán Racca 2010-11-09 16:30:40 EST
I'm sorry Hans. Now it's ok.
Comment 9 Hans de Goede 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:
Comment 10 Kevin Fenzi 2010-11-09 22:10:07 EST
Git done (by process-git-requests).
Comment 11 Fedora Update System 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
Comment 12 Hans de Goede 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
Comment 13 Fedora Update System 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
Comment 14 Hans de Goede 2010-11-20 05:59:15 EST
*** Bug 613654 has been marked as a duplicate of this bug. ***
Comment 15 Fedora Update System 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.
Comment 16 Hans de Goede 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.
Comment 17 Jon Ciesla 2012-01-06 09:20:42 EST
Git done (by process-git-requests).

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