Bug 476460 (pymilter) - Review Request: pymilter - Python interface to sendmail milter API
Summary: Review Request: pymilter - Python interface to sendmail milter API
Keywords:
Status: CLOSED NOTABUG
Alias: pymilter
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 479325
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2008-12-15 00:39 UTC by Stuart D Gathman
Modified: 2009-04-05 16:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-05 16:45:29 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Stuart D Gathman 2008-12-15 00:39:54 UTC
Spec URL: http://customdesigned.users.sourceforge.net/pymilter.spec
SRPM URL: http://customdesigned.users.sourceforge.net/pymilter-0.9.0-1.fc9.src.rpm
Description: This is a python extension module to enable python scripts to
attach to sendmail's libmilter functionality.  Additional python
modules provide for navigating and modifying MIME parts (adds to email package), and sending DSNs or doing CBVs.

This is the first Fedora package I am submitting myself.  Other packages of mine have been submitted by jafo (Sean Reifschneider).

Comment 1 manuel wolfshant 2008-12-15 05:41:06 UTC
Source0 should follow the guidelines described at https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

Comment 2 Stuart D Gathman 2008-12-16 04:28:13 UTC
Fixed Source0

Comment 3 Mamoru TASAKA 2009-01-02 16:19:08 UTC
Some notes:
- Please remove unneeded macros or conditions which are not related
  to Fedora 
  * especially rh7 age condition is not needed
  * And I don't think aix4.1 part is needed.

