This is my first package and I am seeking a sponsor. Spec URL: http://packages.stevejenkins.com/opendkim/2.4.2/sources/opendkim.spec SRPM URL: http://packages.stevejenkins.com/opendkim/2.4.2/sources/opendkim-2.4.2-2.src.rpm Description: OpenDKIM provides an open source library that implements the DKIM service, plus a milter-based filter application that can plug in to any milter-aware MTA, including sendmail, Postfix, or any other MTA that supports the milter protocol.
I've run the spec file and resulting RPMs through rpmlint, and successfully built against f15, f16, f17, 5e-epel and 6e-epel in Koji.
For anyone wanting to check Koji output, my FAS account name is stevej.
I've sponsored Steve.
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. *
(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?
(In reply to comment #5) > > * 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. I've removed the ifarch, so it should force the use of the local LIBTOOL instead of the one provided by opendkim. But I can't test to see if it works yet because I'm still having that same rpmlint error. I'll try to figure out what it is, but if anyone catches it before I do, please share. :)
Found the cause of the rpmlint error - mentioning %install in the changelog.
(In reply to comment #4) > * %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. I'm doing this wrong, since I'm getting build errors, but I can't figure out what's the right way. I have in %install: test -d %{_localstatedir}/spool/%{name} || mkdir -p %{_localstatedir}/spool/%{name} test -d %{_localstatedir}/run/%{name} || mkdir -p %{_localstatedir}/run/%{name} test -d %{_sysconfdir}/%{name} || mkdir %{_sysconfdir}/%{name} and then in %files: %{_localstatedir}/spool/%{name} %{_localstatedir}/run/%{name} %attr(-,%{name},%{name}) %{_localstatedir}/run/%{name} %attr(750,%{name},%{name}) %{_sysconfdir}/%{name}/* and I'm getting: RPM build errors: File not found: /home/fedora/rpmbuild/BUILDROOT/opendkim-2.4.2-3.fc15.i386/var/spool/opendkim File not found: /home/fedora/rpmbuild/BUILDROOT/opendkim-2.4.2-3.fc15.i386/var/run/opendkim File not found: /home/fedora/rpmbuild/BUILDROOT/opendkim-2.4.2-3.fc15.i386/var/run/opendkim File not found by glob: /home/fedora/rpmbuild/BUILDROOT/opendkim-2.4.2-3.fc15.i386/etc/opendkim/*
(In reply to comment #8) > I'm doing this wrong, since I'm getting build errors, but I can't figure out > what's the right way. I have in %install: > > test -d %{_localstatedir}/spool/%{name} || mkdir -p > %{_localstatedir}/spool/%{name} Got it figured out. Building now, and going to test in Koji, then I'll upload a new SRPM and spec.
Do you need all the test -d's ? mkdir -p will create (or not) if necessary.
I guess not... the mkdir -p should be sufficient. How's this look: http://packages.stevejenkins.com/opendkim/2.4.2/working/opendkim.spec
I've taken the key generation out of the spec file, what's the preferred method of including it in the init file? Currently, my spec file pulls the init file from the source tarball. Is it preferable to: a) include my own new init file and then try to push it upstream on a later release, or b) Have the RPM do some Perl magic on the existing init file before copying it to /etc/init.d ?
c) explicitly patch the initfile from the upstream tarball using Patch0: %{name}-2.6.2-initscript.patch, and a %patch0 -p1 later. The spec is getting much better, thanks for the quick turnaround. A few more items to note: Summary: doesn't tell me much at present. How about: Summary: DomainKeys Identified Mail (DKIM) Signature milter and library As the readme tarball is personal, rather than upstream, no need for a full URL on it then. Does the -devel package need to include static libraries for a good reason? http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2 simply don't package the *.a and *.la files. See if you can get away without %defining LIBTOOL. It's not clear to me when that define gets evaluated - it may well be when outside the buildroot, which isn't what you really want. The bit to include kerberos on the include path. That should come from pkgconfig instead, yes? On F14, /usr/lib64/pkgconfig/openssl.pc doesn't pull it in, though I see on EL5 it does. Can you Require: pkgconfig, and have upstream make use of it, instead of the current directory existence test? The %install test to clear the buildroot doesn't need to be tested - you've explicitly set the buildroot above. Guidelines say simply to: %install rm -rf %{buildroot} Likewise for %clean. You can remove the spaces among all the mkdirs. (trivial nit I know...) Your %pre calls exit 0 before calling usermod. Simply add the -G mail flag to the useradd command, and delete the usermod line all together. You can simplify the invocations of ldconfig as noted here: http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries The long echo in %post will likely never be seen by a user - PackageKit, anaconda, and the like won't display it. Consider removing it. in %preun, why are you deleting the socket file and then the directory? Now that the dir is owned by the package, it'll get deleted as the package is uninstalled. Besides, the app itself should be deleting the .sock file when it no longer is listening for connections on it. Look again at the preun and post scriptlets, compare against http://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets in particular, if you have Requires: chkconfig, then you don't have to test for it, and you can also ignore the LSB stuff all together. You need to also condrestart the service, yes? See the scriptlets link above for examples. Thanks, Matt
(In reply to comment #13) > c) explicitly patch the initfile from the upstream tarball using Patch0: > %{name}-2.6.2-initscript.patch, and a %patch0 -p1 later. Yep. Even better. I looked at the keygen logic in the sshd init file and did something very similar (albeit a tad bit simpler, since there are fewer cases to handle). >The spec is getting much better, thanks for the quick turnaround. >A few more items to note: You're an excellent mentor, Matt. Shoving me in the right direction without spoon-feeding everything, and politely telling me to RTFM where appropriate. :) As a non-programmer, I imagine I need more hand-holding than most, so I appreciate your patience. Thank you. > Summary: doesn't tell me much at present. How about: > Summary: DomainKeys Identified Mail (DKIM) Signature milter and library Fixed. > As the readme tarball is personal, rather than upstream, no need for a full URL > on it then. Ok, changed back - but I think I may just do a patch of the existing INSTALL file instead. Seems simpler. > Does the -devel package need to include static libraries for a good reason? > http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2 > simply don't package the *.a and *.la files. Again - me being guilty of just leaving stuff there that was in the original spec file in the upstream's contrib. I removed these two files from %files -n libopendkim-devel: %{_libdir}/*.a %{_libdir}/*.la And then explicitly removed them in %install with: rm -r %{buildroot}%{_libdir}/*.a rm -r %{buildroot}%{_libdir}/*.la to avoid build errors saying I had unpackaged files left in the buildroot. If there's a better way to do this, please let me know. > See if you can get away without %defining LIBTOOL. It's not clear to me when > that define gets evaluated - it may well be when outside the buildroot, which > isn't what you really want. Hmm... looks like it's working now! Some of the other cleanup we did must have allowed it to "just work." > The bit to include kerberos on the include path. That should come from > pkgconfig instead, yes? On F14, /usr/lib64/pkgconfig/openssl.pc doesn't pull > it in, though I see on EL5 it does. Can you Require: pkgconfig, and have > upstream make use of it, instead of the current directory existence test? Fixed. > The %install test to clear the buildroot doesn't need to be tested - you've > explicitly set the buildroot above. Guidelines say simply to: > %install > rm -rf %{buildroot} > > Likewise for %clean. Fixed and fixed. > You can remove the spaces among all the mkdirs. (trivial nit I know...) Fixed! I am totally on board with nits. :) > Your %pre calls exit 0 before calling usermod. Simply add the -G mail flag to > the useradd command, and delete the usermod line all together. Fixed. Big "DUH" on that one. :) > You can simplify the invocations of ldconfig as noted here: > http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries Nice. Clean and efficient is good. Fixed. > The long echo in %post will likely never be seen by a user - PackageKit, > anaconda, and the like won't display it. Consider removing it. Sigh... users. :) Fixed. > in %preun, why are you deleting the socket file and then the directory? Now > that the dir is owned by the package, it'll get deleted as the package is > uninstalled. Besides, the app itself should be deleting the .sock file when it > no longer is listening for connections on it. Good point. Fixed. > Look again at the preun and post scriptlets, compare against > http://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets > in particular, if you have Requires: chkconfig, then you don't have to test for > it, and you can also ignore the LSB stuff all together. Fixed, although I'm not sure if I put the new Requires in the right place. The wiki wasn't very specific. >You need to also > condrestart the service, yes? See the scriptlets link above for examples. As a matter of fact, I do! :) Fixed. Question: I'd really like to move my monster conf file echo into a patch against of the sample conf files that's included in the upstream source. However, I like that I can use variables in the spec file, but the samples have hard-coded paths. Which approach is preferable? Updated spec file and SRPM in: http://packages.stevejenkins.com/opendkim/2.4.2/working/
Most recent Koji results: Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=3293713 F16: http://koji.fedoraproject.org/koji/taskinfo?taskID=3293721 F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=3293724 6E-epel: http://koji.fedoraproject.org/koji/taskinfo?taskID=3293727 5E-epel: http://koji.fedoraproject.org/koji/taskinfo?taskID=3293731
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...
(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/
Oh, also - I got rid of the additional external source and opted for a patch of the source tarball's INSTALL file instead.
pkgconfig should be a BuildRequires, not a Requires. http://fedoraproject.org/wiki/Packaging:UsersAndGroups notes need to Requires(pre): shadow-utils also. This also notes that the user and group should not be removed on uninstall. (see the %preun section) I'll start the formal review next.
(In reply to comment #19) > pkgconfig should be a BuildRequires, not a Requires. That's what I get for staying up late. :) Fixed. > http://fedoraproject.org/wiki/Packaging:UsersAndGroups notes need to > Requires(pre): shadow-utils also. Added. > This also notes that the user and group should not be removed on uninstall. > (see the %preun section) Removed from %preun. > I'll start the formal review next. Party on! :) New SRPM and spec: http://packages.stevejenkins.com/opendkim/2.4.2/working/
Review: * rpmlint: see below * name: OK * spec file name matches: OK * packaging guidelines: OK * licensed: OK * license tag: change to "BSD and Sendmail" * license in %doc: OK * english spec: OK * legible spec: OK * sources match upstream: OK * builds on one arch: OK * exclusivearch?: OK * BuildRequires: OK (after fixing pkgconfig noted above) * locales: OK (N/A) * shared libs invoke ldconfig: OK * no system libs: OK * relocatable?: OK (N/A) * owns dirs: OK * no files more than once: OK * permissions: (re-check) * macros: OK * code or content: OK * large docs: OK (N/A) * docs don't affect runtime: OK * headers in -devel: OK * static libs in -static: OK (N/A) * .so in -devel: OK * -devel requires fully versioned base package: Needs to be fixed * No .la files: OK * desktop file: OK (N/A) * directory ownership: OK * UTF-8 filenames: OK $ rpmlint SPECS/opendkim.spec SRPMS/opendkim-2.4.2-3.fc14.src.rpm RPMS/x86_64/* SPECS/opendkim.spec:67: W: macro-in-comment %{_docdir} SPECS/opendkim.spec:67: W: macro-in-comment %{name} SPECS/opendkim.spec:67: W: macro-in-comment %{version} SPECS/opendkim.spec:67: W: macro-in-comment %{name} SPECS/opendkim.spec:75: W: macro-in-comment %{_docdir} SPECS/opendkim.spec:75: W: macro-in-comment %{name} SPECS/opendkim.spec:75: W: macro-in-comment %{version} SPECS/opendkim.spec:107: W: macro-in-comment %{_localstatedir} SPECS/opendkim.spec:107: W: macro-in-comment %{name} SPECS/opendkim.spec:128: W: macro-in-comment %{_sysconfdir} SPECS/opendkim.spec:128: W: macro-in-comment %{name} SPECS/opendkim.spec:133: W: macro-in-comment %{_sysconfdir} SPECS/opendkim.spec:133: W: macro-in-comment %{name} SPECS/opendkim.spec:137: W: macro-in-comment %{_sysconfdir} SPECS/opendkim.spec:137: W: macro-in-comment %{name} SPECS/opendkim.spec:140: W: macro-in-comment %{_sysconfdir} SPECS/opendkim.spec:140: W: macro-in-comment %{name} opendkim.src: W: spelling-error %description -l en_US sendmail -> send mail, send-mail, Sendai opendkim.src:67: W: macro-in-comment %{_docdir} opendkim.src:67: W: macro-in-comment %{name} opendkim.src:67: W: macro-in-comment %{version} opendkim.src:67: W: macro-in-comment %{name} opendkim.src:75: W: macro-in-comment %{_docdir} opendkim.src:75: W: macro-in-comment %{name} opendkim.src:75: W: macro-in-comment %{version} opendkim.src:107: W: macro-in-comment %{_localstatedir} opendkim.src:107: W: macro-in-comment %{name} opendkim.src:128: W: macro-in-comment %{_sysconfdir} opendkim.src:128: W: macro-in-comment %{name} opendkim.src:133: W: macro-in-comment %{_sysconfdir} opendkim.src:133: W: macro-in-comment %{name} opendkim.src:137: W: macro-in-comment %{_sysconfdir} opendkim.src:137: W: macro-in-comment %{name} opendkim.src:140: W: macro-in-comment %{_sysconfdir} opendkim.src:140: W: macro-in-comment %{name} libopendkim.x86_64: E: postin-without-ldconfig /usr/lib64/libopendkim.so.5.0.1 libopendkim.x86_64: E: non-empty-%post /sbin/ldconfig libopendkim-devel.x86_64: W: no-version-dependency-on libopendkim/libopendkim-libs/liblibopendkim 2.4.2 opendkim.x86_64: W: spelling-error %description -l en_US sendmail -> send mail, send-mail, Sendai opendkim.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/opendkim-genzone ['/usr/lib64'] opendkim.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/opendkim-stats ['/usr/lib64'] opendkim.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/opendkim ['/usr/lib64'] opendkim.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/opendkim-testkey ['/usr/lib64'] opendkim.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/opendkim-testadsp ['/usr/lib64'] opendkim.x86_64: W: non-standard-uid /etc/opendkim/keys opendkim opendkim.x86_64: W: non-standard-gid /etc/opendkim/keys opendkim opendkim.x86_64: W: non-standard-uid /var/run/opendkim opendkim opendkim.x86_64: W: non-standard-gid /var/run/opendkim opendkim opendkim.x86_64: W: dangerous-command-in-%preun userdel opendkim.x86_64: W: service-default-enabled /etc/rc.d/init.d/opendkim opendkim.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/opendkim opendkim-debuginfo.x86_64: E: non-standard-dir-perm /usr/lib/debug 0775L opendkim-debuginfo.x86_64: E: non-standard-dir-perm /usr/src/debug/opendkim-2.4.2 0775L 5 packages and 1 specfiles checked; 9 errors, 44 warnings. The above macro-in-comment bits can be solved using %%{_foo} if necessary in the comment sections. Ignore spelling error. postin-without-ldconfig and non-empty-post are odd; worth looking into, but they should be OK... no-version-dependency: caught above on review, needs fixing. rpath: ugh. Needs fixing. http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath non-standard-uid/gid: OK. dangerous command userdel: caught above, needs fixing. service-default-enabled: needs fixing by patching initscript missing-lsb-keyword: needs fixing by patching initscript non-standard-dir-perm: ignore, it's a bug in rpmlint. :-) File permissions show /usr/share/doc/*/convert_keylist.sh is marked executable. In %install, chmod it 0644. API documentation currently in /usr/share/doc/libopendkim-2.4.2/*.html should only be in the -devel subpackage, not in both. Fixing the above, then I'll check the review again. Thanks, Matt
Matt, the OpenDKIM package builds its own libtool (./libtool built from m4/libtool.m4) and uses it by default. This package-provided libtool attempts to auto-detect system library paths and ends up deciding it's /lib and /usr/lib. Thus anything that links to /lib64 and /usr/lib64 (which opendkim does) will end up using rpath to add to the dynamic loader search path. In testing we found that this was only an issue on x86_64. That was the reason for the %ifarch logic around the %define LIBTOOL macro (because it works properly on i386 arch). Since we added "BuildRequires: libtool" to the spec file and know that libtool will always be present for the build for all arches, the %ifarch then could be dropped and a simple: %define LIBTOOL="LIBTOOL=`which libtool`" will take care of finding /usr/bin/libtool, which has the rpaths hard coded to /lib64 and /usr/lib64 on x86_64. Then pass that to the Makefile by appending %LIBTOOL to %make and make install commands. If you do not pass that LIBTOOL override, it will use the ./libtool and screw up the rpaths. Without doing it this way, you get the rpath errors as above. Regards... Todd Lyons
(In reply to comment #21) > The above macro-in-comment bits can be solved using %%{_foo} if necessary in > the comment sections. Those are from the comments that the echoed config file, so I want those variables to be parsed. Is it OK to leave these warnings? > Ignore spelling error. I awlays do. > postin-without-ldconfig and non-empty-post are odd; worth looking into, but > they should be OK... You're right - from what I Googled on the subject, the postin-without-ldconfig rpmlint error was benign in most cases. However, Google returned absolutely ZERO results for "rpmlint 'non-empty-post'" or "rpmlint 'non-empty-%post'" Is rmplint complaining that the output of that %post wasn't empty, when it expected it to be? Also, my rpmlint output isn't showing these errors (on an i686 FC box). I'm running v1.2. Are you running a newer version? > no-version-dependency: caught above on review, needs fixing. Is this something that needs to be fixed in the spec file or the upstream? > rpath: ugh. Needs fixing. > http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath Mmmmyeeeahhh..... Ugh. This is what the ifarch funkiness in the original version of the spec file was for. Refer to Todd's comments above. I've added the LIBTOOL stuff back in so that it forces the use of the system's libtool instead of the one in the source. > non-standard-uid/gid: OK. > > dangerous command userdel: caught above, needs fixing. You must have grabbed the spec before I had a chance to upload that change based on your feedback earlier this morning. That *should* be fixed, as I removed the group and user delete. > service-default-enabled: needs fixing by patching initscript > missing-lsb-keyword: needs fixing by patching initscript Got rid of the missing-lsb-keyword, but the service-default-enabled is still hanging around. Can you give me a nudge toward what might be causing that? > non-standard-dir-perm: ignore, it's a bug in rpmlint. :-) Freebie! Yay! > File permissions show /usr/share/doc/*/convert_keylist.sh is marked executable. > In %install, chmod it 0644. Fixed. > API documentation currently in /usr/share/doc/libopendkim-2.4.2/*.html should > only be in the -devel subpackage, not in both. Looking in to this.
>(In reply to comment #21) > * -devel requires fully versioned base package: Needs to be fixed Steve just added that, %version-%release. > opendkim.x86_64: W: service-default-enabled /etc/rc.d/init.d/opendkim > > service-default-enabled: needs fixing by patching initscript So does this mean that a package cannot set itself to be started at boot? Currently it's set to: # chkconfig: 2345 41 61 and no Default-Start nor Default-Stop (he used to have it, it's not in his current incarnation, so now LSB and chkconfig settings differ).
Updated spec and SRPMs at: http://packages.stevejenkins.com/opendkim/2.4.2/working/ Please let me know how this one looks, Matt.
much improved. I see the current initscript patch still has: # chkconfig: - 41 59 (this is good) # description: OpenDKIM implements the DomainKeys Identified Mail (DKIM) # service and a milter-based filter application that can plug # in to any milter-aware MTA. # processname: opendkim # pidfile: /var/run/opendkim/opendkim.pid ### BEGIN INIT INFO # Provides: opendkim # Required-Start: opendkim # Required-Stop: opendkim # Default-Start: 2 3 4 5 # Default-Stop: 0 1 6 (this is wrong: delete default-start and -stop lines) Each of the subpackages needs a %doc that includes LICENSE and LICENSE.Sendmail. I know, it's annoying, but they can be installed separately. otherwise, rpmlint looks good now (I'm using Fedora 14 to build and run rpmlint). Everything else looks good. Sorry for the mess around libtool. What you've got now seems to be OK, no more rpaths either, yea! Fix these two, and I'll mark approved.
responding to above: Packages can set themselves to automatically start on boot, if they're strictly necessary. Most shouldn't though. https://fedoraproject.org/wiki/Packaging/SysVInitScript#Why_don.27t_we.... Macro expansion in comments as they're used in the config file generator here is fine. oh, delete the Required-Start and Required-stop lines. Certainly opendkim doesn't require itself to be started in order to start itself.
(In reply to comment #26) > much improved. I see the current initscript patch still has: > > # chkconfig: - 41 59 > > (this is good) Yay! > # Required-Start: opendkim > # Required-Stop: opendkim > # Default-Start: 2 3 4 5 > # Default-Stop: 0 1 6 > (this is wrong: delete default-start and -stop lines) Boo! I've deleted all four of these. > Each of the subpackages needs a %doc that includes LICENSE and > LICENSE.Sendmail. I know, it's annoying, but they can be installed separately. I added them in both with %doc. > otherwise, rpmlint looks good now (I'm using Fedora 14 to build and run > rpmlint). Everything else looks good. I've been testing on Fedora 15 i686, and Todd's been testing on CentOS 5.6 x86_86. Our rpmlints look equally clean, the RPMs build and install on both. > Sorry for the mess around libtool. What you've got now seems to be OK, no more > rpaths either, yea! We scratched our heads a bit, but yeah - I think this is probably the best way to deal with it. The developer is always keen for feedback, so perhaps we'll suggest that he not even try to build libtool if it's already in the path. > Fix these two, and I'll mark approved. New spec and SRPM at: http://packages.stevejenkins.com/opendkim/2.4.2/working/
A quick review of the SHOULDs during review indicates all is well also. Package looks great now. Thanks. (often times it's easiest to start a package from scratch rather than fix up whatever upstream happened to have done. Now that you've done all this cleanup, hopefully you can submit the spec and patches back upstream too.) APPROVED.
(In reply to comment #29) > A quick review of the SHOULDs during review indicates all is well also. That's what I was goin' for. :) > Package looks great now. Thanks. (often times it's easiest to start a package > from scratch rather than fix up whatever upstream happened to have done. True - but doing it this way also taught me a whole lot, which was one of the reasons I wanted to do this. > Now that you've done all this cleanup, hopefully you can submit the spec > and patches back upstream too.) Totally the plan, and luckily we have a very receptive upstream developer. > APPROVED. Alright! Thanks, Matt! I'm gonna fire up the docs so I can start learning about what to do on the next step. :)
New Package SCM Request ======================= Package Name: opendkim Short Description: DomainKeys Identified Mail (DKIM) Signature milter and library Owners: stevej Branches: f14 f15 f16 el5 el6 InitialCC:
Git done (by process-git-requests).
opendkim-2.4.2-3.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/opendkim-2.4.2-3.fc14
opendkim-2.4.2-3.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/opendkim-2.4.2-3.el6
opendkim-2.4.2-3.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/opendkim-2.4.2-3.el5
opendkim-2.4.2-3.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/opendkim-2.4.2-3.fc16
opendkim-2.4.2-3.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/opendkim-2.4.2-3.fc15
opendkim-2.4.2-3.fc16 has been pushed to the Fedora 16 testing repository.
opendkim-2.4.2-3.fc14 has been pushed to the Fedora 14 stable repository.
opendkim-2.4.2-3.fc15 has been pushed to the Fedora 15 stable repository.
opendkim-2.4.2-3.fc16 has been pushed to the Fedora 16 stable repository.
opendkim-2.4.2-3.el5 has been pushed to the Fedora EPEL 5 stable repository.
opendkim-2.4.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.