Bug 240877 - Review Request: archivemail - A tool for archiving and compressing old email in mailboxes
Review Request: archivemail - A tool for archiving and compressing old email ...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-22 11:39 EDT by Gwyn Ciesla
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-25 09:06:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jochen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Example of proposed modifications to the archivemail package (2.68 KB, application/x-gzip)
2007-05-23 10:17 EDT, Francois Aucamp
no flags Details

  None (edit)
Description Gwyn Ciesla 2007-05-22 11:39:34 EDT
Spec URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail-0.7.0-1.src.rpm
Description: 
Archivemail is a tool written in Python for archiving and compressing old
email in mailboxes. It can move messages older than the specified number of
days to a separate mbox format mailbox that is compressed with gzip, or
optionally just delete old email.
Comment 1 Jochen Schmitt 2007-05-22 13:20:30 EDT
Good:
+ Naming seems ok.
+ License seems ok.
+ Tar ball matches with upstream
+ Rpmlint is quite on source package
+ Rpmlint is quite on binary package
+ Pakcage contains verbain copy of the license text
+ Mock build works fine.


Bad:
- Strangs Build stanza:
You make a 'chmod 0644' on setup.py and test_archivmail.py, but you didn't call
any of this python scripts.
- Calling of the text script shows following error message:
python test_archivemail.py
The archivemail script needs to be called 'archivemail.py'
and should be in the current directory in order to be imported
and tested. Sorry.


Comment 2 Gwyn Ciesla 2007-05-22 13:26:58 EDT
I've chmod-ed those scripts because they're part of the upstream tarball but are
extraneous in an rpm install, and rpmlint complained about unnecessarily
executable docs.  Should I provide a patch to test_archivemail.py so that it can
be used as-is after the rpm is installed?
Comment 3 Jochen Schmitt 2007-05-22 13:35:36 EDT
Yes, this will be nice.
Comment 4 Gwyn Ciesla 2007-05-22 13:51:04 EDT
Hmm, my python-fu has been defeated.  What if, instead, I put
test_archivemail.py in /usr/bin alongside archivemail and ln -s archivemail
archivemail.py?  It makes test_archivemail.py happy.
Comment 5 Francois Aucamp 2007-05-22 15:20:34 EDT
How about putting installing the "archivemail" file as
/usr/share/archivemail/archivemail.py (chmod 644), then writing a small Python
wrapper script to import that module and run the main() method? Something like:

------------
#!/usr/bin/python
if __name__ == '__main__':
    import sys
    sys.path.insert(0, '/usr/share/archivemail')
    from archivemail import main
    main()
------------

Not only will this solve your test_archivemail.py problem (with _little_
tweaking), it doesn't clutter up /usr/bin and would actually give you a (very)
small performance boost as the Python interpreter will only parse and process
the text of your small startup script but will use the precompiled byte-code
.pyc file of archivemail, rather than having to re-parse this large file every
time it's executed. 

Also, is it actually necessary to package the project's unit tests in the final
binary RPM? If I wanted that, i would go for the src.rpm, but maybe that's just
me...
Comment 6 Francois Aucamp 2007-05-22 15:24:23 EDT
Oops, I forgot to mention, of course the wrapper script should then be called
"archivemail" and installed to /usr/bin, chmod 755... :-)
Comment 7 Gwyn Ciesla 2007-05-23 09:01:20 EDT
That didn't work, it said it needed archivemail to be called archivemail.py. 
When I renamed it that, it still failed, with a whole slew of errors.

Spec URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail-0.7.0-1.src.rpm

