Bug 634760

Hi,

I am not still member of packaging group, however I hope that you find my casual review useful:

1. You use localstatedir /var/amavis

Why? AFAIK amavisd-new in fedora doesn't use that directory:

repoquery -l amavisd-new | grep "var\/amavis" |wc -l
0

2. BuildRoot: and clean section are not needed anymore.

3. Beware you have some issue about initscript file, run rpmlint, you should not enable a service by default,  take a look at https://fedoraproject.org/wiki/Packaging/SysVInitScript. Also you will find interesting http://fedoraproject.org/wiki/Packaging:Tmpfiles.d and http://fedoraproject.org/wiki/Features/var-run-tmpfs.

4. Also this is no an error, but be aware that you're not using dist tag: http://fedoraproject.org/wiki/DistTag


Hope that helps.

Thanks for the comments. RHEL uses /var/amavis, so I've put in a test for rhel vs fedora for that.

Regarding builroot and the clean section, what's the recommended way to handle that when building for RHEL? Are those two going away entirely, or just superfluous in fedora?

(In reply to comment #2)
> Thanks for the comments. RHEL uses /var/amavis, so I've put in a test for rhel
> vs fedora for that.

Can you post a link to your updated .spec and .srpm?

> Regarding builroot and the clean section, what's the recommended way to handle
> that when building for RHEL? Are those two going away entirely, or just
> superfluous in fedora?

You can just leave these in; they don't hurt anything on Fedora. Once RHEL 5 goes EOL you can remove them.

Did we ever get an updated package?  All I see is the same release 2 package that was originally posted.

I guess I'll close this soon if there's no response.

Hey folks. I changed jobs a few months back, but I'm still interested in getting this package into Fedora.

I'll try to get an updated package online in the next few days.

Hi Andy, since I know you a bit from the OpenAFS community, I'd be willing to sponsor you into the packager group in Fedora.

New SRPM:

http://www.personal.psu.edu/~atc135/rpm/amavisd-milter/amavisd-milter-1.5.0-3.src.rpm

and the corresponding specfile:

http://www.personal.psu.edu/~atc135/rpm/amavisd-milter/amavisd-milter.spec

Comments are appreciated.

Package Review
Generated by fedora-review 0.1.2
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines. Yes, confirmed BSD.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture. Yes, F-15 i686.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST No %config files under /usr.
[x]: MUST Package does not generates any conflict.
[!]: MUST Package obeys FHS, except libexecdir and /usr/target.
     See below discussion about /var/amavis.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/ktdreyer/fedora-scm/amavisd-milter/634760/amavisd-milter-1.5.0.tar.gz :
  MD5SUM this package     : 2c9f601012164d14a0c2815a9e0928fe
  MD5SUM upstream package : 2c9f601012164d14a0c2815a9e0928fe

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[!]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: SHOULD Package functions as described.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.

==== Remaining Issues: ==== 
1. Please include the "LICENSE" file in your %doc line.

2. I'm wondering where the "/var/amavis" line comes from, because amavisd-new in both EPEL 5 and EPEL 6 use /var/spool/amavisd. Could this be a holdover from EPEL 4? From what I can see, I think this needs to be /var/spool/amavisd unconditionally.  

==== Nice-to-haves: ====
1. Most packages in Fedora use dist tags, and I highly recommend them.
   For example, you would change the "Release" line to be:

Release: 3%{dist}

2. It would be nice to have a systemd unit file, but it's not required.

3. It's "better" to use %global rather than %define with amavisdir.
   Note: %define amavisdir /var/amavis %define amavisdir /var/spool/amavisd
   %define _localstatedir %{amavisdir}

Overall looks good. If you take care of these things, I'll approve the package and sponsor you.

> ==== Remaining Issues: ==== 
> 1. Please include the "LICENSE" file in your %doc line.

fixed in -4
 
> 2. I'm wondering where the "/var/amavis" line comes from, because
> amavisd-new in both EPEL 5 and EPEL 6 use /var/spool/amavisd. Could this be
> a holdover from EPEL 4? From what I can see, I think this needs to be
> /var/spool/amavisd unconditionally.  

I was wondering that myself. Digging in a bit further, it looks like the RPMforge packaging of amavisd uses /var/amavis, while EPEL uses /var/spool/amavisd. I've removed that logic completely in -4.
 
> ==== Nice-to-haves: ====
> 1. Most packages in Fedora use dist tags, and I highly recommend them.
>    For example, you would change the "Release" line to be:

Yep, I usually use dist tags. Added in -4.

> 3. It's "better" to use %global rather than %define with amavisdir.
>    Note: %define amavisdir /var/amavis %define amavisdir /var/spool/amavisd
>    %define _localstatedir %{amavisdir}

Fixed in -4

New specfile and srpm are here:
http://www.personal.psu.edu/~atc135/rpm/amavisd-milter/amavisd-milter-1.5.0-4.el6.src.rpm
http://www.personal.psu.edu/~atc135/rpm/amavisd-milter/amavisd-milter.spec

--andy

Hi Andy,

(In reply to comment #9)
> > 3. It's "better" to use %global rather than %define with amavisdir.
> >    Note: %define amavisdir /var/amavis %define amavisdir /var/spool/amavisd
> >    %define _localstatedir %{amavisdir}
> 
> Fixed in -4

Sorry I wasn't clear here. %global is the preferred option for %define in almost all cases. And it's perferable to not mix %global and %define.

There's a discussion about my sponsorship at https://fedorahosted.org/packager-sponsors/ticket/1, and when cwickert looked at this review he suggested that you preserve the timestamps when installing files. You can do this with the "-p" flag to the "install" command.

I'm going to wait to see how my sponsorship request shakes out, if you're ok waiting a few more days?

It looks like systemd unit files are actually required for all new packages: https://fedoraproject.org/wiki/Packaging:Systemd

So amavisd-milter will need to provide a systemd unit file for inclusion in Fedora (sorry I missied this in my comment #8 above.)

Hi Andy, I was going through my older bugs and ran across this one. I wanted to see if you're still interested in getting this into Fedora?

Closing due long inactivity. Feel free to reopen if you want to continue.