[Bug 476460] Review Request: pymilter - Python interface to sendmail milter API

[bugzilla at redhat.com]

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


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


Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody at fedoraproject.org    |mtasaka at ioa.s.u-tokyo.ac.jp
               Flag|                            |fedora-review?




--- Comment #10 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp>  2009-01-23 03:20:11 EDT ---
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 at bmsi.com> 0.9.0-2
- Changes to meet Fedora standards

* Mon Nov 24 2008 Stuart Gathman <stuart at bmsi.com> 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 at bmsi.com> 0.8.10-2
- /var/run/milter directory must be owned by mail
----------------------------------------------------
    This is useful on Fedora CVS for some reason.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.