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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 #17 from Steve Jenkins <steve@xxxxxxxxxxxxxxxx> 2011-08-23 01:18:49 EDT ---
(In reply to comment #16)
> Excellent.  Much progress.

Thx - I'm learning gobs.

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

Ah - I meant to put the Requires(postun): initscripts in there last time. It's
in there now.

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

Good to know! I think I'll keep it as-is for now.


> %configure, you don't need CPPFLAGS anymore.

Check. Fixed.

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

I doubled up with an attr line. Fixed now.


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

Added.

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

Upstream source's "native" OS is BSD, so I don't know how many of those
warnings are by virtue of the fact that the upstream developer doesn't have
access to a RH box for building. He is, however, very responsive to feedback,
but me not being a programmer, I'd have a hard time deciphering what warnings
are worth mentioning to him. I'll forward him a build.log and he can take a
peek, or if there are any biggies that you note, please let me know and I'll
pass them up. 

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

My .rpmmacros is:

%_topdir %(echo $HOME)/rpmbuild

%_smp_mflags %( \
    [ -z "$RPM_BUILD_NCPUS" ] \\\
        && RPM_BUILD_NCPUS="`/usr/bin/getconf _NPROCESSORS_ONLN`"; \\\
    if [ "$RPM_BUILD_NCPUS" -gt 16 ]; then \\\
        echo "-j16"; \\\
    elif [ "$RPM_BUILD_NCPUS" -gt 3 ]; then \\\
        echo "-j$RPM_BUILD_NCPUS"; \\\
    else \\\
        echo "-j3"; \\\
    fi )

%__arch_install_post \
    [ "%{buildarch}" = "noarch" ] || QA_CHECK_RPATHS=1 ; \
    case "${QA_CHECK_RPATHS:-}" in [1yY]*) /usr/lib/rpm/check-rpaths ;; esac \
    /usr/lib/rpm/check-buildroot

and on this latest spec file, I added %{?_smp_mflags} to the make line, and it
seemed to build fine. Although, if that's not a proper test of what you're
asking about, let me know and I'll take another stab at testing it.

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

Most recent changes are now at:

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.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review


[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]