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=520832 Tom "spot" Callaway <tcallawa@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |tcallawa@xxxxxxxxxx AssignedTo|nobody@xxxxxxxxxxxxxxxxx |tcallawa@xxxxxxxxxx Flag| |fedora-review? --- Comment #13 from Tom "spot" Callaway <tcallawa@xxxxxxxxxx> 2009-09-15 10:17:27 EDT --- A few minor issues of note: * When you make an updated package, it is useful to post updated links to the new SRPM in the review bug. (I found your -3 SRPM, but it might have been overlooked by a different reviewer) * You should try to use %{name} and %{version} in the Source0: line, this helps ensure that as you update the package, you don't accidentally build it against older source. * While technically correct to not explicitly list the patch level when applying Patch0, I generally recommend that packagers do so, especially to make it clear to someone who might inherit this package in the future. Basically, you'd change: %patch0 to %patch0 -p0 * I also generally recommend that packagers generate patches at patch level 1. If you use a suffix when making changes (e.g. Makefile.OLD), then it is simple to use the "gendiff" tool to generate a patch: gendiff quotatool-1.4.10 .BAD * It is also useful to use a relevant suffix when applying the patch in the spec file: %patch0 -p0 -b .destdir This can come in handy when trying to regenerate patches for future updates. * Also, patch naming is useful. You named your patch "Makefile.patch", I would recommend something like: quotatool-1.4.10-DESTDIR.patch * You should go ahead and send patches upstream whenever appropriate. In this case, enabling DESTDIR support is useful for all distributions, and harmless if it is not set, so it is appropriate to send it upstream. You can do this by emailing it to their mailing list, or opening a bug in their bug tracker. You should also put a link to wherever you sent the patch to upstream as a comment in the spec file: # Sent upstream: # http://foo.com/bar/bug12345 * One very minor point: In Fedora 10+, it is no longer necessary to call: rm -rf $RPM_BUILD_ROOT immediately after %install. RPM now does this for you as the first step of %install. * You should get in the habit of running rpmlint against the SRPM and binary RPMs. For example, when I ran: [spot@pterodactyl ~]$ rpmlint /home/spot/rpmbuild/SRPMS/quotatool-1.4.10-3.fc12.src.rpm /home/spot/rpmbuild/RPMS/x86_64/quotatool-1.4.10-3.fc12.x86_64.rpm /home/spot/rpmbuild/RPMS/x86_64/quotatool-debuginfo-1.4.10-3.fc12.x86_64.rpm quotatool.src:48: W: macro-in-%changelog doc quotatool.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12) 3 packages and 0 specfiles checked; 0 errors, 2 warnings. Both of these are minor errors, but easily corrected. (You should use %% to escape out any macros used in changelog entries) Please show me an updated SRPM that incorporates fixes for the above items, and I will finish the review and sponsor you. -- 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review