Bug 253649 - Review Request: vinagre - VNC client for the GNOME desktop
Summary: Review Request: vinagre - VNC client for the GNOME desktop
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ray Strode [halfline]
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-08-20 22:57 UTC by Bastien Nocera
Modified: 2007-11-30 22:12 UTC (History)
2 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2007-08-22 16:07:36 UTC
rstrode: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Bastien Nocera 2007-08-20 22:57:17 UTC
Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-1.src.rpm
Description: Vinagre is a VNC client for the GNOME desktop environment.

It uses gtk-vnc. Upstream is at:
http://www.gnome.org/~jwendell/vinagre/

(Note that 0.2 doesn't appear there, because I asked Jonh to do a release for me, so I could get to upload this package before feature freeze).

Comment 1 Ray Strode [halfline] 2007-08-21 15:14:23 UTC
When building in mock I get:


checking for XML::Parser... configure: error: XML::Parser perl module is
required for intltool

I've seen that problem before although I don't remember the details.  Is it that
the packaged intltool requires XML::Parser?  I'm guessing either BuildRequires
the parser or BuildRequires intltool and re-intltoolize will fix it.

Overall looks very good.  Minor nits that don't matter:

- rm -rf $RPM_BUILD_ROOT/%{_datadir}/doc/vinagre/  has an extra / in it (between
ROOT and %{_datadir}
- what docs are in the above anyway?  omf files? Are they worth shipping?
- vendor isn't required anymore for desktop-file-install, just ditch it.
- There are a handful or so Application categories, normally when tossing the
Application category, you pick one of them to replace it with (Maybe use Network?)
- you have %{_datadir}/%{name}/* in the file list but the package should
probably own the %{_datadir}/%{name} dir too.

Comment 2 Bastien Nocera 2007-08-21 15:23:21 UTC
(In reply to comment #1)
> When building in mock I get:
> 
> 
> checking for XML::Parser... configure: error: XML::Parser perl module is
> required for intltool
> 
> I've seen that problem before although I don't remember the details.  Is it that
> the packaged intltool requires XML::Parser?  I'm guessing either BuildRequires
> the parser or BuildRequires intltool and re-intltoolize will fix it.

added perl-XML-Parser as a BR

> Overall looks very good.  Minor nits that don't matter:
> 
> - rm -rf $RPM_BUILD_ROOT/%{_datadir}/doc/vinagre/  has an extra / in it (between
> ROOT and %{_datadir}

Fixed.

> - what docs are in the above anyway?  omf files? Are they worth shipping?

No, they're the usual AUTHORS, README, etc., but the maintainer thought it was
good to install them by himself. I'm removing those, and installing in the usual
versioned docs dir in %files

> - vendor isn't required anymore for desktop-file-install, just ditch it.

Done.

> - There are a handful or so Application categories, normally when tossing the
> Application category, you pick one of them to replace it with (Maybe use Network?)

It's already in Network, so appears in the same menu as TSClient

> - you have %{_datadir}/%{name}/* in the file list but the package should
> probably own the %{_datadir}/%{name} dir too.

Fixed, we want the whole dir.

Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-2.src.rpm

Comment 3 Ray Strode [halfline] 2007-08-21 15:38:21 UTC
change that added BuildRequires to

BuildRequires: perl(XML::Parser)

and add one for libglade2-devel


Comment 4 Bastien Nocera 2007-08-21 15:42:24 UTC
My mistake, fixed here:
Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-3.src.rpm

Comment 5 Ray Strode [halfline] 2007-08-21 16:02:25 UTC
looks like avahi-ui-devel is missing a Requires: avahi-glib-devel which is
breaking mock too, but that's not a problem with this package.

Comment 6 Bastien Nocera 2007-08-21 16:10:36 UTC
Filed as bug 253734

Worked around in:
Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-4.src.rpm

Comment 7 Bastien Nocera 2007-08-21 16:12:49 UTC
More missing deps, this time it's gettext and co.

Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-5.src.rpm

Comment 8 Ray Strode [halfline] 2007-08-21 16:30:10 UTC
Also need BuildRequires: desktop-file-utils

Comment 9 Bastien Nocera 2007-08-21 16:35:02 UTC
And missing desktop-file-utils:
Spec URL: http://people.redhat.com/bnocera/vinagre/vinagre.spec
SRPM URL: http://people.redhat.com/bnocera/vinagre/vinagre-0.2-6.src.rpm

Comment 10 Ray Strode [halfline] 2007-08-21 16:47:13 UTC
looks good.

Comment 11 Bastien Nocera 2007-08-21 17:15:48 UTC
Thanks Ray!

New Package CVS Request
=======================
Package Name: vinagre
Short Description: VNC client for the GNOME desktop
Owners: hadess
Branches: devel
InitialCC: bnocera@redhat.com
Commits by cvsextras:yes

Comment 12 Kevin Fenzi 2007-08-21 23:33:28 UTC
cvs done.

Comment 13 Bastien Nocera 2007-08-22 16:07:36 UTC
Uploaded to rawhide.


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