Bug 233424 - Review Request: perl-mecab - Perl binding for MeCab
Review Request: perl-mecab - Perl binding for MeCab
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-22 09:04 EDT by Mamoru TASAKA
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-11 00:37:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pertusus: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
0.95-3 spec file (1.99 KB, text/plain)
2007-04-01 02:56 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Comment 1 Mamoru TASAKA 2007-03-31 13:26:50 EDT
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/perl-mecab.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/perl-mecab-0.95-2.fc7.src.rpm

* Sat Mar 31 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 0.95-2
- rename the package (see the discussion on #223423)
- Change test file encoding
Comment 2 Ralf Corsepius 2007-04-01 01:26:34 EDT
Needswork:

- MUSTFIX: No License, no copyright inside of the source tarball.
Spec file claims to be BSD/LGPL/GPL, but their no indication inside of the
sources for this claim, and their website is unreadable to me (seemingly written
in Japanese).

- MUSTFIX: Specfile contains: %{!?python_sitearch: ...}
I fail to understand why a perl module's spec would want python_sitearch

- MUSTFIX: Package doesn't acknowledge RPM_OPT_FLAGS
ExtUtils::MakeMaker Makefiles don't use CXXFLAGS, they use OPTIMIZE
=> %{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="${RPM_OPT_FLAGS}"

- MUSTFIX: Package explictly links against -lstdc++
This is a bug in mecap-config

libstdc++ is an internal implementation detail of g++ which must not be
explictily used.

- SHOULDFIX: %check ||:
The ||: in check is an anachronism to cater ancient versions of rpm and should
not be used.

 
Comment 3 Mamoru TASAKA 2007-04-01 01:52:25 EDT
For license, see 233423
Comment 4 Mamoru TASAKA 2007-04-01 02:56:38 EDT
Created attachment 151372 [details]
0.95-3 spec file

0.95-3:

Well, I cannot upload the whole srpm until I have my account
on my university fixed...

> Needswork:
> 
> - MUSTFIX: No License, no copyright inside of the source tarball.
See bug 233423

> - MUSTFIX: Specfile contains: %{!?python_sitearch: ...}
Just because I forgot to remove this...

> - MUSTFIX: Package doesn't acknowledge RPM_OPT_FLAGS
Fixed
 
> - MUSTFIX: Package explictly links against -lstdc++
> This is a bug in mecab-config
Fixed in mecab-0.95-2

> - SHOULDFIX: %check ||:
> The ||: in check is an anachronism to cater ancient versions of 
> rpm and should not be used.

Well, I usually do this (because one of my computer still
uses rpm-4.0.4), however I removed this.
Comment 5 Patrice Dumas 2007-05-08 12:56:28 EDT
Issues:

The following is not useful:
export CXXFLAGS=$RPM_OPT_FLAGS 

If you use (like in the template) pure_install instead of install
you won't need to remove $RPM_BUILD_ROOT%{perl_archlib}/perllocal.pod

I think that the 
Requires:       mecab = %{version}
is not usefull, the soname dependency is enough. Otherwise the
devel BuildRequires should also be versionned.

ls -lR /usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/MeCab.pm
-r--r--r--
it is wrong, you can use what is in the template 
chmod -R u+w $RPM_BUILD_ROOT/*

Remarks:

I would personally not use -f for rm and mv to have build breaking
when things change.

I also don't like the use of %relnumber and %mainver in my
opinion the clutter the spec unnecessarily.

Regarding the license it is now included in the tarball, so it
is fine with me, even though it certainly lacks something in the 
README.
Comment 6 Mamoru TASAKA 2007-05-08 14:11:24 EDT
Thank you for initial comment!

Updated:
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/perl-mecab.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/perl-mecab-0.95-4.fc7.src.rpm
----------------------------------------------
* Wed May  9 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 0.95-4
- Correctly require version specified BuildRequires
- Rewrite accroding to perl template
----------------------------------------------
I named this package as "mecab-perl" initially, so at first
I didn't use perl template...

(In reply to comment #5)
> The following is not useful:
> export CXXFLAGS=$RPM_OPT_FLAGS
Absolutely......

> If you use (like in the template) pure_install instead of install
> you won't need to remove $RPM_BUILD_ROOT%{perl_archlib}/perllocal.pod
This time I borrowed many ideas from perl template.

> I think that the 
> Requires:       mecab = %{version}
> is not usefull, the soname dependency is enough. Otherwise the
> devel BuildRequires should also be versionned.
Well, I thought that I required version specific BuildRequires
(like python-mecab, etc..). Now requiring version

> chmod -R u+w $RPM_BUILD_ROOT/*
Thanks.

> I would personally not use -f for rm and mv to have build breaking
> when things change.
Well, this is a sort of my habit to use "-f" because I saw
on some cases that "-f" is really required (one example is
bug 225791). 
And.. while I don't know the reason, for directories
recursive delete by "rm -r" occasionally hangs up when not
using "-f" (would you know why?)

> I also don't like the use of %relnumber and %mainver in my
> opinion the clutter the spec unnecessarily.
This is also my habit.. 

> Regarding the license 
This was discussed on bug 233423
Comment 7 Patrice Dumas 2007-05-09 16:41:46 EDT
* rpmlint is silent
* name is right
* follows guidelines
* free software licences, included. Files are generated using SWIG, so
  there may be an issue with GPL/LGPL since the swig template is
  missing and proper license headers are missing, and no word about license
  in the README. However mecab is also packaged and people can go to
  the website so it is likely to be right as said in other reviews.
* match upstream
b924751bf58d7d6c4f3b6b5bbe8e1640  mecab-perl-0.95.tar.gz
* sane provides
Provides: MeCab.so perl(MeCab) perl(MeCab::DictionaryInfo) perl(MeCab::Node)
perl(MeCab::Path) perl(MeCab::Tagger) perl(MeCabc)
* %files section right

APPROVED

About -f, for directories, indeed you need the -f, but my 
comments were for
	%{__mv} -f test.pl.utf8 test.pl || \
	%{__rm} -f test.pl.utf8
Comment 8 Mamoru TASAKA 2007-05-09 18:18:18 EDT
Thank you.

Request for CVS admin:

New Package CVS Request
=======================
Package Name:           perl-mecab
Short Description:      Perl binding for MeCab
Owners:                 mtasaka@ioa.s.u-tokyo.ac.jp
Branches:               devel FC-6 FC-5
InitialCC:              (nobody)
Comment 9 Mamoru TASAKA 2007-05-11 00:37:39 EDT
Rebuilding done.

Thank you for reviewing this package. Closing.

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