Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 217654 (tmda) - Review Request: TMDA - Tagged Message Delivery Agent
Summary: Review Request: TMDA - Tagged Message Delivery Agent
Keywords:
Status: CLOSED NEXTRELEASE
Alias: tmda
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-11-29 05:46 UTC by Bernard Johnson
Modified: 2014-08-22 12:05 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-20 02:57:10 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Bernard Johnson 2006-11-29 05:46:49 UTC
Spec URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda.spec
SRPM URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda-1.1.9-1.fc6.src.rpm
Description:
TMDA is an open source anti-spam system and local mail delivery agent.

Comment 1 Bernard Johnson 2006-11-29 05:55:36 UTC
I would appreciate comments on the rpmlint output (my explanations in parenthesis):

E: tmda zero-length /usr/share/doc/tmda-1.1.9/contrib/dot-tmda/lists/blacklist
E: tmda zero-length /usr/share/doc/tmda-1.1.9/contrib/dot-tmda/lists/confirmed
E: tmda zero-length /usr/share/doc/tmda-1.1.9/contrib/dot-tmda/crypt_key

(these are zero byte files from upstream contrib)

W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/update-internaldomains /usr/bin/env
W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/update-internaldomains perl(Getopt::Std)
W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/update-internaldomains perl(strict)
W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/ofmipd-stunnel-xinetd/stunnel-wrapper /bin/sh
W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/ofmipd-stunnel-xinetd/tmda-ofmipd-wrapper /bin/sh
W: tmda doc-file-dependency /usr/share/doc/tmda-1.1.9/contrib/sendit.sh /bin/sh
W: tmda doc-file-dependency /usr/share/doc/tmda-1.1.9/contrib/vadduser-tmda /bin/sh
W: tmda doc-file-dependency /usr/share/doc/tmda-1.1.9/contrib/vmailmgr-vdir.sh
/bin/sh
W: tmda doc-file-dependency /usr/share/doc/tmda-1.1.9/contrib/vpopmail-vdir.sh
/bin/sh
W: tmda doc-file-dependency /usr/share/doc/tmda-1.1.9/contrib/wrapfd3.sh /bin/sh

(these are contrib files from upstream - no real dependency as they are %doc)

W: tmda-emacs no-documentation
W: tmda-ofmipd no-documentation
E: tmda-ofmipd use-tmp-in-%pre

(%pre creates user with /var/tmp as home directory - %pre does not use /tmp)

W: tmda-ofmipd dangerous-command-in-%postun userdel

