Bug 681976 - Review Request: openfst - weighted finite-state transducer library
Summary: Review Request: openfst - weighted finite-state transducer library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-03 18:39 UTC by Jerry James
Modified: 2011-05-19 05:04 UTC (History)
3 users (show)

Fixed In Version: openfst-1.2.7-1.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-17 00:59:08 UTC
Type: ---
steve.traylen: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Jerry James 2011-03-03 18:39:04 UTC
Spec URL: http://jjames.fedorapeople.org/openfst/openfst.spec
SRPM URL: http://jjames.fedorapeople.org/openfst/openfst-1.2.7-1.fc14.src.rpm
Description:
OpenFst is a library for constructing, combining, optimizing, and searching
weighted finite-state transducers (FSTs).  Weighted finite-state transducers
are automata where each transition has an input label, an output label, and a
weight.  The more familiar finite-state acceptor is represented as a
transducer with each transition's input and output label equal.  Finite-state
acceptors are used to represent sets of strings (specifically, regular or
rational sets); finite-state transducers are used to represent binary
relations between pairs of strings (specifically, rational transductions).
The weights can be used to represent the cost of taking a particular
transition.

FSTs have key applications in speech recognition and synthesis, machine
translation, optical character recognition, pattern matching, string
processing, machine learning, information extraction and retrieval among
others.  Often a weighted transducer is used to represent a probabilistic
model (e.g., an n-gram model, pronunciation model).  FSTs can be optimized by
determinization and minimization, models can be applied to hypothesis sets
(also represented as automata) or cascaded by finite-state composition, and
the best results can be selected by shortest-path algorithms.

Comment 1 Steve Traylen 2011-04-29 07:50:42 UTC
Hi ,
It looks like the MD5 sums don't match.

http://openfst.cs.nyu.edu/twiki/pub/FST/FstDownload/openfst-1.2.7.tar.gz  has
md5sum 97196a97d2a1ec88d612321e64dac2e4  

where as  openfst-1.2.7.tar.gz from the .src.rpm has 7e7f70a1cf22a107fb445b4de6af8537.

Comment 2 Jerry James 2011-05-03 16:05:53 UTC
It looks like upstream replaced the 1.2.7 release tarball that originally had February 4 timestamps in it with one containing March 11 timestamps, without changing the version number.  The diff between the two shows a number of bug fixes, and an entirely new class.  That was naughty of them.  I've replaced the SRPM with one containing the new tarball, same URL as before.

Comment 3 Steve Traylen 2011-05-03 19:04:40 UTC
Review openfst-1.2.7-1
https://bugzilla.redhat.com/show_bug.cgi?id=681976

Builds okay with Fedora 16 x86_64 mock.

*** Notes worth reading.

- Package meets naming and packaging guidelines
Yes matches tar ball name.
- Spec file matches base package name.
It does.
- Spec has consistant macro usage.
Yes.
- Meets Packaging Guidelines.
*** Add %{?_isa} tags to BuildRequirs and internal Requires.
- License
ASL 2.0
- License field in spec matches
Very clear licensing with a COPYING file and headers on each source file/
- License file included in package
Yes.
- Spec in American English
Yes
- Spec is legible.
Yes.
- Sources match upstream md5sum:
Yes.
$ md5sum openfst-1.2.7.tar.gz ../SOURCES/openfst-1.2.7.tar.gz 
97196a97d2a1ec88d612321e64dac2e4  openfst-1.2.7.tar.gz
97196a97d2a1ec88d612321e64dac2e4  ../SOURCES/openfst-1.2.7.tar.gz
- BuildRequires correct
*** Add %{?_isa}.
- Spec handles locales/find_lang
Not needed
- Package is relocatable and has a reason to be.
Not relocatable.
- Package has %defattr and permissions on files is good.
It does.
- Package has a correct %clean section.
It does.
- Package has correct buildroot
Not needed anymore.
- Package is code or permissible content.
It does.
- Doc subpackage needed/used.
Not needed.

- Headers/static libs in -devel subpackage.
They are,
- Spec has needed ldconfig in post and postun
It does.
- .pc files in -devel subpackage/requires pkgconfig
No .pc files
- .so files in -devel subpackage.
They are.
- -devel package Requires: %{name} = %{version}-%{release}
*** It does but it should be  %{name}%{?_isa} = %{version}-%{release} these days.
- .la files are removed.
None present.
- Package is a GUI app and has a .desktop file
No GUI.
- Package compiles and builds on at least one arch.
Mock.
- Package has no duplicate files in %files.
None
- Package doesn't own any directories other packages own.
It does not.
- Package owns all the directories it creates.
It does.
- No rpmlint output.

This is quite verbose so I'll put in a sepearte comment.


- final provides and requires are sane:
=== openfst-1.2.7-1.fc16.src.rpm provides

=== openfst-1.2.7-1.fc16.src.rpm requires
libicu-devel  
=== openfst-1.2.7-1.fc16.x86_64.rpm provides
arc_lookahead-fst.so.0()(64bit)  
compact16_acceptor-fst.so.0()(64bit)  
compact16_string-fst.so.0()(64bit)  
compact16_unweighted_acceptor-fst.so.0()(64bit)  
compact16_unweighted-fst.so.0()(64bit)  
compact16_weighted_string-fst.so.0()(64bit)  
compact64_acceptor-fst.so.0()(64bit)  
compact64_string-fst.so.0()(64bit)  
compact64_unweighted_acceptor-fst.so.0()(64bit)  
compact64_unweighted-fst.so.0()(64bit)  
compact64_weighted_string-fst.so.0()(64bit)  
compact8_acceptor-fst.so.0()(64bit)  
compact8_string-fst.so.0()(64bit)  
compact8_unweighted_acceptor-fst.so.0()(64bit)  
compact8_unweighted-fst.so.0()(64bit)  
compact8_weighted_string-fst.so.0()(64bit)  
const16-fst.so.0()(64bit)  
const64-fst.so.0()(64bit)  
const8-fst.so.0()(64bit)  
ilabel_lookahead-fst.so.0()(64bit)  
libfstcompact.so.0()(64bit)  
libfstconst.so.0()(64bit)  
libfstfarscript.so.0()(64bit)  
libfstfar.so.0()(64bit)  
libfstlookahead.so.0()(64bit)  
libfstpdtscript.so.0()(64bit)  
libfstscript.so.0()(64bit)  
libfst.so.0()(64bit)  
olabel_lookahead-fst.so.0()(64bit)  
openfst = 1.2.7-1.fc16
openfst(x86-64) = 1.2.7-1.fc16

=== openfst-1.2.7-1.fc16.x86_64.rpm requires
libdl.so.2()(64bit)  
libdl.so.2(GLIBC_2.2.5)(64bit)  
libfstfar.so.0()(64bit)  
libfstscript.so.0()(64bit)  
libfst.so.0()(64bit)  
libicuuc.so.46()(64bit)  
/sbin/ldconfig  
=== openfst-debuginfo-1.2.7-1.fc16.x86_64.rpm provides
openfst-debuginfo = 1.2.7-1.fc16
openfst-debuginfo(x86-64) = 1.2.7-1.fc16

=== openfst-debuginfo-1.2.7-1.fc16.x86_64.rpm requires
=== openfst-devel-1.2.7-1.fc16.x86_64.rpm provides
openfst-devel = 1.2.7-1.fc16
openfst-devel(x86-64) = 1.2.7-1.fc16

=== openfst-devel-1.2.7-1.fc16.x86_64.rpm requires
libfstcompact.so.0()(64bit)  
libfstconst.so.0()(64bit)  
libfstfarscript.so.0()(64bit)  
libfstfar.so.0()(64bit)  
libfstlookahead.so.0()(64bit)  
libfstpdtscript.so.0()(64bit)  
libfstscript.so.0()(64bit)  
libfst.so.0()(64bit)  
openfst = 1.2.7-1.fc16
=== openfst-tools-1.2.7-1.fc16.x86_64.rpm provides
config(openfst-tools) = 1.2.7-1.fc16
openfst-tools = 1.2.7-1.fc16
openfst-tools(x86-64) = 1.2.7-1.fc16

=== openfst-tools-1.2.7-1.fc16.x86_64.rpm requires
config(openfst-tools) = 1.2.7-1.fc16
libdl.so.2()(64bit)  
libfstfarscript.so.0()(64bit)  
libfstfar.so.0()(64bit)  
libfstpdtscript.so.0()(64bit)  
libfstscript.so.0()(64bit)  
libfst.so.0()(64bit)  
libicudata.so.46()(64bit)  
libicui18n.so.46()(64bit)  
libicuuc.so.46()(64bit)  
openfst = 1.2.7-1.fc16


Items to fix/change/comment on.

1) Fix Add %{?_isa} where appropriate.

2) Wait to see next comment about rpmlint output.

Comment 4 Steve Traylen 2011-05-03 19:22:49 UTC
$ rpmlint  ./openfst.spec 
./openfst.spec:68: E: hardcoded-library-path in ../../lib/.libs
./openfst.spec:70: E: hardcoded-library-path in ../../lib/.libs
./openfst.spec:72: E: hardcoded-library-path in ../../lib/.libs
./openfst.spec:74: E: hardcoded-library-path in ../../lib/.libs
./openfst.spec:78: E: hardcoded-library-path in ../../lib/.libs
./openfst.spec: W: invalid-url Source2: openfst-man.tar.xz
0 packages and 1 specfiles checked; 5 errors, 1 warnings.

For the hardcoded-library-path it's not that obvious exactly what is going
on but by x86_64 builds end up in the correct location so probably fine.

$ rpmlint ./openfst-1.2.7-1.fc16.src.rpm 
openfst.src: W: spelling-error %description -l en_US automata -> automats, automat, automate
openfst.src: W: spelling-error %description -l en_US determinization -> determination, deterministically, deterministic


