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@xxxxxxxxxx> 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@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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review