(if we create a user, shouldn't we also remove it when the package is removed?)

W: tmda-ofmipd service-default-enabled /etc/rc.d/init.d/tofmipd

(will patch upstream file in a future rev)

W: tmda-ofmipd no-reload-entry /etc/rc.d/init.d/tofmipd

(does not support reload)

W: tmda-ofmipd incoherent-init-script-name tofmipd

(named from upstream; also daemon is a different name (TMDA Old Fashioned Mail
Injection Protocol Daemon - tofmipd) - not the TMDA program itself)

Comment 2 Mamoru TASAKA 2007-01-29 18:16:00 UTC
(Removing NEEDSPONSOR as bug 216723)

Comment 3 Mamoru TASAKA 2007-02-09 16:02:42 UTC
Well, for rpmlint issues:

* spurious-executable-perm
* doc-file-dependency
---------------------------------------------
W: tmda spurious-executable-perm /usr/share/doc/tmda-1.1.9/contrib/printdbm
W: tmda doc-file-dependency
/usr/share/doc/tmda-1.1.9/contrib/update-internaldomains /usr/bin/env
---------------------------------------------
  - You may have confused about these messages, however
---------------------------------------------
$ rpmlint -I spurious-executable-perm
spurious-executable-perm :
The file is installed *with executable permissions*, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

$ rpmlint -I doc-file-dependency
doc-file-dependency :
An included file marked as %doc creates a possible additional dependency in
the package.  Usually, this is not wanted and may be caused by eg. example
scripts *with executable bits set* included in the package's documentation.

-------------------------------------
    so please ensure that %doc files do not have executable
    permissions.

* zero-length
-------------------------------------
E: tmda zero-length /usr/share/doc/tmda-1.1.9/contrib/dot-tmda/lists/blacklist
-------------------------------------
  - How do empty documents make sense?

* User home directory
-------------------------------------
E: tmda-ofmipd use-tmp-in-%pre
-------------------------------------
> (%pre creates user with /var/tmp as home 
> directory - %pre does not use /tmp)
  Usually this should be %{_sysconfdir}/%{name} or something
  else (see xorg-x11-xfs or ntp, for example)

*  userdel, groupdel...
-------------------------------------
  /usr/sbin/userdel ofmipd >/dev/null 2>&1 || :
  /usr/sbin/groupdel ofmipd >/dev/null 2>&1 || :
-------------------------------------
  - Still under discussion, however, as far as I know
    current Fedora policy is that "some dangerous commands
    like userdel or so should not automatically done and
    should executed by sysadmin with care".

* init script recording
-------------------------------------
W: tmda-ofmipd service-default-enabled /etc/rc.d/init.d/tofmipd
-------------------------------------
   Well, if you don't have a strong reason you want to enable
   tofmipd daemon by default, please the line
-------------------------------------
# chkconfig: 2345 87 13
-------------------------------------
   in %{_initrddir}/tofmipd to
-------------------------------------
# chkconfig: - 87 13
-------------------------------------

* init daemon reload
-------------------------------------
W: tmda-ofmipd no-reload-entry /etc/rc.d/init.d/tofmipd
-------------------------------------
  - Just change
-------------------------------------
  restart)
-------------------------------------
    in %{_initrddir}/tofmipd to
-------------------------------------
  restart|reload)
-------------------------------------

