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.
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.
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?
Yes, this will be nice.
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.
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...
Oops, I forgot to mention, of course the wrapper script should then be called "archivemail" and installed to /usr/bin, chmod 755... :-)
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?
(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.
(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.
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.
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 ;-)
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
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.
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
Good: + Rpmlint quite on binary RPM. *** APPROVED ***
New Package CVS Request ======================= Package Name: archivemail Short Description: A tool for archiving and compressing old email in mailboxes Owners: limb Branches: FC-5 FC-6 F-7 InitialCC: Great, thanks for the review!
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.
cvs done
Imported and built.
Package Change Request ====================== Package Name: archivemail New Branches: EL-4 EL-5
cvs done.