[Bug 520832] Review Request: quotatool - Disk utility for managing user quotas

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


Tom "spot" Callaway <tcallawa at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |tcallawa at redhat.com
         AssignedTo|nobody at fedoraproject.org    |tcallawa at redhat.com
               Flag|                            |fedora-review?




--- Comment #13 from Tom "spot" Callaway <tcallawa at redhat.com>  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 at 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.