[Bug 229927] New: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: mtasaka@ioa.s.u-tokyo.ac.jp QAContact: fedora-package-review@redhat.com
Spec URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab.... SRPM URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-... Mockbuild log on FC-devel i386: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/LOGS/MOCK-me... 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |229929 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |j.w.r.degoede@hhs.nl Flag| |fedora-review?
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 06:59 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 08:10 EST ------- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-02-26 09:24 EST ------- (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} -----------------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 09:39 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-02-26 10:06 EST ------- 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.... http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-... (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 10:16 EST ------- looks fine, but what about all my other remarks?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-02-26 11:54 EST ------- (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.... http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-...
(for now also write jumandic here) http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/mecab-... http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/mecab-...
* 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 13:34 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-02-26 13:41 EST ------- 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) ---------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
wtogami@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-03-02 08:18 EST ------- Hans, will you have a time to review mecab-jumandic again?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: mecab - Yet Another Part-of-Speech and Morphological Analyzer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229927
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-03-04 20:10 EST ------- Well, importing done for all branching, closing.
Thank you for reviewing and approving this package.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla