Bug 519512 - Review Request: cmusphinx3 - Large vocabulary speech recognition in C
Summary: Review Request: cmusphinx3 - Large vocabulary speech recognition in C
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-08-26 21:26 UTC by Jerry James
Modified: 2009-09-29 14:31 UTC (History)
3 users (show)

Fixed In Version: 0.8-3.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-29 14:31:06 UTC
Type: ---
Embargoed:
martin.gieseking: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Jerry James 2009-08-26 21:26:23 UTC
Spec URL: http://jjames.fedorapeople.org/sphinx3/cmusphinx3.spec
SRPM URL: http://jjames.fedorapeople.org/sphinx3/cmusphinx3-0.8-1.fc11.src.rpm
Description: 
Sphinx-3 is CMU's state-of-the-art large vocabulary speech recognition system.
It uses Hidden Markov Models (HMM) with continuous output probability density
functions (PDF).  It supports several modes of operation.  The more accurate
mode, known as the "flat decoder", is descended from the original Sphinx-3
release.  The faster mode, known as the "tree decoder", was developed
separately.  The two decoders were merged in Sphinx 3.5, though the flat
decoder was not fully functional until Sphinx 3.7.

The previous paragraph is upstream's description.  I have named this packages "cmusphinx3" instead of "sphinx3" because we already have an unrelated "sphinx" package in Fedora, so "sphinx3" looks like a compatibility package.  Matters are made more complex by the fact that upstream has "sphinx3" and "sphinx4" projects (I will be submitting the latter soon) which serve the same purpose and are both under active development, but sphinx3 is written in C and sphinx4 is written in Java.  Combine that with upstream's tendency to refer to sphinx3 version 0.7 as "Sphinx 3.7", as in the paragraph above, and things get a bit confusing.

I have chosen to name the packages "cmusphinx3" and "cmusphinx4" to try to avoid the confusion.

Comment 1 Martin Gieseking 2009-09-15 13:42:14 UTC
The package does not build in mock because of a missing BR:alsa-lib-devel.

According to the following rpmlint error you should also change the permissions  of file _sphinx3.so to 755. The remaining warnings can be ignored.


