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=586777 Toshio Ernie Kuratomi <a.badger@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |a.badger@xxxxxxxxx AssignedTo|nobody@xxxxxxxxxxxxxxxxx |a.badger@xxxxxxxxx Flag| |fedora-review? --- Comment #9 from Toshio Ernie Kuratomi <a.badger@xxxxxxxxx> 2010-05-07 11:12:54 EDT --- I'll trade for python-bunch: https://bugzilla.redhat.com/show_bug.cgi?id=575185 GOOD: * Source matches upstream * builds in koji for f13 --> See notes in NEEDSWORK for other releases * license is correct and approved * spec file is readable * No locale files to handle * not a shared library * No bundled libraries * package owns all files and directories it creates and nothing else * no file listed multiple times in %files * Permissions are proper * macros used consistently * no doc files affect runtime NEEDSWORK * This is a post-release snapshot so you have the release almost correct but it should include the date like this:: 1.$DATEbzr$BZRbzr%{?dist} So like this:: 2.20100507bzr73 * Better method to generate the source:: ### Source tarball is created from latest 73 revision ### bzr branch lp:glyphtracer -r 73 ### cd glyphtracer ### python setup.py sdist ### tarball is in dist/glyphtracer-%{version}.tar.gz Source0: glyphtracer-%{version}.tar.gz The reason this is better is that this is the way most upstream's create their tarballs. So making our tarball this way will let us see problems they would when they make their next release. (And hello jpakkane! In case you didn't know about the setup.py sdist command, there it is :-) When making this change, you also need to change the %setup line: - %setup -q -n %{name} + %setup -q glyphtracer-%{version} * Need to Requires: potrace * If you are building for anything less recent than F13 you need to define python_sitelib at the top of your spec file like this:: %if ! (0%{?fedora} > 12 || 0%{?rhel} > 5) %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")} %endif * This is a GUI app so it needs a .desktop file. RPMLINT: glyphtracer.src: W: spelling-error %description -l en_US vectorised -> vectored, vector, verdigrised glyphtracer.noarch: W: spelling-error %description -l en_US vectorised -> vectored, vector, verdigrised Both vectorize and vectorise are in common use. No need to change this. glyphtracer.src: W: invalid-url Source0: glyphtracer-1.0-73bzr.tar.gz The comments explain how to generate this so it's also fine. glyphtracer.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/glyphtracer.py 0644 /usr/bin/python glyphtracer.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/gtlib.py 0644 /usr/bin/python These are being generated because of the #!/usr/bin/python lines in gtlib.py and glyphtracer.py. I'd remove the line from gtlib.py and submit upstream because there is no code there intended to be run as a script. glyphtracer.py is tougher -- it does have an if __name__ == '__main__' section that allows you to execute the program as a script. Several ways to resolve this: * Decide that glyphtracer.py isn't useful as a library. In that case i'd add it to the setup.py as a script and install it to /usr/bin. Renaming glyphtracer.py to glyphtracer and making this change to setup.py would do that:: - py_modules = ['glyphtracer', 'gtlib'], + py_modules = ['gtlib'], + scripts = ['glyphtracer'], * Decide that in the future glyphtracer.py will be used as a library too -- I'd remove the if __name__ == '__main__' section from glyphtracer.py and add the wrapper script (present in the srpm) to the upstream distribution in the scripts line of setup.py. OTHER (none of these are necessary changes): * Note for jpakkane: usually, people include a copy of the GPL (v3 since that's what you are licensing under) in the tarball. The funny thing about the GPL is that if the software author doesn't specify a version explicitly anywhere, the recipient is officially allowed to use any version of the license (even though the author likely meant GPLv3 or later or GPLv2 or later). So your explitness in license.txt is also good to have. Thanks for that! I'd recommend keeping license.txt and adding a COPYING file with the text of the GPLv3. * -O1 isn't needed with any current versions of rpm in Fedora; the python bytecompilation step will do -O1 for us. * When writing python scripts I like to use:: #!/usr/bin/python -tt The -tt makes it illegal to mix spaces and tabs in a single source file. That's nice to do since different people have different settings for width of tabs. * The package may be misnamed. Now that glyphtracer is being used as a library we may want to name the package python-glyphtracer. This is somewhat vague though. Reading the source, it looks like glyphtracer.py is primarily for supporting /usr/bin glyphtracer... it would be hard to implement a different front end using the classes and methods that glyphtracer.py provides. With that in mind naming the package glyphtracer is fine since it's primary purpose is to support the glyphtracer application. If gtlib.py (or glyphtracer.py with API changes) becomes used by other python programs we may want to rethink that at that time. * Should: no man page for glyphtracer -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review