Bug 434954 - Review Request: WritRecogn - A CJK handwriting recognizer
Review Request: WritRecogn - A CJK handwriting recognizer
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jens Petersen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-26 09:52 EST by Ding-Yi Chen
Modified: 2008-03-02 20:22 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-02 20:22:21 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
petersen: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)
writRecogn.spec-1.patch (2.93 KB, patch)
2008-02-26 22:02 EST, Jens Petersen
no flags Details | Diff
WritRecogn.spec-2.patch (1.90 KB, patch)
2008-02-27 19:00 EST, Jens Petersen
no flags Details | Diff
WritRecogn.spec-3.patch (1.26 KB, patch)
2008-02-28 02:54 EST, Jens Petersen
no flags Details | Diff

  None (edit)
Description Ding-Yi Chen 2008-02-26 09:52:51 EST
Spec URL: http://optusnet.dl.sourceforge.net/sourceforge/writrecogn/writRecogn.spec
SRPM URL: http://optusnet.dl.sourceforge.net/sourceforge/writrecogn/writRecogn-0.1.1-0.fc8.src.rpm
Description: writRecogn recognizes east Asian handwritten characters for Chinese, Japanese, and Korean (CJK) and will interface to input methods such as SCIM. Unlike some implementations which require to build a huge set of character recognition rules, we recognize radicals of Chinese characters, i.e. the word root of the character, then use a character-structure-based input method to search for the word.
Comment 1 Jens Petersen 2008-02-26 21:31:14 EST
rpmlint says:

writRecogn.src: W: summary-ended-with-dot A handwriting recognizer that allows
users to input Chinese characters used in Chinese, Japanese and Korean.
writRecogn.src: E: summary-too-long A handwriting recognizer that allows users
to input Chinese characters used in Chinese, Japanese and Korean.
writRecogn.src: E: description-line-too-long writRecogn recognizes east Asian
handwritten characters for Chinese, Japanese, and Korean (CJK) and will
interface to input methods such as SCIM. Unlike some implementations which
require to build a huge set of character recognition rules, we recognize
radicals of Chinese characters, i.e. the word root of the character, then use a
character-structure-based input method to search for the word. This saves us
from writing recognition rules for tens of thousands of CJK characters. This
should provide better recognition accuracy than current open source handwriting
recognition libraries, like tomoe.
writRecogn.src: E: description-line-too-long The main program which provides the
GUI is writRecogn and there is a commandline character data maintenance program
cdMgr.
Comment 2 Jens Petersen 2008-02-26 21:39:36 EST
Personally I would suggest calling it WritRecogn or writrecogn.
The mixed lower- and uppercase looks a little awkward to me,
for the record anyway.

| BuildRequires:  gtk2-devel >= 2.10 libxml2-devel >= 2.6 sqlite-devel >= 3.0
libtool >= 1.5
| Requires:       gtk2 >= 2.10  libxml2 >= 2.6  sqlite >= 3.0

I think it should sufficient to specific the minimum versions for the 
buildrequires and then the explicit Requires can be droppped.
They are all satisfied by Fedora 7 anyway so not really necessary
for Fedora anyway.
Comment 3 Jens Petersen 2008-02-26 21:51:08 EST
Program seems to run and work ok - I realise it is still alpha at this stage.
I just note a warning at runtime though:

** (writRecogn:9633): WARNING **: Couldn't find pixmap file: lightbulb.png

and also some debug output (but that is ok at this stage).

Also just wondering if it is necessary to ship all the files in
"/usr/share/writRecogn/data/"?
Comment 4 Jens Petersen 2008-02-26 22:01:56 EST
Again about naming I would suggest naming the programs something like
%{_bindir}/WritRecogn and %{_bindir}/WritRecogn-manager.

It would be nice to have a .desktop file for the main program.
Comment 5 Jens Petersen 2008-02-26 22:02:53 EST
Created attachment 296018 [details]
writRecogn.spec-1.patch

minor spec cleanup suggestions
Comment 6 Ding-Yi Chen 2008-02-27 02:40:04 EST
Jens,
Thanks for your comments.

Regarding comment #2:
The building requires are mainly for koji build, otherwise the koji cannot find
essential libraries.

Regarding comment #3:
The lightbulb.png shows a small icon in "Recognize" buttom. This will be fixed
in next release.

Regarding comment #4,#5:
Addressed accordingly.

Regards,
Ding-Yi Chen



Comment 7 Jens Petersen 2008-02-27 03:02:54 EST
Okay thanks.

Please update the URLs for each revision. :)

Spec URL: http://downloads.sourceforge.net/writrecogn/writRecogn.spec
SRPM URL: http://downloads.sourceforge.net/writrecogn/WritRecogn-0.1.1-1.fc8.src.rpm
Comment 8 Jens Petersen 2008-02-27 03:10:49 EST
rpmlint still says:

WritRecogn.src: E: description-line-too-long writRecogn recognizes East Asian
handwritten characters for Chinese, Japanese, and Korean (CJK). Unlike some
implementations which require to build a huge set of character recognition
rules, we recognize radicals of Chinese characters, i.e. the word root of the
character, then use a character-structure-based input method to search for the
word. This saves us from writing recognition rules for tens of thousands of CJK
characters. This should provide better recognition accuracy than current open
source handwriting recognition libraries.
WritRecogn.src: E: description-line-too-long The main program which provides the
GUI is writRecogn and there is a commandline character data maintenance program
cdMgr.
Comment 9 Jens Petersen 2008-02-27 03:40:08 EST
> Regarding comment #3:
> The lightbulb.png shows a small icon in "Recognize" buttom. This will be fixed
> in next release.

In version 0.2?

