https://bugzilla.redhat.com/show_bug.cgi?id=921797 --- Comment #30 from Bojan Smojver <bojan@xxxxxxxxxxxxx> --- (In reply to Adam Williamson from comment #29) > I'm a packager, so I can review this formally. Mostly I endorse what Trever > has done so far in his unofficial review. Additional notes: > > 1. Is the dependency on postfix really necessary? The script can be called > on its own, and it's at least conceivable that something other than postfix > could call it to do something useful. I'm not sure the postfix dep achieves > anything sane. The /usr/libexec/postfix path is owned by postfix. So, to place the policyd-spf there, we do need to have it, I guess. > 2. The definition of python_sitelib is unnecessary and unwanted unless you > intend to build for EL-5. If you do intend to build for EL-5, you should at > least conditionalize it. > https://fedoraproject.org/wiki/Packaging:Python#Macros has a snippet you can > copy/paste. I am not personally interested in it, but I think someone here asked, so we may as well do it. I can do that. > 3. I think the guidelines are encouraging explicit specification of > python2-devel or python3-devel, rather than python-devel. OK, I guess we'll go with python2-devel then, given that this is more backwards compatible, right? > 4. You include the comment line "# Remove CFLAGS=... for noarch packages > (unneeded)" in the %build section, but then include the CFLAGS= definition? > This is a noarch package. It seems like you should drop that comment line, > and drop the "CFLAGS="$RPM_OPT_FLAGS"" from the next line. He, he... rpmdev-newspec artefact. Will kill. > 5. I'm not sure if it's valid to mark an *entire directory* as > %config(noreplace) . I don't think I've seen that before. I think: > > %dir %{_sysconfdir}/python-policyd-spf > %config(noreplace) %{_sysconfdir}/python-policyd-spf/policyd-spf.conf > > would be more conventional. I'm not actually sure what the effects of > marking a directory as noreplace would be, and Google isn't immediately > helpful. Yeah, we can do that. > Aside from those notes, the package looks good to me. If you can resolve the > above I'll mark it as approved. Thank you for reviewing. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=OufdkVBafN&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review