[Bug 226407] Merge Review: sendmail

Tue Feb 20 12:00:28 UTC 2007

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: sendmail

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226407

------- Additional Comments From paul at city-fan.org  2007-02-20 07:00 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.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.