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 #16 from Matt Domsch <matt_domsch@xxxxxxxx> 2011-08-22 23:35:27 EDT --- Excellent. Much progress. 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. 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. %configure, you don't need CPPFLAGS anymore. rpmbuild threw a warning that /var/run/opendkim was included twice. 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. upstream source sure does throw a lot of warnings. :-( Does it build properly if using make -j? http://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make if not, a comment would be good. These are all trivial to fix. I suppose I should do a formal review next... -- 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