Comment 4 Bernard Johnson 2007-02-15 09:01:48 UTC
(In reply to comment #3)
> W: tmda spurious-executable-perm /usr/share/doc/tmda-1.1.9/contrib/printdbm
> W: tmda doc-file-dependency
> E: tmda-ofmipd use-tmp-in-%pre
> W: tmda-ofmipd service-default-enabled /etc/rc.d/init.d/tofmipd
> W: tmda-ofmipd no-reload-entry /etc/rc.d/init.d/tofmipd

I have fixed these problems.

> -------------------------------------
> E: tmda zero-length /usr/share/doc/tmda-1.1.9/contrib/dot-tmda/lists/blacklist
> -------------------------------------
>   - How do empty documents make sense?

Actually, in this case they do.  The dot-tmda directory is made for a user to cp
-r .../dot-tmda to ~/.tmda and then they have a fully setup boilerplate to use
TMDA.  The blacklist & confirmed list are initially blank, and the crypt_key
must be customized.


> *  userdel, groupdel...
> -------------------------------------
>   /usr/sbin/userdel ofmipd >/dev/null 2>&1 || :
>   /usr/sbin/groupdel ofmipd >/dev/null 2>&1 || :
> -------------------------------------
>   - Still under discussion, however, as far as I know
>     current Fedora policy is that "some dangerous commands
>     like userdel or so should not automatically done and
>     should executed by sysadmin with care".

I switched this to fedora-usermgmt style.  I believe this takes a little of the
danger out of the process anyway.  In any case, whatever Fedora policy is chosen
I will adhere to.

Spec URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda.spec
SRPM URL:
http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda-1.1.10-1.fc6.src.rpm

* Thu Feb 15 2007 Bernard Johnson <bjohnson> 1.1.10-1
- version 1.1.10
- substitute RPM_BUILD_ROOT for %%{buildroot} macro
- preserve file timestamps
- change tofmipd user to /etc home directory
- switch to fedora-usermgmt user management
- add Require(post,preun) of /sbin/chkconfig



Comment 5 Mamoru TASAKA 2007-02-17 07:00:25 UTC
Well, for 1.1.10-1.fc7:

* Scriptlets
  - Well, on my environ no one has /etc/ as the home directory.
    For the user tofmipd, I think the home directory should be
    %{_sysconfdir}/tmda or %{_sysconfdir}/tofmipd with the group
    of the directory set as tofmipd.

* Requires
----------------------------------------------
Requires(post): /sbin/chkconfig
Requires(preun):/sbin/chkconfig
Requires(pre):  fedora-usermgmt
Requires(postun):fedora-usermgmt
----------------------------------------------
  - All these are not needed for main package. These are needed
    by -tofmipd package.

  - By the way, why do you use the mixed use of
----------------------------------------------
  /sbin/service tofmipd stop &> /dev/null || :
  %{_initrddir}/tofmipd condrestart &> /dev/null || :
----------------------------------------------
    (i.e. use of /sbin/service v.s. directly calling
          scripts under %{_initrddir} ) ?
    On Fedora, the use of /sbin/service seems to be recommended,
    and Requires(....): /sbin/service is needed.

* Documentation
  - Check if the document "INSTALL" is needed.

* Functionality:
  On FC7 i386, "service tofmipd start" fails 100% as following:
----------------------------------------------------------------
[root@localhost bin]# service tofmipd start
Starting tofmipd: Traceback (most recent call last):
  File "/usr/bin/tmda-ofmipd", line 42, in <module>
    from TMDA import Util
  File "/usr/lib/python2.5/site-packages/TMDA/Util.py", line 27, in <module>
    import email
  File "/usr/lib/python2.5/site-packages/TMDA/pythonlib/email/__init__.py", line
118, in <module>
    import email.mime
ImportError: No module named mime
                                                           [失敗]

(The last word is Japanese, meaning "failed" in English)
----------------------------------------------------------------
  It seems that python tries to search email.mime module from 
  /usr/lib/python2.5/site-packages/TMDA/pythonlib/email/, because
  /email directory is found first. This is because
  /usr/lib/python2.5/site-packages/TMDA/pythonlib
  directory is inserted to the first place in sys.path. 

  However email.mime is on /usr/lib/python2.5/email/mime and
  python seems to search any more...

  The following diff seems to work.
----------------------------------------------------------------
--- tmda-ofmipd.orig    2007-02-16 00:09:06.000000000 +0900
+++ tmda-ofmipd 2007-02-17 15:20:39.000000000 +0900
@@ -36,7 +36,7 @@
     # Prepend /usr/lib/python2.x/site-packages/TMDA/pythonlib
     sitedir = os.path.join(sys.prefix, 'lib', 'python'+sys.version[:3],
                            'site-packages', 'TMDA', 'pythonlib')
-    sys.path.insert(0, sitedir)
+    sys.path.append(sitedir)
 
 
 from TMDA import Util
----------------------------------------------------------------

Comment 6 Bernard Johnson 2007-02-17 09:20:04 UTC
(In reply to comment #5)
> * Scriptlets
>   - Well, on my environ no one has /etc/ as the home directory.
>     For the user tofmipd, I think the home directory should be
>     %{_sysconfdir}/tmda or %{_sysconfdir}/tofmipd with the group
>     of the directory set as tofmipd.

Can we agree that the actual home directory of the daemon does not matter as
long as:
1) The directory in not on a mounted filesystem (other than /)
2) It is not a privileged directory (since the program initially runs as root)

With that in mind, I really do not want to create an empty directory to just
have a home directory for the daemon.  In my mind, it doens't really matter
outside of that.  I wanted /tmp but rpmlint complains for an entirely different
reason.

Seems to me, the next best choice is /etc or even /.  At least for now, I've
changed it to /.

On my system, the following daemons (users) use / as their home directory:
nobody, dbus, avahi, rpc, nscd, haldaemon


> ----------------------------------------------
> Requires(post): /sbin/chkconfig
> Requires(preun):/sbin/chkconfig
> Requires(pre):  fedora-usermgmt
> Requires(postun):fedora-usermgmt
> ----------------------------------------------
>   - All these are not needed for main package. These are needed
>     by -tofmipd package.

Fixed that for next release.

