Bug 226407 - Merge Review: sendmail
Merge Review: sendmail
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:57 EST by Nobody's working on this, feel free to take it
Modified: 2010-08-17 09:39 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-17 08:35:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paul: fedora‑review+


Attachments (Terms of Use)
Patch addressing most rpmlint issues worth fixing (17.47 KB, patch)
2007-02-15 14:05 EST, Paul Howarth
no flags Details | Diff
Patch implementing most of the following review suggestions (39.69 KB, patch)
2007-02-20 06:58 EST, Paul Howarth
no flags Details | Diff
Patch fixing build issue in current rawhide (2.45 KB, patch)
2007-06-26 05:58 EDT, Paul Howarth
no flags Details | Diff
Patch silencing complaint about non-existent %{_libdir}/sasl2/Sendmail.conf (906 bytes, patch)
2010-06-11 09:19 EDT, Paul Howarth
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:57:46 EST
Fedora Merge Review: sendmail

http://cvs.fedora.redhat.com/viewcvs/devel/sendmail/
Initial Owner: twoerner@redhat.com
Comment 1 Paul Howarth 2007-02-14 13:21:02 EST
I'll take a look at this one; I'll have initial comments tomorrow
Comment 2 Paul Howarth 2007-02-15 14:04:01 EST
I've run out of time for going through the review checklist today, so here for
the time being are my thoughts on the rpmlint output:

$ rpmlint sendmail-8.14.0-1.src.rpm
W: sendmail summary-ended-with-dot A widely used Mail Transport Agent (MTA).
=> easy fix, remove dot
W: sendmail invalid-license Sendmail
=> whilst rpmlint doesn't include the Sendmail license in its list of approved
=> licenses, the FSF includes sendmail in its directory of free software
=> (http://directory.fsf.org/sendmail.html) so this is OK
W: sendmail no-url-tag
=> easy fix, add URL tag
W: sendmail unversioned-explicit-provides smtpdaemon
=> normal for virtual provides, ignorable
W: sendmail prereq-use /usr/sbin/alternatives
=> use of fine-grained dependencies is normally preferred, but in this case
== the requirement is needed for a triggerpostun clause (as well as the post
=> and postun scripts) and as far as I know there is no way of specifying
=> fine-grained dependencies for trigger scripts, so this is OK
W: sendmail unversioned-explicit-provides %{_sbindir}/sendmail
W: sendmail unversioned-explicit-provides %{_bindir}/mailq
W: sendmail unversioned-explicit-provides %{_bindir}/newaliases
W: sendmail unversioned-explicit-provides %{_bindir}/rmail
W: sendmail unversioned-explicit-provides %{_mandir}/man1/mailq.1.gz
W: sendmail unversioned-explicit-provides %{_mandir}/man1/newaliases.1.gz
W: sendmail unversioned-explicit-provides %{_mandir}/man5/aliases.5.gz
=> all OK as these files are provided in "alternatives" format
W: sendmail prereq-use chkconfig >= 1.3
=> OK, also needed for a trigger script
W: sendmail prereq-use /usr/sbin/useradd /bin/mktemp fileutils gawk sed sh-utils
=> these should be replaced with the appropriate Requires tags for each
=> scriptlet; "fileutils" should probably be "coreutils" these days
W: sendmail prereq-use fsl >= 1.2.0
=> simple "Requires" should do?
W: sendmail prereq-use openssl
=> not needed since rpm's auto-dependency finder picks up the library dep
W: sendmail prereq-use /usr/sbin/saslauthd, openssl
=> "Requires: /usr/sbin/saslauthd" is sufficient
W: sendmail prereq-use openldap, openssl
=> not needed
W: sendmail prereq-use mysql
=> not needed
E: sendmail hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/sendmail.sendmail
=> legitimate install of /usr/lib/sendmail for backward script compatibility
E: sendmail hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/libmilter.a
=> false positive, the %install script is removing the library incorrectly
=> installed there by the upstream build script
E: sendmail hardcoded-library-path in /usr/lib/sendmail.sendmail
E: sendmail hardcoded-library-path in /usr/lib/sendmail.sendmail
=> legitimate install of /usr/lib/sendmail for backward script compatibility
W: sendmail macro-in-%changelog preun
W: sendmail macro-in-%changelog triggerpostun
W: sendmail macro-in-%changelog post
=> easy fix, escape the macros in the changelog
W: sendmail mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 239)
=> easy fix, expand the tabs
=> the here document for /etc/mail/access could be split off into a separate
=> source file and retain its tabs

$ rpmlint sendmail-8.14.0-1.x86_64.rpm
W: sendmail summary-ended-with-dot A widely used Mail Transport Agent (MTA).
W: sendmail invalid-license Sendmail
W: sendmail no-url-tag
=> already covered
E: sendmail only-non-binary-in-usr-lib
=> rpmlint confused by a symlink to a binary elsewhere
W: sendmail conffile-without-noreplace-flag /etc/rc.d/init.d/sendmail
E: sendmail executable-marked-as-config-file /etc/rc.d/init.d/sendmail
=> initscript shouldn't be a conffile in the first place. I would argue that
=> any customizations should be possible by changing things in
=> /etc/sysconfig/sendmail. Failing that, use %config(noreplace)
E: sendmail file-in-usr-marked-as-conffile /usr/lib64/sasl2/Sendmail.conf
W: sendmail conffile-without-noreplace-flag /usr/lib64/sasl2/Sendmail.conf
=> This is where cyrus-sasl puts things, so not much choice about this
E: sendmail zero-length /etc/mail/mailertable
E: sendmail zero-length /etc/mail/domaintable
E: sendmail zero-length /etc/mail/virtusertable
=> these files intentionally left blank, though perhaps comment lines could
=> be put in them, which would shut rpmlint up
E: sendmail non-standard-executable-perm /usr/sbin/makemap 0555
E: sendmail non-standard-executable-perm /usr/sbin/smrsh 0555
=> should be installed 0755 for debuginfo extraction anyway
E: sendmail non-standard-gid /usr/sbin/sendmail.sendmail smmsp
E: sendmail setgid-binary /usr/sbin/sendmail.sendmail smmsp 02755
E: sendmail non-standard-executable-perm /usr/sbin/sendmail.sendmail 02755
E: sendmail non-standard-dir-perm /var/spool/mqueue 0700
E: sendmail non-standard-uid /var/spool/clientmqueue smmsp
E: sendmail non-standard-gid /var/spool/clientmqueue smmsp
E: sendmail non-standard-dir-perm /var/spool/clientmqueue 0770
=> non-standard but correct
W: sendmail dangling-relative-symlink /usr/bin/newaliases.sendmail
../../usr/sbin/sendmail
W: sendmail dangling-relative-symlink /usr/lib/sendmail.sendmail ../sbin/sendmail
W: sendmail dangling-relative-symlink /usr/bin/mailq.sendmail
../../usr/sbin/sendmail
W: sendmail dangling-relative-symlink /usr/bin/purgestat ../../usr/sbin/sendmail
W: sendmail dangling-relative-symlink /usr/bin/hoststat ../../usr/sbin/sendmail
=> not dangling after alternatives in run in %post
E: sendmail zero-length /var/log/mail/statistics
=> OK
W: sendmail log-files-without-logrotate /var/log/mail
=> this directory contains only the statistics file, which doesn't need
=> rotating
W: sendmail dangerous-command-in-%post cp
=> %post script is well-tested
W: sendmail service-default-enabled /etc/rc.d/init.d/sendmail
=> listens only on localhost out of the box, so OK
E: sendmail incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
E: sendmail incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
=> two daemons running, both can't be coherent at the same time

