[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 #13 from Matt Domsch <matt_domsch at dell.com> 2011-08-22 15:35:32 EDT ---
c) explicitly patch the initfile from the upstream tarball using Patch0:
%{name}-2.6.2-initscript.patch, and a %patch0 -p1  later.

The spec is getting much better, thanks for the quick turnaround.  A few more
items to note:

Summary: doesn't tell me much at present.  How about:
Summary: DomainKeys Identified Mail (DKIM) Signature milter and library

As the readme tarball is personal, rather than upstream, no need for a full URL
on it then.

Does the -devel package need to include static libraries for a good reason?
http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2
simply don't package the *.a and *.la files.

See if you can get away without %defining LIBTOOL.  It's not clear to me when
that define gets evaluated - it may well be when outside the buildroot, which
isn't what you really want.

The bit to include kerberos on the include path.  That should come from
pkgconfig instead, yes?  On F14, /usr/lib64/pkgconfig/openssl.pc doesn't pull
it in, though I see on EL5 it does.  Can you Require: pkgconfig, and have
upstream make use of it, instead of the current directory existence test?

The %install test to clear the buildroot doesn't need to be tested - you've
explicitly set the buildroot above.  Guidelines say simply to:
%install
rm -rf %{buildroot}

Likewise for %clean.

You can remove the spaces among all the mkdirs.  (trivial nit I know...)

Your %pre calls exit 0 before calling usermod.  Simply add the -G mail flag to
the useradd command, and delete the usermod line all together.

You can simplify the invocations of ldconfig as noted here:
http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

The long echo in %post will likely never be seen by a user - PackageKit,
anaconda, and the like won't display it.  Consider removing it.

in %preun, why are you deleting the socket file and then the directory?  Now
that the dir is owned by the package, it'll get deleted as the package is
uninstalled.  Besides, the app itself should be deleting the .sock file when it
no longer is listening for connections on it.

Look again at the preun and post scriptlets, compare against
http://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets
in particular, if you have Requires: chkconfig, then you don't have to test for
it, and you can also ignore the LSB stuff all together.  You need to also
condrestart the service, yes?  See the scriptlets link above for examples.

Thanks,
Matt

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