>   - By the way, why do you use the mixed use of
> ----------------------------------------------
>   /sbin/service tofmipd stop &> /dev/null || :
>   %{_initrddir}/tofmipd condrestart &> /dev/null || :
> ----------------------------------------------
>     (i.e. use of /sbin/service v.s. directly calling
>           scripts under %{_initrddir} ) ?
>     On Fedora, the use of /sbin/service seems to be recommended,
>     and Requires(....): /sbin/service is needed.

It was my mistake.  I had intended to not use service at all as it provides no
additional functionality and adds more requirements to the package.

Unless you have a strong argument for it, I would rather not use service.

> * Documentation
>   - Check if the document "INSTALL" is needed.

It did have a few pointers to documentation, but one of them was wrong, so I
choose to not install it.

Everything in the file can be found elsewhere.

>   On FC7 i386, "service tofmipd start" fails 100% as following:
> ImportError: No module named mime

This was caused by a packaging error which I've fixed.  I also added a note to
the spec file explaining why the package contains pythonlib

Spec URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda.spec
SRPM URL:
http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda-1.1.10-2.fc6.src.rpm

* Sat Feb 17 2007 Bernard Johnson <bjohnson> 1.1.10-2
- consistent start/stop of daemon in scriptlets
- move requirements created by scripts to daemon package that requires them
- don't include the INSTALL file - it's information can be found elsewhere
- %%{python_sitelib}/TMDA/pythonlib/email/mime files were improperly packaged
- dependency on initscripts because of use of daemon function
- note regarding tmda inclusion of pythonlib/email
- change tofmipd user to / home directory


