[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 #5 from Steve Jenkins <steve at stevejenkins.com> 2011-08-22 11:32:33 EDT ---
(In reply to comment #4)
> * 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.

Fixed. That (like a lot of the issues you mentioned) are legacy issues from the
initial spec file in the source package from which I started.


> * Drop the Prefix: line

Fixed


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

It was what I was trying prior to discovering that %{?dist} would work, which
is what I'm using now. So, fixed.


> * Source0 and 1 need URLs to the upstream tarballs

Fixed - as long as it's permissible to link to my own version of the
readme.tar.gz, which includes a README, and (based on other feedback below)
will also contain my version of the conf file.

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

Fixed. I was using option #3 from:

http://fedoraproject.org/wiki/Archive:PackagingDrafts/BuildRoot

but now I'm using option #1.

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

Fixed. Doesn't bother me at the end. :)

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

Without this ifarch section and the LIBTOOL definition lower down, I was unable
to get this to build on EL5, EL6, or Fedora x84_64. Let me know if you'd like
me to remove it so you can see the precise Koji output when it attempts.

> * Don't use %makeinstall if it can be avoided (and it can always be avoided
> with upstream patching). 

Fixed.

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

Fixed. What do you think about simply including the config file in my readme
tarball? Or does that make things less "transparent?"

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

Fixed.

> * 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}/

Fixed. Yes, that looks MUCH cleaner.

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

Not fixed (yet). I need to do some more reading to figure out how to do this.

I've fixed most of the issues in this file and am researching that last one.

The updated spec file is the in the same location, but when I run rpmlint on
the updated one, I get these three errors:

opendkim.spec: E: specfile-error error: Package already exists: %package
debuginfo 
opendkim.spec: E: specfile-error 
opendkim.spec: E: specfile-error error: query of specfile opendkim.spec failed,
can't parse

I can't figure out what's causing them. Anything glaring to you in the updated
specfile that my rookie eyes are missing?

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