Bug 226307

I would be happy to review this package. Look for a full review in a bit. 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (IBM Public License)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
24f3a076a2a1af0ca8dcb9bac3f145fa  postfix-2.3.6.tar.gz
24f3a076a2a1af0ca8dcb9bac3f145fa  postfix-2.3.6.tar.gz.1
See below - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
See below - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
See below - Should have subpackages require base package with fully versioned
depend.
See below - Should have dist tag
See below - Should package latest version
18 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Suggest: add a %{?dist} to release? Makes keeping several branches in sync much
easier.

2. You should use the approved buildroot:

      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

3. 2.3.7 is out. Perhaps upgrade to that?

4. On mysql and pgsql support. Could both of those be enabled in subpackages?
ie, postfix-pgsql, postfix-mysql? They can be quite usefull, and I don't know
of any reason to not include them.

5. Shouldn't pflogsumm be it's own package? It has a separate upstream source.
Was there a reason to include it in this package?
Also, it has no dependency on postfix, but it installs a doc file into the
postfix docdir, meaning if postfix wasn't installed and this package was installed,
then removed, you will be left with a lingering postfix doc dir with nothing in it.
If it was separate, it could also be noarch.
6. Our pal rpmlint is doing quite a bit of talking on this package.

a)

W: postfix prereq-use /sbin/chkconfig, /sbin/service, sh-utils
W: postfix prereq-use fileutils, textutils,
W: postfix prereq-use /usr/sbin/alternatives
W: postfix prereq-use %{_sbindir}/groupadd, %{_sbindir}/useradd

The use of PreReq is deprecated. In the majority of cases, a plain Requires
is enough and the right thing to do. Sometimes Requires(pre), Requires(post),
Requires(preun) and/or Requires(postun) can also be used instead of PreReq.

Take a look at:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

b)

W: postfix unversioned-explicit-provides MTA
W: postfix unversioned-explicit-provides smtpd
W: postfix unversioned-explicit-provides smtpdaemon
W: postfix unversioned-explicit-provides /usr/bin/newaliases
W: postfix unversioned-explicit-provides /usr/sbin/sendmail
W: postfix unversioned-explicit-provides /usr/bin/mailq
W: postfix unversioned-explicit-provides /usr/bin/rmail
E: postfix hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib
E: postfix hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib
E: postfix hardcoded-library-path in /usr/lib/sendmail.postfix
E: postfix hardcoded-library-path in /usr/lib/sendmail.postfix

These are due to the alternatives setup. I think this can be ignored.

c)
E: postfix use-of-RPM_SOURCE_DIR

Suggest: replace %{_sourcedir} with $SOURCE1 ?

d)

W: postfix macro-in-%changelog _docdir

Suggest: change macro to use %% in the changelog instead of %

e)
W: postfix mixed-use-of-spaces-and-tabs (spaces: line 27, tab: line 315)

Suggest: fix to all tabs or all spaces?

f)
E: postfix only-non-binary-in-usr-lib

This is the pfloggsum subpackage. If it was split out to it's own package
it could be noarch.

g)
W: postfix conffile-without-noreplace-flag /etc/rc.d/init.d/postfix
E: postfix executable-marked-as-config-file /etc/rc.d/init.d/postfix

Suggest: should not mark that init file as conf?

h) 
E: postfix file-in-usr-marked-as-conffile /usr/lib64/sasl/smtpd.conf
E: postfix file-in-usr-marked-as-conffile /usr/lib64/sasl2/smtpd.conf

Might be a bug in cyrus-sasl putting it's config files there, but
not postfix's fault. 

i)
W: postfix service-default-enabled /etc/rc.d/init.d/postfix

Suggest: Do no enable by default in the init file. 

j) All these can be ignored I think. Please examine them and
confirm that the permissions and uids are correct:

