Bug 586777 - Review Request: glyphtracer - Program for creating fonts from images
Review Request: glyphtracer - Program for creating fonts from images
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Toshio Ernie Kuratomi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-28 07:04 EDT by Parag Nemade
Modified: 2010-05-24 15:39 EDT (History)
5 users (show)

See Also:
Fixed In Version: glyphtracer-1.1-1.20100510bzr76.fc12
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-05-24 15:35:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
a.badger: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)
spec file update taking care of remaining must issues (2.88 KB, patch)
2010-05-10 00:05 EDT, Toshio Ernie Kuratomi
no flags Details | Diff

  None (edit)
Description Parag Nemade 2010-04-28 07:04:41 EDT
Spec URL: http://paragn.fedorapeople.org/fedora-work/SPECS/glyphtracer.spec
SRPM URL: http://paragn.fedorapeople.org/fedora-work/SRPMS/glyphtracer-0-0.1.r68.fc13.src.rpm
Description: Glyphtracer takes an image of letters. It detects all letter forms
and allows the user to tag them.
They are then vectorised and passed on to Fontforge for fine tuning.
Comment 1 Thomas Spura 2010-04-28 07:27:46 EDT
As already discussed on IRC, it would be better to make a library here:

I think this could be usefull for other packages too, so other could simply import this and then work with it.

