[Bug 731898] Review Request: opendkim - DKIM library and milter

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

--- Comment #16 from Matt Domsch <matt_domsch at dell.com> 2011-08-22 23:35:27 EDT ---
Excellent.  Much progress.

You asked about the proper place for Requires(preun) and (post).  Where they
are is fine.  You also need Requires(postun): initscripts   to handle
/sbin/service for the condrestart.  If it happened that chkconfig and service
were invoked from a subpackage, not from the main package, you would put these
Requires: instead in the %package section for each subpackage.  That isn't the
case here, but should help explain why it's OK to be where they're at.


Re the config file, one thing you could do is leave the pointer to the example
config as a comment at the top as you have, and then leave all the comments out
of the file you generate and install.  That'd make it shorter.  OTOH, the
variables that are being used for substitution very likely don't change from
distro version to version.  %{_sysconfdir} and %{_localstatedir} are the only
interesting ones for functionality, and they haven't changed in years, so
hard-coding those in an file isn't such a problem.  The comments reference
%{_docdir} (again, no worries), and explicitly %{name}-%{version}, for which it
would be nice to have substitution on the version.  However you want to handle
it is fine.

%configure, you don't need CPPFLAGS anymore.

rpmbuild threw a warning that /var/run/opendkim was included twice.

it wouldn't hurt to have an 'exit 0' at the end of %preun and %post, just to be
safe.  The others I think can't fail with a non-zero exit code.

upstream source sure does throw a lot of warnings. :-(

Does it build properly if using make -j?
http://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make
if not, a comment would be good.

These are all trivial to fix.  I suppose I should do a formal review next...

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