Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 229927 - Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: 229929
TreeView+ depends on / blocked
 
Reported: 2007-02-24 15:30 UTC by Mamoru TASAKA
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-05 01:10:10 UTC
hdegoede: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Mamoru TASAKA 2007-02-24 15:30:23 UTC
Spec URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab.spec
SRPM URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-0.94-0.1.pre2.fc7.src.rpm
Mockbuild log on FC-devel i386: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/LOGS/MOCK-mecab.log
Description: 
MeCab is a open source morphological analyzer which uses 
CRF (Conditional Random Fields) as the estimation of parameters.

rpmlint on binary rpms:
-----------------------------------------------------
1. E: mecab no-binary
2. W: mecab no-documentation
3. W: mecab-base conffile-without-noreplace-flag /etc/mecabrc
4. W: mecab-devel no-dependency-on mecab
5. W: mecab-devel no-documentation
-----------------------------------------------------
1. mecab binary rpm is a dummy package which pulls
   mecab-base and mecab-dic (I will sumbit later) so
   actually mecab rpm is empty.
   * mecab requires mecab-base and mecab-dic
   * mecab-dic requires mecab-base and mecab-devel to rebuild
3. This file is expected to be overwritten by mecab-dic
   package so I will not mark this file as noreplace.

Comment 1 Hans de Goede 2007-02-26 11:59:01 UTC
Some initial Should Fix's:
* Better document / explain why you've got an empty base package and then a 
  -base package. I think I get it, mecab-dict will buildrequire mecab-devel
  (and thus mecab-base) if it would actually require mecab plain, then there
  would be a  problem as mecab plain requires macab-dict, which in turn
  buildrequires mecab  plain, chicken and egg problem. Right?
* mecab, not mecab base will go into comps, and thus the %description
  of mecab is what most end users will see. So the decription of mecab
  should be what you currently have for mecab-base and then the description of
  mecab-base would be something like:
  This package contains the actual mecab-files, the mecab package is just a 
  dummy package things are done this way because ......
* Can't we just avoid all this together by not requiring mecab-dict? I 
  understand that mecab is of no use without dicts, but I think there are
  multiple dictionaries possbile right? Now if a user installs mecab through
  yum, he will get the dictionary with the shortest name (as that is what yum 
  will choose). Wouldn't it be better to thus let he user install the dict 
  himself instead of doing this through deps? I think its safe to assume that
  people who want to use mecab no a bit about it as its very specific, and thus
  know they should install a dict.
* Coding Style, as said above this is all SHOULD not MUST. I notice that you
  use %{__cmd} everywhere instead of just plain "cmd" the Fedora standard sort
  of is to use just cmd, so use rm instead of %{__rm} I also find this much 
  easier to read. Notice that this also is what the spec templates used
  throughout fedora contain. But if you're uses to doing things this way and
  don't want to change it, thats fine too. Once I'm done reviewing this I'll
  probably never look at that .spec file again.
* I also have my questions about the configfile handling. Does mecab-base not 
  work (as a buildrequire) without a config file? Having one file owned and in
  2 different packages is very ugly. Also what will happen if i install multiple
  dicitonaries? Again wouldn't it be better to just let the user add any 
  dictionaries. Or the best would be I think to have a %post in dict packages
  where they add themselves to the config file through sed magic, or something
  like that. This is how its handled for fonts and XFs for example.
 
  Related to this I also think that you're getting the noreplace flag wrong,
  noreplace doesn't meant that it cannot be replaced by another package
  noreplace (AFAIK) means that if the file is modified and you update
  to a newer mecab-base, that the upgrade then won't replace the conffile when
  its modified, IOW AFAIK noreplace is exactly what you want.






  

Comment 2 Hans de Goede 2007-02-26 13:10:59 UTC
I just did a 64 bit build and that throws these additional rpmlint errors:
E: mecab-base binary-or-shlib-defines-rpath /usr/bin/mecab ['/usr/lib64']
E: mecab-base binary-or-shlib-defines-rpath /usr/libexec/mecab/mecab-dict-gen
['/usr/lib64']
E: mecab-base binary-or-shlib-defines-rpath /usr/libexec/mecab/mecab-system-eval
['/usr/lib64']
E: mecab-base binary-or-shlib-defines-rpath /usr/libexec/mecab/mecab-cost-train
['/usr/lib64']
E: mecab-base binary-or-shlib-defines-rpath /usr/libexec/mecab/mecab-test-gen
['/usr/lib64']
E: mecab-base binary-or-shlib-defines-rpath /usr/libexec/mecab/mecab-dict-index
['/usr/lib64']

