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)
Summary: | Review Request: TMDA - Tagged Message Delivery Agent | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Bernard Johnson <bjohnson> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | mtasaka |
Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-02-20 02:57:10 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Bernard Johnson
2006-11-29 05:46:49 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) (Removing NEEDSPONSOR as bug 216723) 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)
-------------------------------------
(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 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 ---------------------------------------------------------------- (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 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. 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)? (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. (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? Well, for -emacs subpackage, I respect your judgement. So please fix other issues I pointed out and reupload spec/srpm. 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 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 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. Additional Branches: EL-4, EL-5 EL-4 and EL-5 branches created 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. Not sure what happened before, but I have just now created EL-4 and EL-5 and added it to owners.epel.list. Package Change Request ====================== Package Name: tmda New Branches: epel7 Owners: bjohnson Git done (by process-git-requests). |