[Bug 657403] Review Request: spice-gtk - A GTK widget for SPICE clients

[bugzilla at redhat.com]

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

Hans de Goede <hdegoede at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review?

--- Comment #6 from Hans de Goede <hdegoede at redhat.com> 2011-01-05 05:54:17 EST ---
Full review done:

Good:
- rpmlint checks return:
spice-gtk-python.x86_64: W: no-documentation
spice-gtk-tools.x86_64: W: no-documentation
spice-gtk-tools.x86_64: W: no-manual-page-for-binary snappy
spice-gtk-tools.x86_64: W: no-manual-page-for-binary spicy
6 packages and 0 specfiles checked; 0 errors, 4 warnings.
These can all be ignored.
- package meets naming guidelines
- package meets packaging guidelines
- license (LGPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

Some minor nitpicks / should fix items:
- The Group tag for the main package should be: System Environment/Libraries
- "Requires:      %{name} = %{version}" should be dropped
- The %description for the main package talks about a gtk client and libraries,
  but the gtk client is part of the tools package
- Likewise the %description for tools does not mention the gtk client
- The "Requires: %{name} = %{version}" does not specify the release in the
  package, when subpackages depend on other packages in the same SRPM the
  requires should specify the full NEVR (name epoch version release) like the
  Requires in the devel sub package
- The prefered form for the defattr is:
%defattr(-,root,root,-)
  rather then:
%defattr(-, root, root)
- There is no need to specify dir owner ship and the files inside it if
  you want the package to own the dir and all files, for example this:
%dir %{_includedir}/spice-client-glib/
%{_includedir}/spice-client-glib/*.h
  Can be written simply as:
%{_includedir}/spice-client-glib
  Likewise for spice-client-gtk, also you could consider using
  wildcards, condensing the %files devel to:
%{_libdir}/libspice-client-g*.so
%{_includedir}/spice-client-g*
%{_libdir}/pkgconfig/spice-client-g*.pc  
%{_datadir}/gir-1.0/SpiceClientG*-1.0.gir
%doc %{_datadir}/gtk-doc/html/*

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.