$ rpmlint /var/lib/mock/fedora-11-i386/result/*.rpm
cmusphinx3-libs.i586: W: shared-lib-calls-exit /usr/lib/libs3decoder.so.0.0.6 exit
cmusphinx3-libs.i586: W: no-documentation
cmusphinx3-python.i586: W: no-documentation
cmusphinx3-python.i586: E: non-standard-executable-perm /usr/lib/python2.6/site-packages/_sphinx3.so 0775
6 packages and 0 specfiles checked; 1 errors, 3 warnings.

Comment 2 Martin Gieseking 2009-09-15 13:50:54 UTC
The documentation is quite extensive so that it's probably a good idea to put it in a -doc subpackage.

Comment 3 Jerry James 2009-09-15 21:57:33 UTC
Thanks, Martin.  I've added the missing BR and split out a -doc subpackage.  I also added one more patch to fix a weak symbol issue.  I don't know what to say about the permissions on _sphinx3.so.  Here's what I know:

1) Rpmlint is complaining that the permissions *are* 0775, not that they should be 0775.
2) The permissions should be 0775.
3) I don't get that warning from rpmlint on my x86_64 machine.

As far as I can tell, there is nothing to fix there and rpmlint shouldn't be complaining for you.  New URLs:

http://jjames.fedorapeople.org/sphinx3/cmusphinx3.spec
http://jjames.fedorapeople.org/sphinx3/cmusphinx3-0.8-2.fc11.src.rpm

Comment 4 Martin Gieseking 2009-09-16 06:46:36 UTC
(In reply to comment #3)
> 1) Rpmlint is complaining that the permissions *are* 0775, not that they should
> be 0775.
> 2) The permissions should be 0775.

Sorry for my unclear comment. I thought it's necessary to change the permissions to 0755 because rpmlint doesn't seem to like 0775 and all .so files in my site-packages folder are also set to 0755. I get this error not just in an i586 environment but also on a x86_64 system running Fedora 11.
However, I'm not a python expert. If you say 0775 is necessary, I can't say anything against that at the moment. :)

Comment 5 Jerry James 2009-09-16 14:49:12 UTC
D'oh!  I saw 0775 and my brain translated it into 0755 before handing it to my logic unit.  It's all my brain's fault.  The logic unit worked fine. :-)

I still don't understand, though, because that file has mode 0755 when I build it, which is why I'm not seeing the rpmlint warning.  Perhaps umask has something to do with it.  In any case, I have added an %attr line to the spec file to make sure the right permissions are set.

I neglected to bump the release number, so I have committed the packaging faux pas of pushing new versions to exactly the same URLs as in comment 3.

Comment 6 Martin Gieseking 2009-09-17 20:57:17 UTC
(In reply to comment #5)
> D'oh!  I saw 0775 and my brain translated it into 0755 before handing it to my
> logic unit.  It's all my brain's fault.  The logic unit worked fine. :-)

Hope you've been able to reset your brain successfully. At least I'm relieved to hear I'm not the only one who got a buggy version. :)

 
Here are a couple of further things I noticed: 

- you can drop BR: perl, tcsh as they are not necessary

- BR: python-setuptools-devel should also be removed but the -python subpackage needs BR: python-devel

- I suggest to preserve the timestamp of python-setuptools-devel by adding touch -r, e.g.:
 
  pushd doc/s3-2_files/
  iconv -f WINDOWS-1252 -t UTF-8 master03_stylesheet.css > x.css
  touch -r master03_stylesheet.css x.css
  mv -f x.css master03_stylesheet.css
  popd

Comment 7 Jerry James 2009-09-18 17:36:41 UTC
I saw the configure script checking for /bin/sh, which is why I added the tcsh BR.  I didn't notice that nothing actually USES /bin/sh....  Good catch.

I have made all of the changes you suggested in comment 6.  New URLs:

http://jjames.fedorapeople.org/sphinx3/cmusphinx3.spec
http://jjames.fedorapeople.org/sphinx3/cmusphinx3-0.8-3.fc11.src.rpm

Comment 8 Martin Gieseking 2009-09-24 12:10:09 UTC
Here's my full review. I couldn't find any further issues that need to be fixed. Just a minor suggestion:

Personally, I prefer adding the BRs to the subpackage they actually belong to because then it's easier to reproduce the dependencies. So I would move
  - BR: doxygen      => -doc subpackage
  - BR: python-devel => -python subpackage
  - BR: pkgconfig    => -devel subpackage


$ rpmlint /var/lib/mock/fedora-11-i386/result/cmusphinx3-*
cmusphinx3-devel.i586: W: no-documentation
cmusphinx3-libs.i586: W: shared-lib-calls-exit /usr/lib/libs3decoder.so.0.0.6 exit
cmusphinx3-libs.i586: W: no-documentation
cmusphinx3-python.i586: W: no-documentation
7 packages and 0 specfiles checked; 0 errors, 4 warnings.

All warnings are expected and can be ignored.



---------------------------------
keys used in following checklist:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
    - The name sphinx is already in use for a full-text search engine
    - there are several release series (Sphinx-2, -3, -4, PocketSphinx)
    - upstream URL is www.cmusphinx.org => cmusphinx3 is a proper name

[+] MUST: The spec file name must match the base package %{name}.

[+] MUST: The package must meet the Packaging Guidelines.

[+] MUST: The package must be licensed with a Fedora approved license.
    - BSD two clause variant (according to COPYING and source headers)

[+] MUST: The License field in the package spec file must match the actual license.

[+] MUST: File(s) containing the text of the license(s) for the package must be included in %doc.
    - COPYING listed 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. 
    $ sha1sum sphinx3-0.8.zip*
    343af9767342129e1d673423e9bf1a523eff2254  sphinx3-0.8.zip
    343af9767342129e1d673423e9bf1a523eff2254  sphinx3-0.8.zip.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
    koji scratch build:
    https://koji.fedoraproject.org/koji/taskinfo?taskID=1703288

[.] MUST: If the package does not successfully compile, ...

[+] MUST: All build dependencies must be listed in BuildRequires.

[.] MUST: The spec file MUST handle locales properly.
    - no locales 

[+] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
    - shared libs are placed in subpackage -libs
    - ldconfig is called in %post and %postun for subpackage -libs

[.] MUST: If the package is designed to be relocatable,...
    - not relocatable

[+] MUST: A package must own all directories that it creates.

[+] MUST: Files must not be listed more than once in %files.

[+] MUST: Permissions on files must be set properly.

[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.

[+] MUST: Each package must consistently use macros.

[+] MUST: The package must contain code, or permissable content.

[+] MUST: Large documentation files must go in a -doc subpackage.

[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.

[+] MUST: Header files must be in a -devel package.

[.] MUST: Static libraries must be in a -static package.
    - building of static libraries disabled

[+] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'

[+] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
    symlink libs3decoder.so put in -devel

[+] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}

[+] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
    - .la files are removed

[.] MUST: Packages containing GUI applications must include a %{name}.desktop file.
    - no GUI

[+] 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).

[+] MUST: All filenames in rpm packages must be valid UTF-8.


[+] SHOULD: The reviewer should test that the package builds in mock.
    - builds in mock

[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
    - build in koji

[+] SHOULD: The reviewer should test that the package functions as described.
    - seems to work as expected
    - I made some short recordings and they were properly anylized

[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
  
[+] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
    - subpackage -libs doesn't require the base package
    - all other subpackages list the base package as a requirement

[+] SHOULD: pkgconfig(.pc) should be placed in a -devel pkg.

Comment 9 Martin Gieseking 2009-09-24 16:34:18 UTC
I just noticed that you can drop %attr(0755,root,root) from
  %attr(0755,root,root) %{python_sitearch}/_sphinx3.so
because it's applied automatically.

Comment 10 Jerry James 2009-09-25 17:03:30 UTC
With respect to comment 8, I prefer to have all the BRs in one place because then I can easily see what I need to build.  Let's chalk that one up to personal taste.  Thank you very much for the thorough review.

With respect to comment 9, now I'm really confused.  I added that because of the problem you noted in comment 1 and comment 4.  I never saw that problem myself.  So are you saying that it disappeared as mysteriously as it appeared?  That makes me nervous....

If the package now meets with your approval, can you set the fedora-review flag to +?  Thanks again.

Comment 11 Martin Gieseking 2009-09-25 20:28:21 UTC
(In reply to comment #10)
> With respect to comment 8, I prefer to have all the BRs in one place because
> then I can easily see what I need to build.  Let's chalk that one up to
> personal taste.  Thank you very much for the thorough review.

That's absolutely OK for me, and you're welcome.


> With respect to comment 9, now I'm really confused.  I added that because of
> the problem you noted in comment 1 and comment 4.  I never saw that problem
> myself.  So are you saying that it disappeared as mysteriously as it appeared? 
> That makes me nervous....

Yes that's strange. When I built the package yesterday, the flags were set properly without an explicit %attr prefix. I can't reproduce the problem at the moment...


> If the package now meets with your approval, can you set the fedora-review flag
> to +? 

Sure. 

------------------------
The package is APPROVED.
------------------------

Comment 12 Jerry James 2009-09-25 20:34:23 UTC
Hmmm.  Okay, I'll take the %attr back out and we'll see if the problem crops up on the build servers.  If not, we can blame Heisenberg.

New Package CVS Request
=======================
Package Name: cmusphinx3
Short Description: Large vocabulary speech recognition in C
Owners: jjames
Branches: F-11
InitialCC:

Comment 13 Kevin Fenzi 2009-09-26 02:45:55 UTC
cvs done.

Comment 14 Fedora Update System 2009-09-26 16:59:25 UTC
cmusphinx3-0.8-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/cmusphinx3-0.8-3.fc11

Comment 15 Fedora Update System 2009-09-29 14:30:59 UTC
cmusphinx3-0.8-3.fc11 has been pushed to the Fedora 11 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.