determinization is probably okay in american-english I would have 
thought even it makes me cringe, maybe determining is better?

automa seems to be included twice so is probably intentional.


$ rpmlint  ./openfst-1.2.7-1.fc16.x86_64.rpm 
openfst.x86_64: W: spelling-error %description -l en_US automata -> automats, automat, automate
openfst.x86_64: W: spelling-error %description -l en_US determinization -> determination, deterministically, deterministic
openfst.x86_64: W: shared-lib-calls-exit /usr/lib64/libfstfarscript.so.0.0.0 exit@GLIBC_2.2.5
openfst.x86_64: W: shared-lib-calls-exit /usr/lib64/libfstpdtscript.so.0.0.0 exit@GLIBC_2.2.5
openfst.x86_64: W: shared-lib-calls-exit /usr/lib64/libfstscript.so.0.0.0 exit@GLIBC_2.2.5
openfst.x86_64: W: shared-lib-calls-exit /usr/lib64/libfst.so.0.0.0 exit@GLIBC_2.2.5
1 packages and 0 specfiles checked; 0 errors, 6 warnings.


You may want to at least submit bugs upstream about those exits.

$ rpmlint ./openfst-tools-1.2.7-1.fc16.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint ./openfst-devel-1.2.7-1.fc16.x86_64.rpm 
openfst-devel.x86_64: W: no-documentation
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact16_acceptor-fst.so compact16_acceptor-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact8_unweighted-fst.so compact8_unweighted-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact64_unweighted-fst.so compact64_unweighted-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact64_unweighted_acceptor-fst.so compact64_unweighted_acceptor-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact8_acceptor-fst.so compact8_acceptor-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/arc_lookahead-fst.so arc_lookahead-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/olabel_lookahead-fst.so olabel_lookahead-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact16_unweighted-fst.so compact16_unweighted-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/const8-fst.so const8-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact64_acceptor-fst.so compact64_acceptor-fst.so.0.0.0
openfst-devel.x86_64: W: dangling-relative-symlink /usr/lib64/fst/compact64_string-fst.so compact64_string-fst.so.0.0.0

....


These are links from -devel package to the main package so they are 
all satisfied. You can get rid of the warnings of course by making non relative
with 
pushd

popd.

which would make the rpmlint a lot cleaner.

Comment 5 Jerry James 2011-05-04 04:58:38 UTC
(In reply to comment #4)
> For the hardcoded-library-path it's not that obvious exactly what is going
> on but by x86_64 builds end up in the correct location so probably fine.

Some of the libraries need to be linked against each other.  That is the way I came up with to accomplish the linking.  There may be a prettier way to do it.  In any case, as you say, once the libraries are actually installed, all is well.

> determinization is probably okay in american-english I would have 
> thought even it makes me cringe, maybe determining is better?

Determinization is the process of converting a nondeterministic automaton into a deterministic automaton.  That word is the one chosen by finite automata theorists, so it is correct, although it understandably is not in the standard dictionary.

> automa seems to be included twice so is probably intentional.

You mean "automata", right?  Yes, that is the plural of automaton.

> You may want to at least submit bugs upstream about those exits.

Will do.  I hate it when library authors do that.

> These are links from -devel package to the main package so they are 
> all satisfied. You can get rid of the warnings of course by making non relative
> with 
> pushd
> 
> popd.
> 
> which would make the rpmlint a lot cleaner.

But it wouldn't do any good, since ldconfig would just put them back the way they were.  Those links are correct; rpmlint just can't cope with a .so in a -devel package pointing to a .so.* in another package.

Comment 6 Steve Traylen 2011-05-06 07:01:47 UTC
So other than 

1) Fix Add %{?_isa} where appropriate.

which can be done before import APPROVED.

Steve.

Comment 7 Jerry James 2011-05-06 15:20:07 UTC
New Package SCM Request
=======================
Package Name: openfst
Short Description: Weighted finite-state transducer library
Owners: jjames
Branches: f14 f15
InitialCC:

Comment 8 Jason Tibbitts 2011-05-06 18:10:40 UTC
Git done (by process-git-requests).

Comment 9 Fedora Update System 2011-05-06 20:08:49 UTC
openfst-1.2.7-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/openfst-1.2.7-1.fc15

Comment 10 Fedora Update System 2011-05-06 20:08:57 UTC
openfst-1.2.7-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/openfst-1.2.7-1.fc14

Comment 11 Fedora Update System 2011-05-07 02:49:01 UTC
openfst-1.2.7-1.fc14 has been pushed to the Fedora 14 testing repository.

Comment 12 Fedora Update System 2011-05-17 00:59:02 UTC
openfst-1.2.7-1.fc14 has been pushed to the Fedora 14 stable repository.

Comment 13 Fedora Update System 2011-05-19 05:04:52 UTC
openfst-1.2.7-1.fc15 has been pushed to the Fedora 15 stable repository.


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