$ rpmlint sendmail-cf-8.14.0-1.x86_64.rpm
W: sendmail-cf summary-ended-with-dot The files needed to reconfigure Sendmail.
W: sendmail-cf invalid-license Sendmail
W: sendmail-cf no-url-tag
=> already covered
W: sendmail-cf no-documentation
=> perhaps label /usr/share/sendmail-cf/README as %doc ?
=> (and perhaps don't bother including /usr/share/sendmail-cf/cf/README at
=> all?)
E: sendmail-cf non-executable-script /usr/share/sendmail-cf/sh/makeinfo.sh 0644
=> the m4 snippet that uses this doesn't need it to be executable

$ rpmlint sendmail-debuginfo-8.14.0-1.x86_64.rpm
W: sendmail-debuginfo invalid-license Sendmail
W: sendmail-debuginfo no-url-tag
=> already covered

$ rpmlint sendmail-devel-8.14.0-1.x86_64.rpm
W: sendmail-devel summary-ended-with-dot Extra development include files and
development files.
W: sendmail-devel invalid-license Sendmail
W: sendmail-devel no-url-tag
=> already covered
W: sendmail-devel no-documentation
=> how about:
=> %doc libmilter/docs/*

$ rpmlint sendmail-doc-8.14.0-1.x86_64.rpm
W: sendmail-doc summary-ended-with-dot Documentation about the Sendmail Mail
Transport Agent program.
W: sendmail-doc invalid-license Sendmail
W: sendmail-doc no-url-tag
=> already covered

Comment 3 Paul Howarth 2007-02-15 14:05:03 EST
Created attachment 148135 [details]
Patch addressing most rpmlint issues worth fixing
Comment 4 Robert Scheck 2007-02-15 14:37:52 EST
/usr/lib/sendmail is a dangling symlink, which should be owned by sendmail, 
too. Paul, if this should not be relevant for this review, I'll create a own 
bug report for it, because unowned dangling symlinks are worse. IMHO this is
another MUST for approving.
Comment 5 Thomas Woerner 2007-02-16 06:06:11 EST
No, /usr/lib/sendmail is generated by alternatives. It could also point to
postfix or exim.
Comment 6 Robert Scheck 2007-02-16 09:13:21 EST
Any chance to own /usr/lib/sendmail somehow else?
Comment 7 Paul Howarth 2007-02-16 09:50:35 EST
I think adding:

Provides: /usr/lib/sendmail

would achieve the desired result. I think that is how sendmail "owns"
/usr/sbin/sendmail, which is also managed by alternatives.
Comment 8 Paul Howarth 2007-02-20 06:58:07 EST
Created attachment 148408 [details]
Patch implementing most of the following review suggestions

Review to follow shortly.
Comment 9 Paul Howarth 2007-02-20 07:00:26 EST
Review:
=======

- rpmlint output already covered in Comment #2
- package and spec naming OK
- package generally meets guidelines
- license is "Sendmail", not listed on FSF "list of licenses" page but
  sendmail *is* listed in the FSF directory of free software
  (http://directory.fsf.org/sendmail.html)
- license correctly tagged in spec but license text needs to be packaged
- spec written in English but legibility could be improved
- sources match upstream
- package builds OK on i386 and x86_64 in mock for rawhide
- buildreqs OK
- no locale data included
- no shared libraries included
- not relocatable
- no directory ownership problems
- no permissions problems
- %clean section present and correct
- macro usage is reasonably consistent
- code, not content
- large docs in -doc subpackage
- docs don't affect runtime
- header files and static libraries included in -devel subpackage
  (no upstream support for dynamic libraries)
- no pkgconfig files
- devel package has proper versioned dependency on main package
- no libtool archives
- not a GUI app, no desktop file needed
- scriptlets are complex, but well-tested
- all subpackages have proper versioned dependencies on main package

Needs Work:

* LICENSE file must be included as %doc in the main package I think.
  I'd also suggest moving FAQ, KNOWNBUGS, README, RELEASE_NOTES in the
  same way

* non-standard buildroot %{_tmppath}/%{name}-root
  Guidelines now mandate:
  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* directory creation near the top of %install not using the same macros as
  later parts of %install

Suggestions:

* Group: for devel package should be Development/Libraries I think

* %post script could return non-zero exit code - add "exit 0" at end?

* symlinks for the sendmail-specific versions of files managed using
  alternatives should point to the sendmail-specific targets rather than the
  generic targets, i.e.
  /usr/bin/mailq.sendmail -> ../../usr/sbin/sendmail
  /usr/bin/newaliases.sendmail -> ../../usr/sbin/sendmail
  /usr/lib/sendmail.sendmail -> ../sbin/sendmail
  should instead be:
  /usr/bin/mailq.sendmail -> ../../usr/sbin/sendmail.sendmail
  /usr/bin/newaliases.sendmail -> ../../usr/sbin/sendmail.sendmail
  /usr/lib/sendmail.sendmail -> ../sbin/sendmail.sendmail

* all of the alternatives-managed files should be "provided" by sendmail.
  Currently, the following are provided:
  %{_sbindir}/sendmail
  %{_bindir}/mailq
  %{_bindir}/newaliases
  %{_bindir}/rmail
  %{_mandir}/man1/mailq.1.gz
  %{_mandir}/man1/newaliases.1.gz
  %{_mandir}/man5/aliases.5.gz
  Also needed are:
  /usr/lib/sendmail
  %{_sysconfdir}/pam.d/smtp
  %{_mandir}/man8/sendmail.8.gz

* general legibility improvements:
- group the RPM tags at the top of the spec into general, build-time, and
  run-time sections
- extra comments, particularly in %install
- dispense with here documents
- consistent indentation
- replace initdir with prefdefined initrddir macro
- use the "symlinks" program to fix up symlinks rather than the scripted
  routine that figures out how deep in the hierarchy a particular target
  directory is

* removal of old cruft
- the symbols _FFR_WORKAROUND_BROKEN_NAMESERVERS, _FFR_SMTP_SSL,
  _FFR_BLOCK_PROXIES, _FFR_UNSAFE_SASL, _FFR_MILTER_ROOT_UNSAFE
  no longer appear in the sendmail source and hence can be removed from the
  spec
- the MySQL stuff in the spec appears to be there to support building with a
  patch maintained outside of the upstream sendmail source:
  http://www.palsenberg.com/index.php/plain/projects/sendmail_mysql_map_class
  This patch is not included in the Fedora package so why is the rest of the
  mysql support included?

* addition of old cruft
- if %{old_setup} is supported, include the aliases file so that just changing
  the macro value and rebuilding the spec will work. Otherwise, drop
  %{old_setup} altogether.

* is it worth packaging /usr/share/sendmail-cf/cf/README ?

* macro usage:
- should /etc/alises  be %{_sysconfdir}/aliases ?
- should /etc/mail be %{_sysconfdir}/mail ?
- should /etc/pam.d be %{_sysconfdir}/pam.d ?
- should /etc/smrsh be %{_sysconfdir}/smrsh ?
- should /var/spool be %{_localstatedir}/spool ?
- files are installed to macro-ised directories in %install but the %files
  list has hardcoded directory names like /usr/bin

* timestamps - I suggest trying to preserve all timestamps in upstream files
  that get packaged

* scripted edits are done in %install using a mixture of perl and sed scripts,
  though they're all just straightforward search and replace changes. Best to
  just use sed for that.

* the alternatives call in %post assumes that manpages are compressed using
  gzip, and will fail if for instance the buildsystem is configured to use
  bzip2 instead (or no compression). I think it's best to make the same
  assumptions everywhere, so I'd either (a) make the %post script very clever
  so it detects the manpage filenames (hard), or (b) hardcode the .gz
  extension for manpages in the %files list so that package build will fail on
  such systems.

* Many of the %attr references in the %files list are redundant and could be
  removed.

* The current %post script clobbers my access database every time I upgrade
  the sendmail package, because I have tweaked /etc/mail/Makefile to create
  the access.db from the concatenation of two files, /etc/mail/access and
  /etc/mail/access.shared. The %post script creates a new /etc/mail/access.db
  from /etc/mail/access, which loses the bulk of my entries. I would like to
  propose that if /usr/bin/make is installed, the /etc/mail/Makefile is used
  to generate the updated databases, else revert to using the original direct
  call to makemap.


I'd also like to see the conditional build options (SASL1/SASL2/TLS etc.)
reworked to use rpmbuild's --with/--without options so that alternative builds
can be done straight from the SRPM without tweaking the spec file itself. Let's
leave that for now though.
Comment 10 Paul Howarth 2007-02-21 05:49:01 EST
Review process has changed again; reviewer remains as assignee now.
Comment 11 Paul Howarth 2007-02-21 05:53:44 EST
Grr, reporter is nobody; whose bright idea was that?
Comment 12 Ville Skyttä 2007-04-07 14:25:50 EDT
Should add "Provides: MTA"; other packages that provide a MTA do that as well
(exim, postfix, ssmtp, esmtp (bug filed, pending))
Comment 13 Patrice Dumas 2007-04-07 16:36:33 EDT
(In reply to comment #9)

> * all of the alternatives-managed files should be "provided" by sendmail.

Is it really sure? What is the point of providing the man pages?


>   %{_mandir}/man1/mailq.1.gz
>   %{_mandir}/man1/newaliases.1.gz
>   %{_mandir}/man5/aliases.5.gz
>   %{_mandir}/man8/sendmail.8.gz
> 
Comment 14 Paul Howarth 2007-04-08 03:08:09 EDT
Providing the manpages means that 'rpm -qf /usr/share/man/man1/mailq.1.gz' etc.
will return a meaningful result.
Comment 15 Patrice Dumas 2007-04-08 05:05:55 EDT
Uhm. For me a meaningfull result would be that that file isn't
provided by any package. A file provide is useful if other packages
may depend on it (like %_sbindir/sendmail), but in my opinion 
it would be better if the package didn't own the alternative symlinks
when possible.

Not to mention that, for other reduced functionality MTA like esmtp
those man pages and the binaries are in the alternate system, even
when they don't do anything, so I think it is even more wrong to 
provide them.
Comment 16 Paul Howarth 2007-04-09 09:38:16 EDT
I think that if it's OK to provide %_sbindir/sendmail then it should be OK to
provide the alternatives-based files too for consistency. Whilst there may well
be other packages with dependencies on %_sbindir/sendmail, which isn't going to
apply in the cases of manpages, the provides for the manpages cou;d still be
useful for someone using say repoquery to find a manpage for a file so as to
discover the file format etc.

However, I don't see this as a blocker either way.
Comment 17 Thomas Woerner 2007-04-12 10:56:42 EDT
> E: sendmail hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/libmilter.a
> => false positive, the %install script is removing the library incorrectly
> => installed there by the upstream build script
This is wrong, because /usr/lib/libmilter.a gets removed for a 64 bit build,
because /usr/lib/libmilter.a has to be 32 bit here, but sendmail installs the 64
bit build.

symlinks is not usable, because it would create realtive links outside of the
buildroot.

%{_localstatedir} ist not /var it is %{_prefix}/var, ergo /usr/var.

/etc/aliases is not part of the sendmail package.

===> Please have a look at sendmail-8.14.1-1.
Comment 18 Paul Howarth 2007-04-12 11:14:30 EDT
(In reply to comment #17)
> > E: sendmail hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/libmilter.a
> > => false positive, the %install script is removing the library incorrectly
> > => installed there by the upstream build script
> This is wrong, because /usr/lib/libmilter.a gets removed for a 64 bit build,
> because /usr/lib/libmilter.a has to be 32 bit here, but sendmail installs the 64
> bit build.

OK, but that's what I meant; the upstream installer puts it in /usr/lib and the
spec puts it in /usr/lib64, so the upstream-installed one needs to be removed on
x86_64.

> symlinks is not usable, because it would create realtive links outside of the
> buildroot.

Are you sure? I tried the patch from Comment #8 before posting it and it looked
to be doing the right thing to me.

> %{_localstatedir} ist not /var it is %{_prefix}/var, ergo /usr/var.

$ rpm --eval '%{_localstatedir}'
/var

> /etc/aliases is not part of the sendmail package.

It should be if the package is supposed to work with %{old_setup} = yes
(obviously not for current Fedora versions but for people trying to rebuild for
older distros). I don't think %{old_setup} should be supported half-heartedly -
either let it work properly or remove it altogether.

> ===> Please have a look at sendmail-8.14.1-1.

Where can I find that? It's not in my just-updated cvs checkout.
Comment 19 Thomas Woerner 2007-04-12 11:29:19 EDT
OK, agree now with _localstatedir:

/usr/lib/rpm/macros:
%_localstatedir         %{_prefix}/var
/usr/lib/rpm/redhat/macros:
%_localstatedir         /var

I am always looking at /usr/lib/rpm/macros instead of /usr/lib/rpm/redhat/macros
 ...

The old_setup switch is gone, also update support for sendmail < 8.12.0 with the
corresponding triggers and post script tweaks.

sendmail-8.14.1-1.1 is in rawhide with additional prereq replacements.
Comment 20 Paul Howarth 2007-04-13 13:11:36 EDT
rpmlint output, ignoring things explained away in Comment #2.

W: sendmail unversioned-explicit-provides MTA
=> I'm not convinced about the merits of "Provides: MTA" vs. "Provides:
smtpdaemon", and the discussion on fedora-devel-list didn't seem to lead to any
particular conclusion. It's harmless enough though so I'm not going to quibble
about it.

E: sendmail hardcoded-library-path in /usr/lib/sendmail
=> This comes from "Provides: /usr/lib/sendmail", is intentional and required
for backwards compatibility (OK).

W: sendmail unversioned-explicit-provides %{_sysconfdir}/pam.d/smtp
W: sendmail unversioned-explicit-provides %{_mandir}/man8/sendmail.8.gz
== Provides for alternatives-based files (OK).

W: sendmail mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 232)
=> Should use either spaces or tabs consistently, not a mixture of the two.

E: sendmail non-standard-executable-perm /usr/sbin/makemap 0555
=> Permission should be 0755 to enable debuginfo extraction.

E: sendmail non-readable /etc/aliases.db 0640
=> This has appeared because the original package explicitly used %attr(0644...
for this file. Harmless because (a) the file is %ghosted and (b) the permissions
will be 0640 on the installed system anyway if the file is created properly.

E: sendmail executable-marked-as-config-file /etc/rc.d/init.d/sendmail
=> Marking initscripts as %config is a contentious subject. I'm against it on
the basis that configurability should be encapsulated in /etc/sysconfig/sendmail
etc. but I can see the merits of the argument the other way too. Do you have a
strong opinion on this or is this just an inheritance from older packaging?

E: sendmail non-standard-executable-perm /usr/sbin/smrsh 0555
=> Permission should be 0755 to enable debuginfo extraction.

E: sendmail postin-without-chkconfig /etc/rc.d/init.d/sendmail
=> /sbin/chkconfig --add sendmail was removed from %post along with the stuff
for upgrading from old versions, needs to be reinstated.

Other comments:

FFR_UNSAFE_SASL still present in sasl2 config - why?

Why is the not-included mysql support stuff still in the spec?

Any thoughts on my suggestion for the %post script near the end of Comment #9?

Finally, the line containing "MSPQOWN=${nameuser}" in the spec has a trailing
space on it. Please remove it, it's been annoying me for ages ;-)

Comment 21 Robert Scheck 2007-04-15 12:04:43 EDT
The directory /usr/share/doc/sendmail-8.14.1 is no longer owned!
Comment 22 Thomas Woerner 2007-04-16 05:28:22 EDT
All done in package sendmail-8.14.1-2.
Comment 23 Paul Howarth 2007-04-16 13:16:57 EDT
rpmlint looks OK now apart from:
W: sendmail mixed-use-of-spaces-and-tabs (spaces: line 868, tab: line 2)
some very minor cosmetic changes to the changelog can fix this

The provides for "MTA" and smtpdaemon are well before the rest of the provides
in the spec; it'd be good if they were all together.

Requires(postun) doesn't need to include chkconfig

Requires: openssl (with_tls, with_sasl, with_ldap) is redundant; autodetected
library dependencies will pull in the correct package if sendmail is built
against openssl.

Similarly, Requires: openldap is redundant.

%description doc says that "papers are provided in PostScript(TM) and troff
formats" but it's now a PDF.

The "touch" and initial "chmod" parts of "create db files" in %install are
redundant because the files were created in the immediately preceding part of
the spec, and with the correct permissions.

Installation of %{maildir}/Makefile would be better grouped with the
installation of the other files in %{maildir} above "create db files" rather
than grouped with the initscript installation.

Still no comment on using /etc/mail/Makefile for map rebuilds in %post?

%attr(0755,root,root) for %{_initrddir}/sendmail in $files is redundant as those
are the attributes it would have by default.

So, mainly those are cosmetic changes and in general I'm happy with this.

Any more comments from anyone else before I approve this one?


Comment 24 manuel wolfshant 2007-04-16 13:47:58 EDT
Because sendmail it basically the reference for all MTAs, I'd kindly ask you to
take a look at the thread started by Ville Skyttä:
https://www.redhat.com/archives/fedora-devel-list/2007-April/msg00377.html

I have no objections at all for sendmail providing any or all of the three
capabilities mentioned in that thread (/usr/sbin/sendmail, MTA, smtpdaemon), but
maybe with this occasion we could define some sort of standard, defining the
exact meaning of each capability. With that definitions at hand we could then
file bugs and hopefully fix mdadm (see also
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226134), mailx and any
other programs acting as MTA or having MTA requirements.
Comment 25 Robert Scheck 2007-04-18 16:01:22 EDT
Warnung: Datei doppelt aufgelistet: /usr/share/sendmail-cf/README
Warnung: Datei doppelt aufgelistet: /usr/share/sendmail-cf/sendmail.schema

This means the files above are listed twice.
Comment 26 Paul Howarth 2007-06-26 05:58:29 EDT
Created attachment 157873 [details]
Patch fixing build issue in current rawhide

The current package in CVS does not build in rawhide. This is because m4 in
rawhide has recently been upgraded to version 1.4.9, and versions of m4 later
than 1.4.8 return a non-zero exit code if a file to be include-d is not found.
One of the patches applied in the Fedora package to the default submit.mc
triggers this problem during the early part of %install and causes the package
build to fail.

The attached patch treats submit.{mc,cf} similarly to sendmail.{mc,cf} and
fixes this problem.
Comment 27 Paul Howarth 2009-06-03 06:07:26 EDT
Time to get this review moving again. Here's a refresher of where we stand:

Current rpmlint state
---------------------

Source Package:
W: unversioned-explicit-provides MTA
W: unversioned-explicit-provides smtpdaemon
W: unversioned-explicit-provides server(smtp)
=> All Ok - these are valid virtual provides for which a version would be inappropriate
W: unversioned-explicit-provides %{_sbindir}/sendmail
W: unversioned-explicit-provides %{_bindir}/mailq
W: unversioned-explicit-provides %{_bindir}/newaliases
W: unversioned-explicit-provides %{_bindir}/rmail
W: unversioned-explicit-provides /usr/lib/sendmail
W: unversioned-explicit-provides %{_sysconfdir}/pam.d/smtp
W: unversioned-explicit-provides %{_mandir}/man1/mailq.1.gz
W: unversioned-explicit-provides %{_mandir}/man1/newaliases.1.gz
W: unversioned-explicit-provides %{_mandir}/man5/aliases.5.gz
W: unversioned-explicit-provides %{_mandir}/man8/sendmail.8.gz
=> Current guidelines on alternatives usage
   (https://fedoraproject.org/wiki/PackagingDrafts/UsingAlternatives, not
   yet written into the main guidelines but approved by FPC - see
   https://fedoraproject.org/wiki/Packaging:Minutes/20090414) says to %ghost these files
   rather than using explicit provides.
   (I see this is currently under discussion on fedora-packaging)
E: hardcoded-library-path in /usr/lib/sendmail
E: hardcoded-library-path in %{buildroot}/usr/lib
E: hardcoded-library-path in %{buildroot}/usr/lib/sendmail.sendmail
E: hardcoded-library-path in /usr/lib/sendmail.sendmail
E: hardcoded-library-path in /usr/lib/sendmail.sendmail
=> legitimate install of /usr/lib/sendmail for backward script compatibility
sendmail.src: W: mixed-use-of-spaces-and-tabs (spaces: line 964, tab: line 2)
=> easily fixed, and should be

Main Binary Package:
W: only-non-binary-in-usr-lib
=> rpmlint confused by a symlink to a binary elsewhere
E: file-in-usr-marked-as-conffile /usr/lib64/sasl2/Sendmail.conf
W: conffile-without-noreplace-flag /usr/lib64/sasl2/Sendmail.conf
=> This is where cyrus-sasl puts things, so not much choice about this at the moment
W: non-standard-gid /usr/sbin/sendmail.sendmail smmsp
E: setgid-binary /usr/sbin/sendmail.sendmail smmsp 02755
E: non-standard-executable-perm /usr/sbin/sendmail.sendmail 02755
E: non-standard-dir-perm /var/spool/mqueue 0700
W: non-standard-uid /var/spool/clientmqueue smmsp
W: non-standard-gid /var/spool/clientmqueue smmsp
E: non-standard-dir-perm /var/spool/clientmqueue 0770
=> non-standard but correct
W: log-files-without-logrotate /var/log/mail
=> This directory contains only the statistics file, which doesn't need rotating
W: dangerous-command-in-%post chown
=> %post script is well-tested
W: service-default-enabled /etc/rc.d/init.d/sendmail
=> Listens only on localhost out of the box, so OK
E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
=> Two daemons running, both can't be coherent at the same time
E: zero-length /var/log/mail/statistics
=> Intended and OK
E: executable-marked-as-config-file /etc/mail/make
=> Script needs to be editable for people that build their databases in non-standard ways

Configuration Package:
E: non-executable-script /usr/share/sendmail-cf/sh/makeinfo.sh 0644
=> Doesn't need to be executable due to the way it's called; could shut
=> rpmlint up by removing the shellbang or making it executable though

Documentation Package:
W: file-not-utf8 /usr/share/doc/sendmail-8.14.3/contrib/etrn.0
=> Should be recoded to UTF-8 like everything else in Fedora
E: wrong-script-interpreter /usr/share/doc/sendmail-8.14.3/contrib/bounce-resender.pl "/usr/local/bin/perl"
E: wrong-script-interpreter /usr/share/doc/sendmail-8.14.3/contrib/etrn.pl "/usr/local/bin/perl"
E: wrong-script-interpreter /usr/share/doc/sendmail-8.14.3/contrib/smcontrol.pl "/usr/local/bin/perl"
=> Should be using %{__perl} instead

Milter Runtime Library Package:
W: no-documentation
=> OK, there isn't really any documentation appropriate to this package

7 packages and 0 specfiles checked; 18 errors, 25 warnings.

A few other general comments
----------------------------

Please consider adding the patches from Bug #485426 to fix socket descriptor
leaks to child processes.

Requires: chkconfig for %postun isn't needed.

Requires: openssl/openldap aren't needed - automatic library dependencies suffice.

sendmail-doc and sendmail-cf could be noarch subpackages.

The %attr(0755,root,root) for %{_initrddir}/sendmail isn't necessary because
the initscript is installed with the correct permissions in the first place.

%global is preferred over %define for the sort of use made in this package.

The initscript sources /etc/rc.d/init.d/functions and hence the main package
should have a dependency on initscripts. Adding this would then mean that the
directory ownership of /etc/NetworkManager/dispatcher.d was unnecessary, as
the initscripts package also owns that directory.

Consider adding a "/" suffix to directories owned by the package in the %files
lists, for clarity.

The provides for "MTA" and smtpdaemon are well before the rest of the provides
in the spec; it'd be good if they were all together.

%description doc says that "papers are provided in PostScript(TM) and troff
formats" but it's now a PDF.

Is there any point to supporting with_sasl1? It's not been used since RHEL 3
and there are lots of things in the current package that would prevent it
being rebuilt for RHEL 3 (e.g. it would need to include /etc/aliases again).
Comment 28 Paul Howarth 2009-06-14 17:33:06 EDT
The /etc/mail/make script handles the authinfo database but not the msp-authinfo database (see http://www.sendmail.org/m4/msp.html).

Adding this line after the one that handles the authinfo database works for me:

test -f msp-authinfo && makedb msp-authinfo.db && chown smmsp:smmsp msp-authinfo{,.db} && chmod 640 msp-authinfo{,.db}
Comment 29 Miroslav Lichvar 2009-12-15 09:42:14 EST
Sorry for taking so long.

Most of this (hopefully all blockers) should be fixed in sendmail-8.14.3-9.fc13.

I didn't include the msp-authinfo support as it requires special permissions and makemap fails when trying to regenerate the file.

Thanks for the review.
Comment 30 Jaroslav Škarvada 2010-02-07 21:00:24 EST
What about sendmail-8.14.4-2.fc13? Still blockers?
Comment 31 Marcela Mašláňová 2010-02-23 08:16:12 EST
Let's move it a little:
[marca@caladan devel]$ rpmlint sendmail-8.14.4-3.fc13.x86_64.rpm 
sendmail.x86_64: W: only-non-binary-in-usr-lib
> This should be fixed.
sendmail.x86_64: E: file-in-usr-marked-as-conffile /usr/lib64/sasl2/Sendmail.conf
> Probably can't fix
sendmail.x86_64: W: conffile-without-noreplace-flag /usr/lib64/sasl2/Sendmail.conf
> Use noreplace flag if user can lost settings.
sendmail.x86_64: E: setgid-binary /usr/sbin/sendmail.sendmail smmsp 02755
sendmail.x86_64: E: non-standard-executable-perm /usr/sbin/sendmail.sendmail 02755
sendmail.x86_64: E: zero-length /var/log/mail/statistics
sendmail.x86_64: E: executable-marked-as-config-file /etc/mail/make
sendmail.x86_64: E: non-standard-dir-perm /var/spool/mqueue 0700
sendmail.x86_64: E: non-standard-dir-perm /var/spool/clientmqueue 0770
> Strange, but probably ok in this case.
sendmail.x86_64: W: log-files-without-logrotate /var/log/mail
> Couldn't be lograted?
sendmail.x86_64: W: dangerous-command-in-%post chown
sendmail.x86_64: W: service-default-enabled /etc/rc.d/init.d/sendmail
sendmail.x86_64: E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
sendmail.x86_64: E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
sendmail.x86_64: W: service-default-enabled /etc/rc.d/init.d/sendmail
> Ok in this case
1 packages and 0 specfiles checked; 9 errors, 6 warnings.
[marca@caladan devel]$ rpmlint sendmail-8.14.4-3.fc13.src.rpm 
sendmail.src:59: W: unversioned-explicit-provides MTA
sendmail.src:59: W: unversioned-explicit-provides smtpdaemon
sendmail.src:59: W: unversioned-explicit-provides server(smtp)
> Ok in this case.
sendmail.src:60: W: unversioned-explicit-provides %{_sbindir}/sendmail
sendmail.src:60: W: unversioned-explicit-provides %{_bindir}/mailq
sendmail.src:60: W: unversioned-explicit-provides %{_bindir}/newaliases
sendmail.src:61: E: hardcoded-library-path in /usr/lib/sendmail
sendmail.src:61: W: unversioned-explicit-provides %{_bindir}/rmail
sendmail.src:61: W: unversioned-explicit-provides /usr/lib/sendmail
sendmail.src:62: W: unversioned-explicit-provides %{_sysconfdir}/pam.d/smtp
sendmail.src:63: W: unversioned-explicit-provides %{_mandir}/man1/mailq.1.gz
sendmail.src:63: W: unversioned-explicit-provides %{_mandir}/man1/newaliases.1.gz
sendmail.src:64: W: unversioned-explicit-provides %{_mandir}/man5/aliases.5.gz
sendmail.src:64: W: unversioned-explicit-provides %{_mandir}/man8/sendmail.8.gz
> It was mentioned before, you can use ghost for these.
sendmail.src:269: E: hardcoded-library-path in %{buildroot}/usr/lib
sendmail.src:308: E: hardcoded-library-path in %{buildroot}/usr/lib/sendmail.sendmail
sendmail.src:429: E: hardcoded-library-path in /usr/lib/sendmail.sendmail
sendmail.src:478: E: hardcoded-library-path in /usr/lib/sendmail.sendmail
> Ok in this case.
1 packages and 0 specfiles checked; 5 errors, 13 warnings.
Comment 32 Marcela Mašláňová 2010-02-23 08:43:32 EST
? rpmlint on every package.
OK name of package accords to the Naming Guidelines.
OK spec file name match the base package %{name}.
OK package meet the Packaging Guidelines.
OK package has a good license.
OK spec file is written in American English.
OK spec file for the package must be legible.
OK The sources used to build the package must match the upstream source.
> 1b23d5000c8e7bfe82ec1a27f2f5fdc5
OK successful koji compilation.
OK correct BuildRequires, Requires.
- proper use of %find_lang macro.
OK shared library must call ldconfig in %post and %postun.
OK relocatable package must state this fact.
OK package must own their directories.
OK permissions on files must be set properly.
OK %clean section with rm -rf %{buildroot}.
OK consistent use of macros.
OK package contains code, or permissable content.
OK large documentation go in a -doc subpackage.
OK %doc must not affect the runtime of the application.
OK header files must be in a -devel package.
- static libraries must be in a -static package.
- pkgconfig(.pc) files must 'Requires: pkgconfig'.
OK library files with a suffix (.so) must go in -devel.
> in this case separate packages -milter
OK usually devel packages must require the base package.
OK Remove .la libtool archives.
- GUI applications must include a %{name}.desktop file.
OK %install section starts with rm -rf %{buildroot}.

%defattr(-,root,root)
Should be %defattr(-,root,root,-)
Comment 33 Jaroslav Škarvada 2010-03-03 04:45:03 EST
Thank you for review, let's move to sendmail-8.14.4-4.fc14, here are my comments:

sendmail.x86_64: W: only-non-binary-in-usr-lib
This should be fixed.
> Caused by symlink /usr/lib/sendmail.sendmail. This approach is used accross MTAs. Fixing this warning requires changing all other MTAs - this is out of my scope.

sendmail.x86_64: W: conffile-without-noreplace-flag
/usr/lib64/sasl2/Sendmail.conf
Use noreplace flag if user can lost settings.
> Fixed.

sendmail.x86_64: W: log-files-without-logrotate /var/log/mail
Couldn't be lograted?
> Nothing to rotate, there is only /var/log/mail/statistics with small internal statistics.

sendmail.src:60: W: unversioned-explicit-provides %{_sbindir}/sendmail
sendmail.src:60: W: unversioned-explicit-provides %{_bindir}/mailq
sendmail.src:60: W: unversioned-explicit-provides %{_bindir}/newaliases
sendmail.src:61: E: hardcoded-library-path in /usr/lib/sendmail
sendmail.src:61: W: unversioned-explicit-provides %{_bindir}/rmail
sendmail.src:61: W: unversioned-explicit-provides /usr/lib/sendmail
sendmail.src:62: W: unversioned-explicit-provides %{_sysconfdir}/pam.d/smtp
sendmail.src:63: W: unversioned-explicit-provides %{_mandir}/man1/mailq.1.gz
sendmail.src:63: W: unversioned-explicit-provides
%{_mandir}/man1/newaliases.1.gz
sendmail.src:64: W: unversioned-explicit-provides %{_mandir}/man5/aliases.5.gz
sendmail.src:64: W: unversioned-explicit-provides %{_mandir}/man8/sendmail.8.gz
It was mentioned before, you can use ghost for these.
> A little tricky to implement, but all are ghosted now.

After these changes, it is possible that you encounter following new warning:
sendmail.x86_64: W: read-error /etc/pam.d/smtp [Errno 2] No such file or directory: '/tmp/rpmlint.sendmail-8.14.4-4.fc14.x86_64.rpm.QncWlw//etc/pam.d/smtp'
It is false positive, caused by bug in rpmlint, I reported it with patch to bugzilla (#570086).
Comment 34 Marcela Mašláňová 2010-03-03 06:53:53 EST
I'd like to ACCEPT it if maintainer of this review don't mind.
Comment 35 Miroslav Lichvar 2010-03-04 02:37:51 EST
Please be careful with the %ghosts. If there are more MTA packages installed, removing sendmail will remove the files as other MTAs don't own the files.

I'm not sure if it's possible to make a clean upgrade to the new alternatives system. I posted about the problem on fedora-packaging list when the new system was introduced, but there were no solutions proposed.
Comment 36 Jaroslav Škarvada 2010-03-05 08:15:54 EST
Thank you for pointing this out Mirek. I think it is worth to keep this change as it is according to https://fedoraproject.org/wiki/Packaging:Alternatives

I filled bugs addressing this issue accross all affected MTAs:
esmtp #570797
ssmtp #570799
exim #570800
postfix #570801
Comment 37 Paul Howarth 2010-03-05 09:38:42 EST
A few minor nits I spotted this morning:

 * Can we make the -cf and -docs packages noarch?
 * Can we change %define to %global throughout?
 * The perl script interpreter fixup for contrib stuff could get done in %prep rather than %install
 * %description doc still says that "papers are provided in PostScript(TM) and troff formats" even though the only format packaged is PDF

I'll have a last look next week when I have a bit more time.
Comment 38 Jaroslav Škarvada 2010-03-09 09:27:59 EST
Thank you Paul. 

It looks like the:
sendmail.x86_64: E: file-in-usr-marked-as-conffile
/usr/lib64/sasl2/Sendmail.conf
can also be easily fixed.

I will wait for Paul's comments and than implement all these in rawhide.
Comment 39 Paul Howarth 2010-03-09 09:54:26 EST
(In reply to comment #38)
> Thank you Paul. 
> 
> It looks like the:
> sendmail.x86_64: E: file-in-usr-marked-as-conffile
> /usr/lib64/sasl2/Sendmail.conf
> can also be easily fixed.

How do you propose to fix this? Surely not by removing the %config?

> I will wait for Paul's comments and than implement all these in rawhide.    

I hope to be able to take another look tomorrow.
Comment 40 Jaroslav Škarvada 2010-03-09 10:18:18 EST
It looks like the sasl is scanning the /usr/lib64/sasl2 for config and than tries the /etc/sasl2. From the spec file it looks like as a long time when this behaviour was implemented so I think it should be safe to simple move the config file (also with some check for custom config in old location, maybe in %prep). I will test this behaviour and if OK I will implement it.
Comment 41 Jaroslav Škarvada 2010-04-08 09:00:55 EDT
Tested on F12 with config in /etc/sasl2 and the SASL2 worked as expected. From the CVS log it looks like this approach can be used for all systems from RHEL-5. Thus no need for config in /usr/lib64/sasl2.
Comment 42 Paul Howarth 2010-04-08 10:57:43 EDT
Can you post the changes you made for this, up update CVS in devel?
Comment 43 Jaroslav Škarvada 2010-06-09 04:02:17 EDT
I patched sendmail to build with libdd-5, rebuilt it with libdb-5 and switched to /etc/sasl2 config dir. The proposed changes are in CVS - sendmail-8.14.4-6.fc14.

The sasl tries /usr/lib64/sasl2 and /etc/sasl2 config dirs in sequence and the first has higher priority. I add test for config in /usr/lib64/sasl2. If there exists config I move it to /etc/sasl2 replacing whatever is there (/etc/sasl2 has lower priority thus it wasn't used before and this approach shouldn't be a problem). I also fix timestamps. The config did not change between releases thus if not modified by user, the rpm verify should pass after upgrade.

I used post section for this change, because if used in pre section, the old config was always replaced by file from the package (even with noreplace).

Any comments/improvements are welcome.
Comment 44 Paul Howarth 2010-06-09 10:29:07 EDT
Seems to work nicely apart from an annoying message at daemon startup:

error: safesasl(/usr/lib64/sasl2/Sendmail.conf) failed: No such file or directory

(this is with an F-12 build)
Comment 45 Jaroslav Škarvada 2010-06-09 11:08:19 EDT
I can't see this message in rawhide. Originally I planned this feature for rawhide.

I will check later with F12 for curiosity.
Comment 46 Paul Howarth 2010-06-10 05:46:44 EDT
(In reply to comment #45)
> I can't see this message in rawhide. Originally I planned this feature for
> rawhide.
> 
> I will check later with F12 for curiosity.    

I see the same thing on F-13 too. And it turns up in logwatch reports as well:

 --------------------- sendmail Begin (detail=3) ------------------------ 

 
 
 SENDMAIL CONFIGURATION
 ----------------------
 
 Warning: STARTTLS file errors:
     CRLFile missing
 
 SASL database Errors:
    In file /usr/lib64/sasl2/Sendmail.conf:
       No such file or directory: 2 Time(s)
...

Of course it's not a problem if it doesn't happen on Rawhide.
Comment 47 Jaroslav Škarvada 2010-06-10 07:14:41 EDT
Interesting, thank you for pointing this out, I will look on it.

I tested only on rawhide. I also tested the SASL auth with various SASL configs and it worked as expected. What I did (on rawhide):

# /etc/init.d/sendmail stop
# /etc/init.d/sendmail start
# echo test | sendmail root
# sync
# logwatch --range today --detail 10 --print --service All
# grep -r sasl2 /var/log/*

And I can not see this error on rawhide. I am also going to check the F12/F13. Please can you point me directly to place where this error is logged? And also for the F12/F13 is the SASL config working for you even in case of this error?
Comment 48 Paul Howarth 2010-06-10 07:19:36 EDT
It's logged in /var/log/maillog at daemon startup:

Jun 10 10:44:17 roary sendmail[18035]: gethostbyaddr(192.168.122.1) failed: 1
Jun 10 10:44:17 roary sendmail[18035]: error: safesasl(/usr/lib64/sasl2/Sendmail.conf) failed: No such file or directory
Jun 10 10:44:17 roary sendmail[18036]: starting daemon (8.14.4): SMTP+queueing@01:00:00
Jun 10 10:44:17 roary sendmail[18036]: STARTTLS: ServerCertFile missing
Jun 10 10:44:17 roary sendmail[18036]: started as: /usr/sbin/sendmail -bd -q1h
Jun 10 10:44:17 roary sm-msp-queue[18045]: starting daemon (8.14.4): queueing@01:00:00

The SASL config still works for me (at least using CRAM-MD5 auth).
Comment 49 Jaroslav Škarvada 2010-06-10 11:30:42 EDT
For me the error message doesn't appear after daemon restart but after every:

# /usr/sbin/sendmail -O LogLevel=14 -bs -Am

Thus it seems to be filtered out by default loglevel. Confirmed for F13 and also for rawhide. The good thing is that the functionality is not affected by this message. I will investigate the source.
Comment 50 Jaroslav Škarvada 2010-06-10 11:36:40 EDT
Looks like the 'paranoia' safesasl check inside the sendmail code doesn't count with /etc/sasl2. Probably patching will be needed.
Comment 51 Paul Howarth 2010-06-10 11:45:36 EDT
(In reply to comment #49)
> For me the error message doesn't appear after daemon restart but after every:
> 
> # /usr/sbin/sendmail -O LogLevel=14 -bs -Am
> 
> Thus it seems to be filtered out by default loglevel. Confirmed for F13 and
> also for rawhide. The good thing is that the functionality is not affected by
> this message. I will investigate the source.    

Ah, right. Didn't occur to me to mention that I had:

define(`confLOG_LEVEL',`13')dnl

in my .mc file. Sorry about that.
Comment 52 Paul Howarth 2010-06-11 09:19:14 EDT
Created attachment 423270 [details]
Patch silencing complaint about non-existent %{_libdir}/sasl2/Sendmail.conf

This patch skips the warning for missing SASL config file under %{_libdir}. A missing /etc/sasl2/Sendmail.conf still generates the error. Other errors (such as world-writable file) still generate the error too.
Comment 53 Jaroslav Škarvada 2010-06-11 10:28:53 EDT
Thanks, well done. Integrated in sendmail-8.14.4-7.fc14.
Comment 54 Jaroslav Škarvada 2010-06-14 04:49:19 EDT
Suggestions from comment 37 implemented in sendmail-8.14.4-8.fc14.
Comment 55 Jaroslav Škarvada 2010-06-28 09:46:35 EDT
Any more blockers?
Comment 56 Paul Howarth 2010-07-06 12:24:51 EDT
OK, here's another pass:

rpmlint
=======
sendmail.src:61: W: unversioned-explicit-provides MTA
sendmail.src:61: W: unversioned-explicit-provides smtpdaemon
sendmail.src:61: W: unversioned-explicit-provides server(smtp)
=> normal for virtual provides, ignorable

sendmail.src:270: E: hardcoded-library-path in %{buildroot}/usr/lib
sendmail.src:309: E: hardcoded-library-path in %{buildroot}/usr/lib/sendmail.sendmail
sendmail.src:408: E: hardcoded-library-path in %{buildroot}/usr/lib/sendmail
sendmail.src:445: E: hardcoded-library-path in /usr/lib/sendmail.sendmail
sendmail.src:501: E: hardcoded-library-path in /usr/lib/sendmail.sendmail
sendmail.src:518: E: hardcoded-library-path in /usr/lib/sendmail
=> legitimate install of /usr/lib/sendmail for backward script compatibility

sendmail.x86_64: W: only-non-binary-in-usr-lib
=> false positive; there is a symlink to a real binary in /usr/sbin

sendmail.x86_64: E: setgid-binary /usr/sbin/sendmail.sendmail smmsp 02755L
sendmail.x86_64: E: non-standard-executable-perm /usr/sbin/sendmail.sendmail 02755L
=> needs to be setgid to function correctly

sendmail.x86_64: E: zero-length /var/log/mail/statistics
=> needs to be present and empty, hence OK

sendmail.x86_64: E: executable-marked-as-config-file /etc/mail/make
=> needs to be executable, needs to be customizable, hence OK

sendmail.x86_64: E: non-standard-dir-perm /var/spool/mqueue 0700L
sendmail.x86_64: E: non-standard-dir-perm /var/spool/clientmqueue 0770L
=> permissions for mail queues are correct

sendmail.x86_64: W: log-files-without-logrotate /var/log/mail
=> statistics file here does not need rotating

sendmail.x86_64: W: no-manual-page-for-binary hoststat
sendmail.x86_64: W: no-manual-page-for-binary rmail.sendmail
sendmail.x86_64: W: no-manual-page-for-binary purgestat
=> see below

sendmail.x86_64: W: dangerous-command-in-%post chown
=> necessary to ensure correct permissions, which sendmail is fussy about

sendmail.x86_64: W: service-default-enabled /etc/rc.d/init.d/sendmail
=> listens only on localhost out of the box, so OK

sendmail.x86_64: E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
sendmail.x86_64: E: incoherent-subsys /etc/rc.d/init.d/sendmail sm-client
=> two daemons running, both can't be coherent at the same time

sendmail-cf.noarch: E: non-executable-script /usr/share/sendmail-cf/sh/makeinfo.sh 0644L /bin/sh
=> the m4 snippet that uses this doesn't need it to be executable, hence OK

sendmail-devel.x86_64: W: spelling-error %description -l en_US addons -> ad dons, ad-dons, Abaddon
=> could change it to add-ons but it would still fail the spell check

sendmail-doc.noarch: W: spelling-error %description -l en_US troff -> toff, tr off, tr-off
=> whilst troff is spelt correctly, the assertion that documentation is
   provided in troff format is false; the %description should be re-written
   to reflect the actual contents of the package

sendmail-milter.x86_64: W: no-documentation
=> perhaps move README.libmilter to this package?

Review Checklist
================

- rpmlint mainly OK, as above
- package and spec file naming OK
- package meets guidelines
- license is Sendmail, which is Fedora-approved and matches sources
- upstream license file is packaged OK
- spec file written in English and is legible
- source tarball matches upstream
- package builds fine in mock and koji on all supported architectures
- buildreqs OK
- no translations to worry about
- ldconfig called properly in %post and %postun of milter package; no other
  packages contain shared libraries
- no bundled system libraries
- package not intended to be relocatable
- directory ownership OK
- no duplicate files
- %defattr present and correct, all permissions OK
- macro usage is consistent
- code, not content
- large docs moved to -doc subpackage
- docs don't affect runtime
- header files and libmilter.so symlink properly included in devel subpackage
- no static or libtool libraries packaged
- no pkgconfig files packaged
- devel subpackage has proper dependency on main package
- cf subpackage has proper dependency on main package
- doc subpackage has proper dependency on main package
- milter subpackage correctly does not depend on main package
- not a GUI app, no desktop file needed
- filenames all OK
- package functions OK
- scriptlets are sane
- no file dependencies outside /bin, /sbin, and /usr/sbin
- manpage coverage mostly complete

Issues
======
The hoststat and purgestat commands are described in the main sendmail(8) man
page, so it would be useful to add stub pages for them:

%{_mandir}/man8/hoststat.8 and %{_mandir}/man8/purgestat.8 can just be:
.so man8/sendmail.8

As for rmail, there is a manpage installed but it's not switched in and out
using alternatives. Adding it to the alternatives group should resolve the
rpmlint issue.

%description for the doc subpackage needs rewriting.

/usr/share/sendmail-cf/cf/README is rather useless to end users and doesn't
really need packaging.

The "touch" and initial "chmod" parts of "create db ghosts" in %install are
redundant because the files were created in the immediately preceding part of
the spec, and with the correct permissions.

Some comments in the spec file about the purpose of each patch would be good,
even though there's no upstream bug tracker or development mailing list to
refer to.

Is /usr/sbin/saslauthd really required? I use the auxprop method without
needing saslauthd to run.
Comment 57 Robert Scheck 2010-07-06 13:53:01 EDT
Well...but not everyone likes auxprop - I prefer saslauthd.
Comment 58 Paul Howarth 2010-07-06 14:23:07 EDT
(In reply to comment #57)
> Well...but not everyone likes auxprop - I prefer saslauthd.    

Fair enough, though I need to install cyrus-sasl-md5 manually to get my preferred config working so shouldn't that be a hard requirement too? ;-)
Comment 59 Paul Howarth 2010-07-08 08:08:52 EDT
(In reply to comment #56)
> sendmail-milter.x86_64: W: no-documentation
> => perhaps move README.libmilter to this package?

Adding LICENSE as %doc to this package will have made this one go away. However, there was no need for the addition of "License: Sendmail" to the milter subpackage as it was already inheriting that tag from the main package:

$ rpm -q --qf "%{LICENSE}\n" sendmail-milter
Sendmail
Comment 60 Jaroslav Škarvada 2010-07-08 08:55:09 EDT
OK, thanks, the primary purpose of this update was to be compliant with the new Licensing Guidelines and not to kill the rpmlint warning (side effect ;). I will fix this and previously mentioned problems in new release.
Comment 61 Jaroslav Škarvada 2010-08-04 06:22:33 EDT
 (In reply to comment #58)
> (In reply to comment #57)
> > Well...but not everyone likes auxprop - I prefer saslauthd.    
> 
> Fair enough, though I need to install cyrus-sasl-md5 manually to get my
> preferred config working so shouldn't that be a hard requirement too? ;-)    

In case we ship sasl2 config with pwcheck_method:saslauthd, I think the saslauthd in require is OK. But shouldn't we add cyrus-sasl-plain? It don't need shared secret and AFAIK the TLS/PLAIN is quite common setup thus the basic auth would work out of the box.
Comment 62 Robert Scheck 2010-08-04 07:12:15 EDT
Please do not any dependency to cyrus-sasl-* because I've enough standalone
installations, where sendmail is just sending out e-mails without any auth.
Comment 63 Paul Howarth 2010-08-04 10:58:13 EDT
I think the status quo is OK, no need to change the requires, given that the shipped Sendmail-sasl2.conf is for saslauthd.

Perhaps adding a README.auth with a quick howto (which packages to install, what to configure) for a few common auth scenarios would be better than adding a bunch of requires?
Comment 64 Jaroslav Škarvada 2010-08-06 05:50:31 EDT
OK, thanks. Please check sendmail-8.14.4-10.fc15.

>The "touch" and initial "chmod" parts of "create db ghosts" in %install are
>redundant because the files were created in the immediately preceding part of
>the spec, and with the correct permissions.

Looks OK for me (line 395+). The previous parts are for sources, these are for .db.

>%description for the doc subpackage needs rewriting.

I did my best, but english is not my native language. Feel free to propose something more appropriate.
Comment 65 Paul Howarth 2010-08-09 10:47:03 EDT
(In reply to comment #64)
> >The "touch" and initial "chmod" parts of "create db ghosts" in %install are
> >redundant because the files were created in the immediately preceding part of
> >the spec, and with the correct permissions.
> 
> Looks OK for me (line 395+). The previous parts are for sources, these are for
> .db.

Quite right.

> >%description for the doc subpackage needs rewriting.
> 
> I did my best, but english is not my native language. Feel free to propose
> something more appropriate.    

I'd suggest this:

  This package contains the Sendmail Installation and Operation Guide (PDF),
  text files containing configuration documentation, plus a number of contributed
  scripts and tools for use with Sendmail.

A couple of other quick points:

 * the "README.redhat" (newconfig patch) describes changes that happened for
   sendmail 8.12.x back in July 2002 and reflects what is now standard
   practice in Sendmail installations. I wouldn't have thought it was
   needed any more.

 * README.sendmail doesn't appear to contain anything of use to people
   installing the package.

Robert, do you have any objections to this package being approved now?
Comment 66 Jaroslav Škarvada 2010-08-17 05:06:42 EDT
Thanks.

In sendmail-8.14.4-11.fc15:
- README.libmilter was moved to milter subpackage
- README.redhat was removed
- doc subpackage description was updated

I keep the README.sendmail file because:
- In the future there can be added useful information.
- Included instructions about debugging hooks can be useful.
- It is installed in doc subpackage thus the regular user couldn't be confused.
Comment 67 Paul Howarth 2010-08-17 08:35:04 EDT
OK, it's time I stopped nit-picking. There have been no blockers here for some time. Thanks for being a responsive maintainer Jaroslav.

APPROVED.
Comment 68 Jaroslav Škarvada 2010-08-17 09:39:33 EDT
Thanks for review.

Note You need to log in before you can comment on or make changes to this bug.