[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 #4 from Matt Domsch <matt_domsch at dell.com> 2011-08-21 23:58:37 EDT ---
This is not a thorough review, but some things that jumped out at me reading
the spec file.

* Drop the "All rights reserved." text.  If you're asserting separate copyright
license for the spec file itself (which I would prefer not) then explicitly
state the license here.  Presumably you mean for the spec file to be licensed
under the same license as the rest of the package (BSD), so make that explicit.

* Drop the Prefix: line

* What is OSshort ?  On Fedora 14, that resolves to nil; where is it coming
from?

* Source0 and 1 need URLs to the upstream tarballs

* There are better forms for BuildRoot (though it's ignored on F15+)

* I find it unusual for %changelog to come so high in the file; I'm used to it
being at the end.  I guess it works where it is though.

* libtool in the oldest distro noted here (EL5) is v2.2, so I would be
surprised if you need your own copy ever, much less the ifarch x86_64 test and
the other tests around it during %build.

* Don't use %makeinstall if it can be avoided (and it can always be avoided
with 
 upstream patching). 
http://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

* you're massive echoed config file works because your config doesn't happen to
have single quotes in it.  Better to use a shell here document, e.g.

cat > opendkim.conf << EOF
big long config file
EOF


* %post creates /var/run/opendkim and /var/spool/opendkim and /etc/opendkim but
they are not owned by the package in %files.  Better to create those in
%install with the desired permissions, and own them in %files with permissions
as necessary.

* consider creating the opendkim group first, then creating the user with that
group, rather than fixing it up afterwards.  Users and groups should be created
in %pre, rather than %post, so that they are available to the system when file
permissions are set in %files. e.g.:

%pre
getent group mirrormanager >/dev/null || groupadd -r mirrormanager
getent passwd mirrormanager >/dev/null || \
  useradd -r -g mirrormanager -d /var/lib/mirrormanager -s /sbin/nologin \
  -c "MirrorManager" mirrormanager
exit 0

%files
%defattr(-,root,root,-)
%{_datadir}/%{name}
%attr(-,mirrormanager,mirrormanager) %dir %{_localstatedir}/lib/%{name}/

* consider moving key generation to startup of the service, rather than during
RPM install time.  This is what openssh does, for example.  It's quite possible
that there is insufficient entropy available at RPM install time to generate
keys, which will then cause the install to hang.

*

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