E: postfix non-standard-uid /var/spool/postfix/trace postfix
E: postfix non-standard-dir-perm /var/spool/postfix/trace 0700
E: postfix non-standard-gid /usr/sbin/postqueue postdrop
E: postfix setgid-binary /usr/sbin/postqueue postdrop 02755
E: postfix non-standard-executable-perm /usr/sbin/postqueue 02755
E: postfix non-standard-uid /var/spool/postfix/defer postfix
E: postfix non-standard-dir-perm /var/spool/postfix/defer 0700
E: postfix non-standard-uid /var/spool/postfix/saved postfix
E: postfix non-standard-dir-perm /var/spool/postfix/saved 0700
W: postfix non-conffile-in-etc /etc/postfix/postfix-files
E: postfix non-standard-uid /var/spool/postfix/corrupt postfix
E: postfix non-standard-dir-perm /var/spool/postfix/corrupt 0700
E: postfix non-standard-uid /var/spool/postfix/deferred postfix
E: postfix non-standard-dir-perm /var/spool/postfix/deferred 0700
E: postfix non-standard-uid /var/spool/postfix/flush postfix
E: postfix non-standard-dir-perm /var/spool/postfix/flush 0700
E: postfix non-standard-uid /var/spool/postfix/public postfix
E: postfix non-standard-gid /var/spool/postfix/public postdrop
E: postfix non-standard-dir-perm /var/spool/postfix/public 0710
W: postfix non-conffile-in-etc /etc/postfix/TLS_LICENSE
E: postfix non-standard-gid /usr/sbin/postdrop postdrop
E: postfix setgid-binary /usr/sbin/postdrop postdrop 02755
E: postfix non-standard-executable-perm /usr/sbin/postdrop 02755
E: postfix non-standard-uid /var/spool/postfix/private postfix
E: postfix non-standard-dir-perm /var/spool/postfix/private 0700
W: postfix non-conffile-in-etc /etc/postfix/LICENSE
E: postfix non-standard-uid /var/spool/postfix/incoming postfix
E: postfix non-standard-dir-perm /var/spool/postfix/incoming 0700
E: postfix non-standard-uid /var/spool/postfix/bounce postfix
E: postfix non-standard-dir-perm /var/spool/postfix/bounce 0700
E: postfix non-standard-uid /var/spool/postfix/active postfix
E: postfix non-standard-dir-perm /var/spool/postfix/active 0700
W: postfix non-conffile-in-etc /etc/postfix/main.cf.default
E: postfix non-standard-uid /var/spool/postfix/hold postfix
E: postfix non-standard-dir-perm /var/spool/postfix/hold 0700
E: postfix non-standard-uid /var/spool/postfix/maildrop postfix
E: postfix non-standard-gid /var/spool/postfix/maildrop postdrop
E: postfix non-standard-dir-perm /var/spool/postfix/maildrop 0730

7. The dependencies on perl here all seem to be due to the qshape script.
Would it be worth splitting that script out as well?
That would remove:
perl(File::Find)
perl(Getopt::Std)
perl(IO::File)
perl(strict)
/usr/bin/perl
Requires from the postfix package.

8. 18 outstanding bugs. Several of them seem related directly to the packaging.
Perhaps address those as part of the review here?

9. The LDAP conditionals mention redhat 8.0. Can we now just drop that stuff
for f7/fc6/fc5?

10. Some unneeded Buildrequires:
BuildRequires: gawk, perl, sed, ed, db4-devel, pkgconfig, zlib-devel
See: http://www.fedoraproject.org/wiki/Packaging/Guidelines#Exceptions

Can remove perl, sed at least, and possibly gawk and ed.

11. In the pfloggsumm subpackage
Requires: perl-Date-Calc
is not needed. rpm picks up on that itself.

12. Why is the
umask 022
line there in %build and %install and several other sections?

13. Might use %{?_smp_mflags} in the make line to speed up build times?

Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back 
to me, and change the 'fedora-review' flag back to ? for me to take action. 

Resetting flags and such per the new offical review guidelines: 
https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html

Comments:

4) It is only possible to compile the suppoort into postfix. There is no dynamic
loading suppport in postfix upstream. Building sub packages, which replace
programs in the main package is not an option.

5) Yes, but the question is if we want to support pflogsum at all anymore.
f) Will be done in devel
h) Yes
j) These are correct

7) Nope, I do not think that it is worth plitting out. Perl is needed by other
base components.

8) Mostly done

Please have a look at postfix-2.4.3-2.fc7 in the F7 build tree.

postfix-2.4.3-2.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

1. ok. good. 

2. ok. good. 

3. ok. good. 

4. Ugh. I do see that now. I was hoping it could be done cleanly somehow. ;( 
The debian dynamic patches sound like they don't work... 
how bad is it just to enable postgres/mysql and eat the dependencies. 
Of course this is not a blocker, it's up to you, but it would be very nice to
have something here. 

5. True. I was just pointing out that it could be totally seperate... 
if you want to drop it entirely I don't think that would be bad. 
Of course you would need proper Obsoletes/Provides to make sure the upgrade path
from it was ok. 

6. ok. looks good. 

7. ok. Up to you. 

8. ok. looks good. 

9. ok. looks good. 

10. ok. looks good. 

11. ok. looks good. 

12. ok. looks good. 

13. ok. looks good. 

All the blockers I see are solved, so this package is APPROVED.