(In reply to comment #3)
> Also just wondering if it is necessary to ship all the files in
> "/usr/share/writRecogn/data/"?

How about this?

(In reply to comment #4)
> It would be nice to have a .desktop file for the main program.

See
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
for details about .desktop files.
Comment 10 Jens Petersen 2008-02-27 04:13:55 EST
I think you need to give credit for the zh handwriting data from tomoe
(which is under LGPL2+).
Comment 11 Jens Petersen 2008-02-27 19:00:46 EST
Created attachment 296143 [details]
WritRecogn.spec-2.patch

Patch to fix rpmlint warnings.
Comment 12 Ding-Yi Chen 2008-02-27 21:02:28 EST
As for /usr/share/writRecogn/data: Files starting with handwriting are 
required, others are just examples.

Tomoe does not have .desktop, but cellwrite does. :-)
Will add the .desktop on 0.2 for main program. WritRecogn-manager, on the other 
hand, is a command line program, thus the .desktop is not required,.

An acknowledge section is appended to AUTHORS.

New spec: http://dchen.fedorapeople.org/files/rpms/WritRecogn.spec
New srpm: http://dchen.fedorapeople.org/files/rpms/
WritRecogn-0.1.1-2.fc8.src.rpm

Regards,
Comment 13 Jens Petersen 2008-02-27 21:13:44 EST
Thanks for the update.

(In reply to comment #12)
> As for /usr/share/writRecogn/data: Files starting with handwriting are 
> required, others are just examples.

Ok, then let's remove the examples from the binary package?

> Tomoe does not have .desktop, but cellwrite does. :-)

scim-tomoe requires scim and has a menu entry on the SCIM command menu
so it is ok.  Until WritRecogn is integrated with Input Methods it needs
to have a desktop menu.

> An acknowledge section is appended to AUTHORS.

Thanks
Comment 14 Jens Petersen 2008-02-27 21:40:34 EST
Here is the review:

 +: ok,  -: needs attention

MUST Items:
[+] MUST: rpmlint must be run on every package.
[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines.

Better to set BuildRoot to either:

%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
or
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[-] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.

They do not match.  Please update the upstream source file (or better bump the
upstream version).

[+] MUST: The package must successfully compile and build into binary rpms on at
least one supported architecture.
[+] MUST: All build dependencies must be listed in BuildRequires
[-] MUST: A package must own all directories that it creates.

Need to own %{_datadir}/%{name}

[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section
[+] MUST: Each package must consistently use macros
[+] MUST: The package must contain code, or permissable content.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[-] MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section. This is described in detail in the desktop files section of
Packaging Guidelines. If you feel that your packaged GUI application does not
need a .desktop file, you must put a comment in the spec file with your explanation.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT). See Prepping BuildRoot For %install for details.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[+] SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.
Comment 15 Ding-Yi Chen 2008-02-28 00:52:44 EST
Hi,
Comment #14 addressed.
New spec: http://dchen.fedorapeople.org/files/rpms/WritRecogn.spec
New srpm: http://dchen.fedorapeople.org/files/rpms/
WritRecogn-0.1.1-3.fc8.src.rpm
Comment 16 Jens Petersen 2008-02-28 02:52:55 EST
In future please describe the actual changes in the .spec changelog.

I think you need the full path to the icon in the desktop file for it to display.

You need to use a tarball from upstream in your package.

Otherwise it looks ok to me now.
Comment 17 Jens Petersen 2008-02-28 02:54:52 EST
Created attachment 296168 [details]
WritRecogn.spec-3.patch

few more suggestions
Comment 18 Mamoru TASAKA 2008-02-28 03:46:14 EST
Some comments:
- When installing icons under %_datadir/icons/hicolor/, icon cache
  must be updated. Otherwide gnome panel entry won't display the installed
  icon correctly ("GTK+ icon cache" of
  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets )

- build.log shows (for example:
  http://koji.fedoraproject.org/koji/getfile?taskID=474798&name=build.log )
------------------------------------------------------------------------
config.status: creating src/utils/Makefile
config.status: creating data/Makefile
config.status: creating config.h
config.status: executing depfiles commands
+ make -j2
cd . && /bin/sh /builddir/build/BUILD/WritRecogn-0.1.1/missing --run aclocal-1.10 
cd . && /bin/sh /builddir/build/BUILD/WritRecogn-0.1.1/missing --run autoconf
 cd . && /bin/sh /builddir/build/BUILD/WritRecogn-0.1.1/missing --run
automake-1.10 --foreign 
/bin/sh ./config.status --recheck
running CONFIG_SHELL=/bin/sh /bin/sh ./configure 
--build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
--target=x86_64-redhat-linux-gnu --program-prefix= --prefix=/usr
--exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
--datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
--libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/usr/com
--mandir=/usr/share/man --infodir=/usr/share/info --disable-static
build_alias=x86_64-redhat-linux-gnu host_alias=x86_64-redhat-linux-gnu
target_alias=x86_64-redhat-linux-gnu CFLAGS=-O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m64 -mtune=generic CXXFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
FFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic  --no-create --no-recursion
---------------------------------------------------------------------
  Here autotools are called after configure is finished and then
  configure is rechecked.
  This means timestamps of some files must be fixed (i.e. some files
  need "touch"ing).

Comment 20 Jens Petersen 2008-02-28 23:56:05 EST
Thanks, I think the package now satisfies all the Packaging Requirements.
(As Tasaka commented it would be better to fix the build configure issue.)

Package is APPROVED.
Comment 21 Ding-Yi Chen 2008-02-29 00:54:45 EST
New Package CVS Request
=======================
Package Name: WritRecogn
Short Description: A CJK handwriting recognizer
Owners: dchen
Branches: F-7 F-8
InitialCC: dchen
Cvsextras Commits:yes
Comment 22 Jens Petersen 2008-02-29 01:33:40 EST
cvs admin done

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