Maybe it'd be better just to put the test_archivemail in /usr/bin with
archivemail.py and be done with it?
Comment 8 Francois Aucamp 2007-05-23 09:48:57 EDT
(In reply to comment #7)
> That didn't work, it said it needed archivemail to be called archivemail.py. 
> When I renamed it that, it still failed, with a whole slew of errors.

It works just fine.

Your "slew of errors" all have to do with the file location and execution
permission of the archivemail.py file. Comment #5 was just an _example_. You
still need to patch both archivemail.py (one-liner) and test_archive.py
(one-liner + simple sed substitution) to work with these new file locations and
permissions. I'll attach example patches shortly.

> Spec URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail.spec
> SRPM URL:
http://zanoni.jcomserv.net/fedora/archivemail/archivemail-0.7.0-1.src.rpm
> 
> Maybe it'd be better just to put the test_archivemail in /usr/bin with
> archivemail.py and be done with it?

No. One of the main tasks of a package maintainer is to make sure their packages
integrate well with the rest of the distribution; anyone armed with google can
roll their own RPMs, but that does not make them Fedora maintainers. Cluttering
up /usr/bin with pointless symlinks is bad.

Also, in general your average end-user isn't interested in unittests; I'm not
even convinced they should be shipped in the binary RPM... but putting them
under %docs at least gives intereste people (aka developers) the option of using
them, I suppose.
Comment 9 Gwyn Ciesla 2007-05-23 10:02:50 EDT
(In reply to comment #8)

> It works just fine.

Ah, I misunderstood.  I told you, my python is not nearly so developed as my PHP
or even my C++. :)

> 
> No. One of the main tasks of a package maintainer is to make sure their packages
> integrate well with the rest of the distribution; anyone armed with google can
> roll their own RPMs, but that does not make them Fedora maintainers. Cluttering
> up /usr/bin with pointless symlinks is bad.

Agreed, and I was not proposing to use symlinks, I was proposing to move the
test_ script there directly.

> Also, in general your average end-user isn't interested in unittests; I'm not
> even convinced they should be shipped in the binary RPM... but putting them
> under %docs at least gives intereste people (aka developers) the option of using
> them, I suppose.

Which is why I put it in %doc in the first place (which would still be my
preference).  I was thinking I could put it in put with a note detailing how a
user could get the test script to work, namely copy it and the main script to
the same place and run.  Given the Python shipped in Fedora, it should be
working from the get-go once the rpm is installed.  Hence my use of the word
'extraneous in Comment #2.
Comment 10 Francois Aucamp 2007-05-23 10:17:19 EDT
Created attachment 155242 [details]
Example of proposed modifications to the archivemail package

Ok, I've created a tar.gz file containing an example archivemail startup
script, a patch for test_archivemail.py, and the spec file that ties all this
stuff together, heavily commented (based on your original spec file).

The resulting RPM install and runs fine. You can use the command "python
/usr/share/doc/archivemail-0.7.0/test_archivemail.py" to run the unit tests.
Comment 11 Francois Aucamp 2007-05-23 10:31:32 EDT
Mid-air collision! :-)

(In reply to comment #9)
> Agreed, and I was not proposing to use symlinks, I was proposing to move the
> test_ script there directly.

Fair enough, but I don't think its necessary to expose the unittests to all
end-users; to put in bluntly: they don't do anything useful[1]

> Which is why I put it in %doc in the first place (which would still be my
> preference). 

Mine too, in this case. :-)

> I was thinking I could put it in put with a note detailing how a
> user could get the test script to work, namely copy it and the main script to
> the same place and run.  

Yes, very good idea. But if you apply my proposed changes, no copying/renaming
is required, only a simple Readme.tests (or whatever) saying pretty much what I
said in the last paragraph of comment #10.

> Given the Python shipped in Fedora, it should be
> working from the get-go once the rpm is installed.  Hence my use of the word
> 'extraneous in Comment #2.

Yes. Btw, I also removed some unecessary %doc entries (maifest, setup.py, etc) -
these are required for application installation, which is RPM's job.


Jochen: By the way, are you reviewing this package? If so, please assign the bug
to yourself to avoid confusion (I noticed the flag when I was about to do
this... :-)  )


[1] except if you are debugging/developing, which normal users of the app won't
be doing ;-)
Comment 12 Gwyn Ciesla 2007-05-23 11:17:17 EDT
Very nice.  I added the readme and fixed a permissions issue with the new source
file.  The only rpmlint warning is about the test script not being executable,
which is expected.

Spec URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail-0.7.0-3.src.rpm
Comment 13 Jochen Schmitt 2007-05-23 16:31:30 EDT
Good:
+ Testscript run fine.
+ Local install and uninstall works fine.

Bad:
- Script in %{_docdir} contains a shebang line at the first line. This coused
the rpmlint meessage you refered. So please remove this shebang line from the
script.
Comment 14 Gwyn Ciesla 2007-05-24 07:51:10 EDT
Complains about the shebang in /usr/share/archivemail/archivemail.py, fixed that
too.
Spec URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/archivemail/archivemail-0.7.0-4.src.rpm
Comment 15 Jochen Schmitt 2007-05-24 10:17:04 EDT
Good:
+ Rpmlint quite on binary RPM.

*** APPROVED ***
Comment 16 Gwyn Ciesla 2007-05-24 10:21:56 EDT
New Package CVS Request
=======================
Package Name: archivemail
Short Description: A tool for archiving and compressing old email in mailboxes
Owners: limb@jcomserv.net
Branches: FC-5 FC-6 F-7
InitialCC: 

Great, thanks for the review!
Comment 17 Gwyn Ciesla 2007-05-24 10:43:45 EDT
FYI, upstream bug report:
http://sourceforge.net/tracker/index.php?func=detail&aid=1670422&group_id=49630&atid=456910
I have included this patch for the initial import.  Introduces no problems.
Comment 18 Tom "spot" Callaway 2007-05-24 11:13:14 EDT
cvs done
Comment 19 Gwyn Ciesla 2007-05-25 09:06:45 EDT
Imported and built.
Comment 20 Gwyn Ciesla 2007-07-12 11:22:27 EDT
Package Change Request
======================
Package Name: archivemail
New Branches: EL-4 EL-5
Comment 21 Kevin Fenzi 2007-07-12 13:19:03 EDT
cvs done.

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