Which can and must be fixed by adding the following two lines:
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool
after %configure

The lack of rpath however will cause %check to fail, this can be fixed by
adding: "export LD_LIBRARY_PATH=`pwd`/src/.libs" between %check and cd tests


Comment 3 Mamoru TASAKA 2007-02-26 14:24:14 UTC
(In reply to comment #2)
> The lack of rpath however will cause %check to fail, this can be fixed by
> adding: "export LD_LIBRARY_PATH=`pwd`/src/.libs" between %check and cd tests

Actually removing rpath completely simply breaks the test because
the binary (not installed) actually requres rpath, and your fix
(also seemingly recommended on Fedora) seems just a workaround.

Rather, does the following work for you?
-----------------------------------------------------
%configure
%{__sed} -i -e '/dlsearch/s|\(/[a-z/]*\)lib\([a-z/]*\)|\1lib\2 \1lib64\2|g' libtool

%{__make} %{?_smp_mflags}
-----------------------------------------------------


Comment 4 Hans de Goede 2007-02-26 14:39:55 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > The lack of rpath however will cause %check to fail, this can be fixed by
> > adding: "export LD_LIBRARY_PATH=`pwd`/src/.libs" between %check and cd tests
> 
> Actually removing rpath completely simply breaks the test because
> the binary (not installed) actually requres rpath, and your fix
> (also seemingly recommended on Fedora) seems just a workaround.
> 
> Rather, does the following work for you?
> -----------------------------------------------------
> %configure
> %{__sed} -i -e '/dlsearch/s|\(/[a-z/]*\)lib\([a-z/]*\)|\1lib\2 \1lib64\2|g'
libtool
> 
> %{__make} %{?_smp_mflags}
> -----------------------------------------------------
> 

Yes that works too, notice that you should replace the lib64 in there with
%{_lib}, you could also use %ifarch, but that becomes ugly soon as you will need
atleast x86_64 and ppc64 there then and in the future maybe others.


Comment 5 Mamoru TASAKA 2007-02-26 15:06:41 UTC
Well, here what we are required is simply to remove rpath, so
here I include your advice.

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-0.94-0.2.pre2.fc6_LC.src.rpm
(well, for normal rpmbuild, I set dist as "fc6_LC"...)
---------------------------------------------------------
* Mon Feb 26 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 0.94-0.2.pre2
- Remove rpath on 64bits.

Comment 6 Hans de Goede 2007-02-26 15:16:08 UTC
looks fine, but what about all my other remarks?


Comment 7 Mamoru TASAKA 2007-02-26 16:54:54 UTC
(In reply to comment #6)
> looks fine, but what about all my other remarks?

Just I didn't look them...
Well, your suggestion seems preferable. However I am always annoyed
when I review packages
* main package requires some data
* and the data is provided by several package and only one is
  required
and the views are different between the packages...

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-0.94-0.3.pre2.fc6_LC.src.rpm

(for now also write jumandic here)
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab-jumandic.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-jumandic-5.1.20051121-2.fc6_LC.src.rpm

* Tue Feb 27 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 0.94-0.3.pre2
- Package requirement deps reconstruct

* Tue Feb 27 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 5.1.20051121-2
- Package requirement deps reconstruct



Comment 8 Hans de Goede 2007-02-26 18:34:21 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > looks fine, but what about all my other remarks?
> 
> Just I didn't look them...
> Well, your suggestion seems preferable. However I am always annoyed
> when I review packages
> * main package requires some data
> * and the data is provided by several package and only one is
>   required
> and the views are different between the packages...
> 
> 

This new version looks much better and rpmlint likes it too :)

So here's the full review:

MUST:
=====
* rpmlint output is:
W: mecab-devel no-documentation
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* No locales
* Shared libraries, ldconfig run as required
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* -devel package as needed
* no .desktop file required

Approved by Hans de Goede, time to file a CVS branch request.


Comment 9 Mamoru TASAKA 2007-02-26 18:41:44 UTC
Thank you!!

Request to CVS admin about new package:
---------------------------------------------
New Package CVS Request
=======================
Package Name:          mecab
Short Description:     Yet Another Part-of-Speech and Morphological Analyzer
Owners:                mtasaka@ioa.s.u-tokyo.ac.jp
Branches:              FC-devel FC-6 FC-5
InitialCC:             (nobody)
---------------------------------------------

Comment 10 Mamoru TASAKA 2007-03-02 13:18:55 UTC
Hans, will you have a time to review mecab-jumandic again?

Comment 11 Mamoru TASAKA 2007-03-05 01:10:10 UTC
Well, importing done for all branching, closing.

Thank you for reviewing and approving this package.


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