[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 #14 from Steve Jenkins <steve at stevejenkins.com> 2011-08-22 20:46:46 EDT ---
(In reply to comment #13)
> c) explicitly patch the initfile from the upstream tarball using Patch0:
> %{name}-2.6.2-initscript.patch, and a %patch0 -p1  later.

Yep. Even better. I looked at the keygen logic in the sshd init file and did
something very similar (albeit a tad bit simpler, since there are fewer cases
to handle).

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

You're an excellent mentor, Matt. Shoving me in the right direction without
spoon-feeding everything, and politely telling me to RTFM where appropriate. :)
As a non-programmer, I imagine I need more hand-holding than most, so I
appreciate your patience. Thank you.

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

Fixed.

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

Ok, changed back - but I think I may just do a patch of the existing INSTALL
file instead. Seems simpler.

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

Again - me being guilty of just leaving stuff there that was in the original
spec file in the upstream's contrib. I removed these two files from %files -n
libopendkim-devel:

%{_libdir}/*.a
%{_libdir}/*.la

And then explicitly removed them in %install with:

rm -r %{buildroot}%{_libdir}/*.a
rm -r %{buildroot}%{_libdir}/*.la

to avoid build errors saying I had unpackaged files left in the buildroot. If
there's a better way to do this, please let me know.

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

Hmm... looks like it's working now! Some of the other cleanup we did must have
allowed it to "just work."

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

Fixed.

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

Fixed and fixed.

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

Fixed! I am totally on board with nits. :)

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

Fixed. Big "DUH" on that one. :)

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

Nice. Clean and efficient is good. Fixed.

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

Sigh... users. :) Fixed.

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

Good point. Fixed.

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

Fixed, although I'm not sure if I put the new Requires in the right place. The
wiki wasn't very specific.

>You need to also
> condrestart the service, yes?  See the scriptlets link above for examples.

As a matter of fact, I do! :) Fixed.

Question: I'd really like to move my monster conf file echo into a patch
against of the sample conf files that's included in the upstream source.
However, I like that I can use variables in the spec file, but the samples have
hard-coded paths. Which approach is preferable?

Updated spec file and SRPM in:

http://packages.stevejenkins.com/opendkim/2.4.2/working/

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