[Bug 703719] Review Request: spice-xpi - mozilla extension for spice client

[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=703719

--- Comment #3 from Hans de Goede <hdegoede at redhat.com> 2011-06-07 09:07:11 EDT ---
Hi,

Full review done:

Good:
=====
- package meets naming guidelines
- spec file legible, in am. english
- package compiles on devel (x86)
- no missing BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
===========
- rpmlint checks return:
[hans at shalem ~]$ rpmlint rpmbuild/SRPMS/spice-xpi-2.5-1.fc15.src.rpm
rpmbuild/RPMS/x86_64/spice-xpi-*
spice-xpi.src: W: invalid-url Source0: spice-xpi-2.5.tar.bz2
spice-xpi.x86_64: W: incoherent-version-in-changelog 2.5.1 ['2.5-1.fc15',
'2.5-1']
3 packages and 0 specfiles checked; 0 errors, 2 warnings.
 Both will need to be fixed:
 -now that we've an official tarbal up please replace Source0 with:
  Source0: http://spice-space.org/download/releases/%{name}-%{version}.tar.bz2
 -The version in the changelog should be 2.5-1 not 2.5.1
 -Note that you're supposed to bump the release field each time you make
  changes, even during review so the next changelog entry should have:
  2.5-2 (and Release: near the top should be 2)

- please drop the tarname and tarversion macro-s they are identical to Name
  resp Release and rpm automatically defines %{name} and %{version} for these.

- License: should be: "MPLv1.1 or GPLv2+ or LGPLv2+"

- please change the URL to http://spice-space.org

- please drop BuildRoot, it is not needed in recent Fedora (no in epel-6 or
  newer)

- likewise drop the "rm -rf $RPM_BUILD_ROOT" from %install and the entire
%clean
  section

- please drop the gcc-c++ BuildRequires, gcc-c++ is part of the default
buildroot, see: http://fedoraproject.org/wiki/PackagingGuidelines#BuildRequires
(and then exceptions list)

- please add a comment explaining why ExclusiveArch is used in this case simply
  add the following line above the ExclusiveArch line:
# https://bugzilla.redhat.com/show_bug.cgi?id=613529

- Please drop the following Requires:
  -log4cpp, this is a used library, rpm automatically generates dependencies
   for dynamically linked libraries
  -firefox, this plugin should be usable in other browsers too

- please drop the " -n %{tarname}-%{tarversion}" arguments to %setup,
  %setup uses " -n %{name}-%{version}" as default

- make dist has already generated configure and Makefile.in, etc. No need to
  redo that, please drop the following calls from %build:
aclocal
libtoolize --automake
autoheader
automake --add-missing
autoconf

- with these dropped the following BuildRequires can also be removed:
BuildRequires:  automake
BuildRequires:  libtool
BuildRequires:  autoconf

- the standard for %defattr used in Fedora is:
%defattr(-,root,root,-)

- it is convention to have the %doc line as the first line after %defattr in
  %files

Regards,

Hans

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