So you could install it in %{python3_sitelib} and set up a starter script (I'm sure upstream will incorporate it).

e.g.:

#!/usr/bin/python3
from glyphtracer import start_program

start_program()


(Currently untested, but should work)

(By the way, is this python3 or python2, you BR python3, but it starts with python2...)
Comment 2 Parag Nemade 2010-04-29 04:14:01 EDT
Thanks Che and Tomspur for your feedback.

I have updated package now
Spec URL: http://paragn.fedorapeople.org/fedora-work/SPECS/glyphtracer.spec
SRPM URL:
http://paragn.fedorapeople.org/fedora-work/SRPMS/glyphtracer-0-0.1.r69.fc13.src.rpm

Can anyone please review this package?
Comment 3 jpakkane 2010-04-29 17:44:03 EDT
The spec file should have a dependency on potrace. Also, there is an actual 1.0 release, you should probably use that rather than a bzr checkout.
Comment 4 Parag Nemade 2010-04-30 02:50:23 EDT
jpakkane,
   Thanks I have used upstream released 1.0 release. I also have request if you can add setup.py in upstream then it will be easy for installation of this program.

Spec URL: http://paragn.fedorapeople.org/fedora-work/SPECS/glyphtracer.spec
SRPM URL:
http://paragn.fedorapeople.org/fedora-work/SRPMS/glyphtracer-1.0-1.fc13.src.rpm
Comment 5 jpakkane 2010-05-02 09:20:12 EDT
Now it has a setup.py.
Comment 6 Parag Nemade 2010-05-02 23:57:38 EDT
Thanks. I have updated again package.

Spec URL: http://paragn.fedorapeople.org/fedora-work/SPECS/glyphtracer.spec
SRPM URL: http://paragn.fedorapeople.org/fedora-work/SRPMS/glyphtracer-0-0.1.r72.fc13.src.rpm

I have kept wrapper file to be installed in /usr/bin as its easy way to generate ttf file from a given image.
Comment 8 Parag Nemade 2010-05-07 02:55:16 EDT
anyone to review this? I can review your package in exchange of this review.
Comment 9 Toshio Ernie Kuratomi 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
Comment 10 Toshio Ernie Kuratomi 2010-05-07 11:13:57 EDT
Grr typo:
* This is a post-release snapshot so you have the release almost correct but it
  should include the date like this::
    1.$DATEbzr$BZR%{?dist}

  So like this::
    2.20100507bzr73%{?dist}
Comment 11 jpakkane 2010-05-09 10:00:47 EDT
I don't plan on providing any sort of API stability in gtlib or glyphtracer. Thus I would not recommend importing them to anyone else.
Comment 12 Toshio Ernie Kuratomi 2010-05-09 12:37:43 EDT
jpakkane, Do these changes look good?

  bzr branch lp:~toshio/glyphtracer/misc-packaging-changes
Comment 13 jpakkane 2010-05-09 15:45:33 EDT
I have merged your changes to trunk. Thanks.
Comment 14 Toshio Ernie Kuratomi 2010-05-10 00:05:03 EDT
Created attachment 412706 [details]
spec file update taking care of remaining must issues

Here's a spec file patch that works with the current snapshot to resolve all remaining blockers.  The only thing it lacks is the SHOULD to include a man page.  I don't think that glyphtracer has any non-QT commandline options, though, so I certainly won't block on that.
Comment 15 Parag Nemade 2010-05-10 00:20:03 EDT
Thanks Toshio for your detailed review and patch for upstream.

Thanks jpakkane for committing Toshio's patch in upstream.

I have again pulled current bzr code and here is new package

SPEC URL: http://paragn.fedorapeople.org/fedora-work/SPECS/glyphtracer.spec
SRPM URL: http://paragn.fedorapeople.org/fedora-work/SRPMS/glyphtracer-1.1-1.20100510bzr76.fc13.src.rpm
Comment 16 Parag Nemade 2010-05-10 00:21:32 EDT
Looks Mid-air collision. But anyway I still submitted my comment.
Comment 17 Toshio Ernie Kuratomi 2010-05-10 01:04:20 EDT
All issues taken care of.

APPROVED.
Comment 18 Parag Nemade 2010-05-10 01:13:20 EDT
Thanks Toshio!

New Package CVS Request
=======================
Package Name: glyphtracer
Short Description: Program for creating fonts from images
Owners: pnemade
Branches: F-12 F-13 EL-5
InitialCC:
Comment 19 Huzaifa S. Sidhpurwala 2010-05-10 01:17:40 EDT
cvs done
Comment 20 Parag Nemade 2010-05-10 01:40:44 EDT
Thanks Huzaifas!

Built for all Fedora branches.
Comment 21 Fedora Update System 2010-05-10 01:41:41 EDT
glyphtracer-1.1-1.20100510bzr76.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/glyphtracer-1.1-1.20100510bzr76.fc13
Comment 22 Fedora Update System 2010-05-10 01:44:06 EDT
glyphtracer-1.1-1.20100510bzr76.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/glyphtracer-1.1-1.20100510bzr76.fc12
Comment 23 Parag Nemade 2010-05-10 02:05:32 EDT
I am unable to build this for EPEL5. Can anyone help me what changes I need to make this program working for python 2.4.3?

See failed scratch build http://koji.fedoraproject.org/koji/taskinfo?taskID=2176307
Comment 24 Toshio Ernie Kuratomi 2010-05-10 06:05:23 EDT
Does changing class definitions from:
    class Foo():

to:
    class Foo(object):

work?
Comment 25 Parag Nemade 2010-05-10 06:48:24 EDT
Thanks again Toshio!

It worked fine. Also need to add --vendor to desktop-file-install and scratch  build is successful at http://koji.fedoraproject.org/koji/taskinfo?taskID=2176848
Comment 26 jpakkane 2010-05-10 11:40:01 EDT
I have done the same fix in trunk.
Comment 27 Fedora Update System 2010-05-10 12:57:29 EDT
glyphtracer-1.1-1.20100510bzr76.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update glyphtracer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/glyphtracer-1.1-1.20100510bzr76.fc12
Comment 28 Fedora Update System 2010-05-10 17:50:17 EDT
glyphtracer-1.1-1.20100510bzr76.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update glyphtracer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/glyphtracer-1.1-1.20100510bzr76.fc13
Comment 29 Fedora Update System 2010-05-24 15:35:52 EDT
glyphtracer-1.1-1.20100510bzr76.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 30 Fedora Update System 2010-05-24 15:39:01 EDT
glyphtracer-1.1-1.20100510bzr76.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

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