- Also please explain why you want to introduce %name, %version or %release
  macro (note that when you write "Version: 0.9.0", the %version macro
  is automatically defined (and same for %name, %release)

- For source tarball hosted by sourceforge.net, please follow
  https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

- Remove "Vendor" item. This is automatically set when rebuiding
  your srpm on Fedora site.

- Your srpm does not build
  * One reason is that "BuildRequires: ed" is missing
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1029717
  * The second reason is that "%_libdir/libmilter.a" (in sendmail-devel)
    is not compiled with -fPIC. So either
    - C module support for this package should be dropped
    - Or you should file a bug against "sendmail" to compile
      libmilter.a with -fPIC (I guess filing a bug is better
      anyway)

- "%files -f INSTALLED_FILES" in your method is not allowed on
  Fedora, because with this method all needed directories under
  python sitelib/sitearch directory are not owned correctly:
  https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories

- We now recommend %defattr(-,root,root,-)

- The directory %libdir is not owned by this package
  (again please refer to "UnownedDirectories" wiki page)

- "%config" file should be under %_sysconfdir (/etc) and files
  under %_libdir should not be marked as %config.

Comment 4 Stuart D Gathman 2009-01-08 03:47:25 UTC
- Removed age support (Note, I still support aix4.1 and RH7.3, but can keep those off the main trunk)

- I like to put macros like %version at the top where they are easily changed.  However, that is not needed in this case - so they are now removed.

- I did follow the sourceforge hosting instructions.  Oh - Fedora doesn't like the sf.net shortcut.  Fixed.

 * added BuildRequires: ed
 * the whole point of this package is the C module.  It builds for me on Fedora 9 (and RH5.2 and RH7.3 and AIX4.1).  I guess I need to learn to use this koji thing now to see the problem for myself.  I followed the koji link you provided, and it is for a mrpt package, not pymilter.  However, I see that the failure is for Fedora 11, so I assume that the build failed for Fedora 11.  This is almost certainly due to libmilter.a.  I've had to file that bug for RH73, and Centos4 and Centos5.  I guess it isn't obvious to the sendmail packagers that libmilter is very often used in dynamically loaded modules (for Perl, Python, and other scripting languages).  But I can only test on F9 at the moment.

- I added %dir %{python_sitelib}/Milter and %dir %{libdir} so that directories under sitelib are owned correctly.

- Hmm, the "start.sh" file is not a config file per se.  It is a glue file that it is sometimes useful for an admin to be able to modify and not have updates wipe out his changes.  It really doesn't belong in etc.  If it is really a problem, I will simply remove the %config tag for Fedora.  Note that this package doesn't have any config files, and the start.sh glue file is for the convenience of applications that use the pymilter library.  What is really needed is a python module to "run as a service" with pid file, etc.

New SRPM is:
http://customdesigned.users.sourceforge.net/pymilter-0.9.0-2.fc9.src.rpm

Comment 5 Mamoru TASAKA 2009-01-08 16:58:25 UTC
(In reply to comment #4)
>  * the whole point of this package is the C module.  It builds for me on Fedora
> 9 (and RH5.2 and RH7.3 and AIX4.1).  I guess I need to learn to use this koji
> thing now to see the problem for myself.  

> I followed the koji link you
> provided, and it is for a mrpt package, not pymilter.  

Oops, sorry. your -2 srpm fails to build due to the same reason:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1040488

> However, I see that the
> failure is for Fedora 11, so I assume that the build failed for Fedora 11. 

Also fails on F-10:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1040512

> This is almost certainly due to libmilter.a.  I've had to file that bug for
> RH73, and Centos4 and Centos5.  I guess it isn't obvious to the sendmail
> packagers that libmilter is very often used in dynamically loaded modules (for
> Perl, Python, and other scripting languages).  But I can only test on F9 at the
> moment.

So would you file a new bug against sendmain about -fPIC issue
on libmilter.a (and post this bug a number you filed)?

Comment 6 Stuart D Gathman 2009-01-08 20:46:22 UTC
Looking at the logs for F10 and F11, it *does* build for i386.  It is failing for x86_64.  So apparently, only the x86_64 libmilter.a is missing the -fPIC.  That makes more sense, and explains why "It Works for Me"(tm).

Comment 7 Stuart D Gathman 2009-01-08 21:06:04 UTC
Reported bug for sendmail component:
https://bugzilla.redhat.com/show_bug.cgi?id=479325

Comment 8 Mamoru TASAKA 2009-01-09 14:57:28 UTC
For now I want to wait for the response from sendmail
maintainer.

Comment 9 Stuart D Gathman 2009-01-22 04:25:39 UTC
Removed -lsmutil with patch.  (Some systems need it.)

New SRPM:
http://customdesigned.users.sourceforge.net/pymilter-0.9.0-3.fc9.src.rpm

Comment 10 Mamoru TASAKA 2009-01-23 08:20:11 UTC
For 0.9.0-3:

* Re-definig %__python
  - Would you explain why you redefine %__python?
    (On Fedora %__python is defined as /usr/bin/python).

* Seemingly redundant version specific dependency
  - python 2.4 is introduced on Fedora Core 4
  - sendmail 8.13.1 is already on Fedora Core 3
    I think Requires: python ">= 2.4", sendmail ">= 8.13" is
    now just redundant.

* Macros
  - Use macros for some basic directories.
    https://fedoraproject.org/wiki/Packaging/RPMMacros
    /var should be %{_localstatedir}

* Using INSTALLED_FILES
  - Your way of using INSTALLED_FILES is still unsafe. It
    will cause unowned directories when pymilter comes
    to create some subdirectories under %_libdir or
    %python_sitearch/Milter .

    Instead there is much simpler and safer way like:
------------------------------------------------------
%files
%defattr(-,root,root,-)
%doc README ...
%{python_sitearch}/Milter/
%{libdir}/
%dir %attr(0755,mail,mail) /var/run/milter
------------------------------------------------------
    ! Note
      For example, %{python_sitearch}/Milter/ contains the directory
      %{python_sitearch}/Milter/ itself and all files/directories/etc
      under this directory.
    ! By the way this package uses %python_sitearch not %python_sitelib
      so currently rebuild fails on x86_64, ppc64.

* No config file
  - Files under %_libdir (for this package start.sh) should not be 
    marked as %config
    (If this file really should be marked as %config, put the file
     under %_sysconfdir and use symlink).

? Seemingly missing directory
  - It seems that %libdir/start.sh needs %{_localstatedir}/log/milter
    but it is uncertain which rpm owns this directory
    (at least yum returns currently no packages in Fedora owns
    this directory).
    Should this directory be owned by this package? Or is it expected
    that this directory already exists? (in such case which rpm
    will create this directory?)

! %changelog format
  - I recommend that one line is put between each %changelog entry
    like:
----------------------------------------------------
%changelog
* Wed Jan 07 2009 Stuart Gathman <stuart> 0.9.0-2
- Changes to meet Fedora standards

* Mon Nov 24 2008 Stuart Gathman <stuart> 0.9.0-1
- Split pymilter into its own CVS module
- Support chgfrom and addrcpt_par
- Support NS records in Milter.dns

* Mon Aug 25 2008 Stuart Gathman <stuart> 0.8.10-2
- /var/run/milter directory must be owned by mail
----------------------------------------------------
    This is useful on Fedora CVS for some reason.

Comment 11 Stuart D Gathman 2009-01-24 17:47:07 UTC
Ok, I'm going to jettison start.sh. /var/run/milter, /var/log/milter and create a milter-base package for the python milters that use it.  Is there a better way than having a package own one tiny shell script and two directories?

That will remove those objections to pymilter (and start.sh truly is cruft if you don't use it).

I have already cut support for AIX and RH73, but I *must* support EL3 - EL5 because that is where all the production milters run.  (I have one more RH73 system that is being migrated to EL4 as I speak.  Truly a golden version.)  I could make a fedora version of the spec file - but I don't want to maintain both (not so bad for just pymilter, but if more packages are accepted...).  I know!  A preprocessor to convert a spec file template to fedora specific and EL specific versions!  It could have features like %if, %define, oh wait...  

I define python because I need to build pymilter for python versions other than the system default.  For instance, on EL4, the system python is 2.3, but the production milter requires python2.4.  Both python and python modules are easily built to coexist in multiple versions.  (Of course, that argues for cutting start.sh again - so it's history.)  Would it be acceptable if an %ifdef excludes the python redefinition for fedora?

The version dependency is only redundant if building only for Fedora.  Would it be acceptable if I %ifdef away the dependency for Fedora?

Will use %{localstatedir}

start.sh and associated directories will be cut

Will reformat %changelog

Comment 12 Mamoru TASAKA 2009-01-25 06:22:32 UTC
(In reply to comment #11)
> Ok, I'm going to jettison start.sh. /var/run/milter, /var/log/milter and create
> a milter-base package for the python milters that use it.  Is there a better
> way than having a package own one tiny shell script and two directories?
> 
> That will remove those objections to pymilter (and start.sh truly is cruft if
> you don't use it).

- Well, are /var/run/milter and /var/log/milter directories supposed to be
  used also by other applications than pymilter?

  On Fedora there are already some srpms named *milter*. Would
  you know why none of these packages own these two directories?
  If you are unsure, I think it is better that these two directories
  should be owned by pymilter.
  

> I define python because I need to build pymilter for python versions other than
> the system default.  For instance, on EL4, the system python is 2.3, but the
> production milter requires python2.4.  Both python and python modules are
> easily built to coexist in multiple versions.  (Of course, that argues for
> cutting start.sh again - so it's history.)  Would it be acceptable if an %ifdef
> excludes the python redefinition for fedora?
> 
> The version dependency is only redundant if building only for Fedora.  Would it
> be acceptable if I %ifdef away the dependency for Fedora?

- If you want to maintain pymilter also on EL-4, okay.

Comment 13 Stuart D Gathman 2009-01-27 00:17:37 UTC
> - Well, are /var/run/milter and /var/log/milter directories supposed to be
>   used also by other applications than pymilter?

The /var/run/milter, /var/log/milter, /usr/lib/pymilter directories and start.sh are not used at all by pymilter.  They are simply there so that some package can own them, and since all python milters use pymilter, it made sense to put them there.  However, not all python milters use those directories, so maybe it doesn't make sense.

>   On Fedora there are already some srpms named *milter*. Would
>   you know why none of these packages own these two directories?
>   If you are unsure, I think it is better that these two directories
>   should be owned by pymilter.

Milters can be written in any language - C being the most popular.  Only
milters written in Python use pymilter.  And even those don't necessarily use
the log and run directories provided.

Now I am leaning toward removing the %config flag from start.sh and leaving
things the way they are otherwise.

Comment 14 Stuart D Gathman 2009-01-27 00:21:03 UTC
It just occurred to me that maybe /var/run/milter should be renamed to /var/run/pymilter and likewise for /var/log/milter.  But since no other package has claimed them, maybe I won't (since it would involve changes to my dependent packages).

Comment 15 Stuart D Gathman 2009-01-27 02:34:25 UTC
Kept start.sh and dirs, but removed config flag.
Removed INSTALLED_FILES
Spaced changelog
Two changes for RHEL controlled by RHEL flag (redefine __python and require minimum python/sendmail versions).

New SRPM:
http://customdesigned.users.sourceforge.net/pymilter-0.9.0-3.fc9.src.rpm

Comment 16 Stuart D Gathman 2009-01-27 02:35:18 UTC
Kept start.sh and dirs, but removed config flag.
Removed INSTALLED_FILES
Spaced changelog
Two changes for RHEL controlled by RHEL flag (redefine __python and require minimum python/sendmail versions).

New SRPM:
http://customdesigned.users.sourceforge.net/pymilter-0.9.0-4.fc9.src.rpm

Comment 17 Mamoru TASAKA 2009-01-29 17:34:33 UTC
For 0.9.0-4

* Unused macros
  - It seems that %python_sitelib is not used.

* Fedora/RHEL conditional
  - There are some useful macros, see:
    https://fedoraproject.org/wiki/Packaging/DistTag#Conditionals
    For example:
---------------------------------------------------------
%if 0%{?el3} || 0%{?el4}
%define __python python2.4
%endif
---------------------------------------------------------

* About version dependency
  - If you are to support both RHEL and Fedora by one spec file,
    then
----------------------------------------------------------
Requires: %{pythonbase} >= 2.4, sendmail >= 8.13
BuildRequires: ed, %{pythonbase}-devel >= 2.4, sendmail-devel >= 8.13
----------------------------------------------------------
    is sufficient
    (The reason I wrote that version dependency is redundant was
     that it is redundant just to support Fedora)

* python module dependency
  - Would you check if all needed dependencies related to python
    modules are included in Requires:
    For example, Milter/dns.py contains:
----------------------------------------------------------
     3  import DNS
     4  from DNS import DNSError
----------------------------------------------------------
    It seems this package should have "Requires: python-pydns".
    Also python-spf may be needed.

* Macros in %changelog
  - $ rpm -q --changelog pymilter shows:
----------------------------------------------------------
* Thu Jan 08 2009 Stuart Gathman <stuart> 0.9.0-4
- Stop using INSTALLED_FILES to make Fedora happy
- Remove config flag from start.sh glue
- Own /var/log/milter
- Use /var <--------------------
----------------------------------------------------------
    that macros are expanded in %changelog. To prevent this,
    use %% (instead of %) in %changelog.

Comment 18 Stuart D Gathman 2009-02-02 03:40:23 UTC
The python-pydns is an optional dependency.  The dns module is often handy for milters that need to lookup MX records, for callback, for instance - but is not part of the core functionality.  Debian packages would have an optional dependency.  In milter applications, missing optional packages result in smoothly degraded function.  A user of pymilter, would get a package not found exception if they tried to use Milter/dns.  I am torn, I really ought to put the simplified wrapper for pydns (Milter/dns) in pydns - but was reluctant to do so, wanting to minimize changes to pydns.  But now that I am the maintainer of pydns, perhaps that is the best way.  Milters could also use dnspython - a larger and more capable DNS library for python.  In fact, pyspf-2.1 can use either library.

So how does Fedora handle optional packages?  Just add the dependency, and who cares if a few extra K for pydns is pulled in?  I have a feeling it is either that or move the Milter/dns module to pydns.  However, that does not solve the larger problem of optional or alternate libraries - which can crop up in any potentially dynamically linked language, including C.

%python_sitelib  - I am not clear when to use python-sitelib vs python-archlib.  I am hearing however that the policy is to remove all unused macros - and resurrect them from CVS if needed.

I will fix that and the other issues, and will think about whether to remove Milter/dns or just add the dependency.

Comment 19 Mamoru TASAKA 2009-02-02 14:27:00 UTC
For optional dependency:
- I don't know how debian deals with dependency, however as
  rpm does not have Requires(hint) or so, package maintainer must
  judge whether to add such optional dependency to explicit Requires
  or not.
  Fedora's default policy is to support as much functionality
  in the software as possible (and usually it tends to add more
  dependency to the software). However the actual judgment is left
  to the maintainer, so if you have some reason you don't want to
  add python-pydns to explicit Requires, I don't object against it
  (but please write a brief comment in pymilter spec file)

Comment 20 Stuart D Gathman 2009-02-03 22:07:28 UTC
While testing install on EL5 with SELinux enabled, I found that this is required:

# chcon -t textrel_shlib_t '/usr/lib/python2.4/site-packages/milter.so'

Should I go ahead and add that to %post processing?

Comment 21 Mamoru TASAKA 2009-02-04 16:49:15 UTC
I am not familiar with selinux, however I guess it is better 
that you ask Dan Walsh how to deal with this (maybe policy will
be added in selinux-policy?) after this is approved.

Comment 22 Mamoru TASAKA 2009-02-05 18:44:41 UTC
I will wait for your new srpm with the issue I commented
on comment 17 resolved.

Comment 23 Stuart D Gathman 2009-02-06 02:18:31 UTC
Used the 0%{?el4} convention.  Since dns is needed for callback support, and callback are a highly useful function for milters, I decided to include the python-pydns dependency.  For RHEL < 5 pydns in required instead, since python-pydns is always built for the system python.  (Should we have a similar convention for python-pymilter vs pymilter?)  Removed the % from %changelog. Removed the unused sitelib macro.  Did not address the selinux issue.  Could be a python-devel issue that is resolved in Fedora anyway.  Will have to wait until the actual python milter applications are released for Fedora to test.

http://customdesigned.users.sourceforge.net/pymilter-0.9.0-5.fc9.src.rpm

Comment 24 Stuart D Gathman 2009-02-06 05:13:19 UTC
Incorporated a bug fix.

http://customdesigned.users.sourceforge.net/pymilter-0.9.1-1.fc9.src.rpm

Comment 25 Mamoru TASAKA 2009-02-06 15:54:45 UTC
Okay, now this package itself is good. So:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 26 Mamoru TASAKA 2009-02-13 15:46:36 UTC
ping?

Comment 27 Stuart D Gathman 2009-02-14 00:27:04 UTC
Swamped at my paying job.  Debating whether to try and submit milter/milter-spf [http://bmsi.com/python/milter.html] as  my test (it would be nice to have a package that actually *uses* pymilter as part of Fedora).  The problem is, I already know they don't currently meet Fedora packaging guidelines.  I suppose if I "flunk" I can keep trying.  I would also need to upgrade my home server to Fedora 10 to test running everything under Fedora.  (Currently runs in production on EL3, EL4, EL5).

Comment 28 Mamoru TASAKA 2009-02-15 06:21:18 UTC
I guess trying pre-review is much easier.

Comment 29 Mamoru TASAKA 2009-03-01 16:00:49 UTC
ping again?

Comment 30 Stuart D Gathman 2009-03-04 17:14:59 UTC
Taking some time now to try reviewing.

Comment 31 Mamoru TASAKA 2009-03-26 14:24:46 UTC
ping again?

Comment 32 Stuart D Gathman 2009-03-31 14:17:33 UTC
Server at stuart still pinging, but thrashing under heavy load.

Comment 33 Mamoru TASAKA 2009-04-04 16:28:40 UTC
Actually no progress for more than one month.

Usually under situation the review request is closed as
NOTABUG so that other person can take over the package.
Would you have time for review request _now_ ? If not,
would you consider to once close this bug?

Comment 34 Stuart D Gathman 2009-04-04 18:22:27 UTC
Yes, if someone wants to take over the package fine.  I will keep the Fedora specs in CVS, and plug away at additional packages.

Comment 35 Mamoru TASAKA 2009-04-05 16:45:29 UTC
Okay, then once close this bug. Please feel free to submit
a new review request when you have some time.


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