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@xxxxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review