Comment 7 Mamoru TASAKA 2007-02-17 13:45:06 UTC
Well, 
(In reply to comment #6)
> (In reply to comment #5)
> > * Scriptlets
> Seems to me, the next best choice is /etc or even /.  At least for now, I've
> changed it to /.
Okay.

Almost clean.  For -2:
* Version/Release specific dependency:
  - Usually the dependency against main package should be release specific.
    i.e.
-------------------------------------------------------------
Requires:       tmda = %{version}-%{release}
-------------------------------------------------------------

* Cosmetic issue: consistent macro use
  - Well, you use both
--------------------------------------------------------------
%{_sysconfdir}/rc.d/init.d
%{_initrddir}
--------------------------------------------------------------
   Please unify them.

* Again documentation:
  - Maybe the following documents are useful?
--------------------------------------------------------------
NEWS (usually this should be included)
--------------------------------------------------------------

* For -emacs package:
  - Well, I don't think it is useful to split only one file with 27K 
    and to create another package with have no dependency essentially.
    And.. tmda.el is installed in main package as a documentation.

    IMO simply unifying .el file into main package and having both
    %{_datadir}/emacs and %{_datadir}/emacs/site-lisp directories owned
    also by main package, removing emacs dependency is simpler.

Comment 8 Mamoru TASAKA 2007-02-17 15:45:47 UTC
Forgot to check the following..

* python module dependency
  - /usr/bin/tmda-ofmipd contains:
-------------------------------------------------
   346  if remoteauth['proto'] == 'ldap':
   347      try:
   348          import ldap
   349      except ImportError:
   350          raise ImportError, \
   351                'python-ldap (http://python-ldap.sf.net/) required.'
--------------------------------------------------
    Consider to add "Requires: python-ldap"

  - site-packages/TMDA/FilterParser.py contains:
--------------------------------------------------
   724      def __search_cdb(self, pathname, keys, actions, source):
   725          """
   726          Search DJB's constant databases; see <http:/cr.yp.to/cdb.html>.
   727          """
   728          import cdb
   729          cdb = cdb.init(pathname)
   730          found_match = 0
--------------------------------------------------
    Does this package need "python-cdb" (currently not available
    on Fedora)?

Comment 9 Bernard Johnson 2007-02-17 17:57:43 UTC
(In reply to comment #8)
>     Consider to add "Requires: python-ldap"

ldap authentication is optional.  It is one of several types available.  It is
only required if you use it.  Most people use file authentication or rpop/rimap
authentication as they are the simplest to setup.

>     Does this package need "python-cdb" (currently not available
>     on Fedora)?

cdb storage is optional.  Most people will use dbm instead since it is built-in.

Comment 10 Bernard Johnson 2007-02-17 18:37:31 UTC
(In reply to comment #7)
>   - Usually the dependency against main package should be release specific.
>     i.e.

Duh, I knew that :-/

> * Cosmetic issue: consistent macro use
>   - Well, you use both
> %{_sysconfdir}/rc.d/init.d
> %{_initrddir}

Good catch, I will fix that.

>   - Maybe the following documents are useful?
> NEWS (usually this should be included)

I will add this.

> * For -emacs package:
>   - Well, I don't think it is useful to split only one file with 27K 
>     and to create another package with have no dependency essentially.
>     And.. tmda.el is installed in main package as a documentation.
> 
>     IMO simply unifying .el file into main package and having both
>     %{_datadir}/emacs and %{_datadir}/emacs/site-lisp directories owned
>     also by main package, removing emacs dependency is simpler.

Isn't this contrary to the same argument being used against logrotate scripts now?

The packaging guidelines
(http://fedoraproject.org/wiki/Packaging/Guidelines#head-a5931a7372c4a00065713430984fa5875513e6d4),
one file and directory ownership say:

"The rule of thumb is that your package should own all of the directories it
creates except those owned by packages which your package depends on."

If I create a subpackage, it has two beneficial effects:
1) I don't break the rules and make exceptions for my package.
2) I don't drag in emacs dependencies to the main package.

>     And.. tmda.el is installed in main package as a documentation.

And to address this specifically...  Yes, it came from documentation, to add
additional functionality as I did with print{cdb,dbm}.  We always have the
option to just include it in %docs and not pull it in like I do.  What do you
think about that?

Comment 11 Mamoru TASAKA 2007-02-18 06:26:07 UTC
Well, for -emacs subpackage, I respect your judgement.
So please fix other issues I pointed out and reupload 
spec/srpm.

Comment 12 Bernard Johnson 2007-02-18 06:40:11 UTC
Spec URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda.spec
SRPM URL:
http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda-1.1.10-3.fc6.src.rpm

* Sat Feb 17 2007 Bernard Johnson <bjohnson> 1.1.10-3
- added NEWS to %%doc
- macro cleanup
- exact version dependency for subpackage

Comment 13 Bernard Johnson 2007-02-18 10:02:51 UTC
Ooops.  Missed one small item:
Spec URL: http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda.spec
SRPM URL:
http://www.symetrix.com/~bjohnson/projects/Fedora-Extras/tmda-1.1.10-4.fc6.src.rpm


* Sun Feb 18 2007 Bernard Johnson <bjohnson> 1.1.10-4
- missed a %%{version}-%%{release} dependency, fixed it

Comment 14 Mamoru TASAKA 2007-02-18 14:23:57 UTC
Okay.

-----------------------------------------------
  This package (tmda) is APPROVED by me.
-----------------------------------------------
  Please add this package to CVSSyncNeeded and wait
  until cvsadmin creates the initial directories for
  this package.

Comment 15 Bernard Johnson 2007-03-07 18:56:48 UTC
Additional Branches: EL-4, EL-5

Comment 16 Dennis Gilmore 2007-03-07 19:04:09 UTC
EL-4 and EL-5 branches created

Comment 17 Bernard Johnson 2007-03-13 21:54:05 UTC
Perhaps they did not get created correctly.  I can't find them in my checkout
directory, and they don't show up in the viewvc view either.

Comment 18 Warren Togami 2007-03-13 23:24:11 UTC
Not sure what happened before, but I have just now created EL-4 and EL-5 and
added it to owners.epel.list.

Comment 19 Bernard Johnson 2014-08-22 02:38:34 UTC
Package Change Request
======================
Package Name: tmda
New Branches: epel7
Owners: bjohnson

Comment 20 Gwyn Ciesla 2014-08-22 12:05:54 UTC
Git done (by process-git-requests).


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