Bug 249006 - Review Request: m17n-contrib - Contributed m17n input maps
Review Request: m17n-contrib - Contributed m17n input maps
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jens Petersen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-20 05:56 EDT by Parag AN(पराग)
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-25 00:04:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
petersen: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)
m17n-contrib.spec-macro.patch (7.40 KB, patch)
2007-07-23 02:44 EDT, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.1.patch (2.38 KB, patch)
2007-07-23 03:57 EDT, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.2.patch (2.52 KB, patch)
2007-07-23 20:49 EDT, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.1.3-script-perm.patch (376 bytes, patch)
2007-07-24 03:29 EDT, Jens Petersen
no flags Details | Diff

  None (edit)
Description Parag AN(पराग) 2007-07-20 05:56:53 EDT
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-1.fc7.src.rpm
Description: This package contains extra multilingualization (m17n) datafiles for m17n-lib which describe input maps, encoding maps, and OpenType font data for many languages which are not present in m17n-db package.
   With introduction of this package in Fedora, I am trying to split current m17n-db package into m17n-db and m17n-contrib packages.
Comment 1 Jens Petersen 2007-07-23 01:53:08 EDT
I think hi-typewriter.mim should be in m17n-db since it is from:
http://cvs.m17n.org/viewcvs/m17n/m17n-db/MIM/.

You need to use make in %build and "make install" in %install.
Comment 2 Jens Petersen 2007-07-23 02:44:45 EDT
Created attachment 159763 [details]
m17n-contrib.spec-macro.patch

Ok here is a patch with a macro to define the subpackages
and other improvements.  You need to download the missing script
from cvs.
Comment 3 Parag AN(पराग) 2007-07-23 03:08:09 EDT
Thanks for your inputs.
here is updated package links
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-1.1.fc8.src.rpm

Also, updated m17n-db package is available at http://people.redhat.com/pnemade/m17n
Comment 4 Jens Petersen 2007-07-23 03:57:55 EDT
Created attachment 159767 [details]
m17n-contrib.spec-1.1.patch

missing script should be installed 755 - my bad.

Few other bits of cleanup and description improvements.
Comment 5 Parag AN(पराग) 2007-07-23 04:44:41 EDT
Ok done with changes, but I kept same release number. So new links are
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-1.1.fc8.src.rpm
Comment 6 Jens Petersen 2007-07-23 08:23:23 EDT
In the future please always bump the release that makes the reviewers job easier.

I suggest making the tbl2mim.swk script executable in the srpm.

For me the above links are still pointing to the old version of the package
(ie same as comment 3).
Comment 7 Parag AN(पराग) 2007-07-23 08:47:34 EDT
(In reply to comment #6)
> In the future please always bump the release that makes the reviewers job easier.
> 
> I suggest making the tbl2mim.swk script executable in the srpm.
> 
> For me the above links are still pointing to the old version of the package
> (ie same as comment 3).

I rechecked again and new links already have updated SPEC.
Ok now I bump release. So new links are
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-1.2.fc8.src.rpm

Comment 8 Mamoru TASAKA 2007-07-23 09:20:17 EDT
Some notes:

* Please try to keep timestamps as much as possible.
  For recent Makefile, the following method usually works.
-------------------------------------------------------
make install DESTDIR=<foo> INSTALL="%{__install} -c -p"
-------------------------------------------------------

* Check the directory ownership
-------------------------------------------------------
[root@localhost mock]# LANG=C rpm -ivh m17n-contrib-1.1.2-1.2.fc8.noarch.rpm
m17n-contrib-marathi-1.1.2-1.2.fc8.noarch.rpm 
Preparing...                ########################################### [100%]
   1:m17n-contrib           ########################################### [ 50%]
   2:m17n-contrib-marathi   ########################################### [100%]
[root@localhost mock]# LANG=C rpm -qf /usr/share/m17n/icons/mr-inscript.png 
m17n-contrib-marathi-1.1.2-1.2.fc8
[root@localhost mock]# LANG=C rpm -qf /usr/share/m17n/icons/
file /usr/share/m17n/icons is not owned by any package
-------------------------------------------------------
Comment 9 Jens Petersen 2007-07-23 19:17:58 EDT
> [root@localhost mock]# LANG=C rpm -qf /usr/share/m17n/icons/
> file /usr/share/m17n/icons is not owned by any package

Yep I spotted that too: it is actually an old m17n-db bug.
We should have it owned by m17n-db, which will be required by m17n-contrib,
which m17n-contrib-* will require.
Comment 10 Jens Petersen 2007-07-23 20:27:40 EDT
Tasaka-san,

> make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -c -p"

BTW I was wondering if it wouldn't make sense to redefine %makeinstall
to be the above, in say redhat-rpm-config.
Comment 11 Jens Petersen 2007-07-23 20:42:10 EDT
Parag, your version is still without my last patch.
Comment 12 Jens Petersen 2007-07-23 20:49:55 EDT
Created attachment 159825 [details]
m17n-contrib.spec-1.2.patch

Here is the patch again - I bumped the release for you.

Please apply or merge.
Comment 13 Parag AN(पराग) 2007-07-23 21:12:14 EDT
(In reply to comment #12)
> Created an attachment (id=159825) [edit]
> m17n-contrib.spec-1.2.patch
> 
> Here is the patch again - I bumped the release for you.
> 
> Please apply or merge.

Don't know what happened but I was applied patch successfully I guess.
Anyway, reapplying patch. here are new links

Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-1.2.fc8.src.rpm

Comment 14 Mamoru TASAKA 2007-07-23 21:59:47 EDT
(In reply to comment #10)
> Tasaka-san,
> 
> > make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -c -p"
> 
> BTW I was wondering if it wouldn't make sense to redefine %makeinstall
> to be the above, in say redhat-rpm-config.
I think it would make sense. Most Makefile (created by recent
autotool, for example) will accept this method.
Comment 15 Jens Petersen 2007-07-23 22:44:57 EDT
> I think it would make sense. Most Makefile (created by recent
> autotool, for example) will accept this method.

Ok - perhaps you can submit a patch for that.
Might be best to grep all the current spec files to check how many use
%makeinstall currently though - some of them might break...
Comment 16 Parag AN(पराग) 2007-07-23 22:52:32 EDT
oops I thought after applying patch I do not need to bump release.
ok here are updated links
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.2-3.fc8.src.rpm
Comment 17 Parag AN(पराग) 2007-07-24 02:35:17 EDT
Upstream released new version 1.1.3
Here are updated links
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.3-1.fc8.src.rpm

But I got following rpmlint on binary m17n-contrib rpm
E: m17n-contrib non-executable-script /usr/share/m17n/scripts/tbl2mim.awk 0644
This text file contains a shebang or is located in a path dedicated for
executables, but lacks the executable bits and cannot thus be executed.  If
the file is meant to be an executable script, add the executable bits,
otherwise remove the shebang or move the file elsewhere.


Comment 18 Parag AN(पराग) 2007-07-24 02:36:43 EDT
as per comment 6 , I kept tbl2mim.awk script with 755 permissions
Comment 19 Jens Petersen 2007-07-24 03:29:16 EDT
Created attachment 159837 [details]
m17n-contrib.spec-1.1.3-script-perm.patch

This worksaround the script being installed 644.
Could you report it upstream please when you have time.
Comment 20 Jens Petersen 2007-07-24 03:58:40 EDT
Above patch fixes the above rpmlint warning.
The rpmlint warnings like "W: m17n-contrib-assamese no-documentation"
can be waived since the main m17n-contrib package carries the %doc files
and is required by m17n-contrib-*.

Formal review follows:
+ package follow naming guidelines
+ meets packaging guidelines (formerly part of m17n-db)
+ license is LGPL, included
+ spec file is clean
+ verified source tarball is pristine:
f9a6c6d19cb4f27be15db3ec11f95247  m17n-contrib-1.1.3.tar.gz
+ noarch build fine
+ buildrequires m17n-db
+ uses find_lang
+ requires m17n-db for directory ownership
(please add icons dir ownership there)
+ filelists and perms ok with above patch
+ macros consistently used

Package is APPROVED.

Please fix the script permission before importing to cvs.
Comment 21 Parag AN(पराग) 2007-07-24 04:37:57 EDT
Thanks for review.
Comment 22 Parag AN(पराग) 2007-07-24 04:42:18 EDT
here are updated links
Spec URL: http://people.redhat.com/pnemade/m17n/m17n-contrib.spec
SRPM URL: http://people.redhat.com/pnemade/m17n/m17n-contrib-1.1.3-1.1.fc8.src.rpm
Comment 23 Parag AN(पराग) 2007-07-24 04:48:34 EDT
New Package CVS Request
=======================
Package Name: m17n-contrib
Short Description: Contributed m17n input maps
Owners: pnemade@redhat.com
Branches: FC-6 F-7 EL-5
InitialCC: extras-qa@fedoraproject.org

Comment 24 Jens Petersen 2007-07-24 09:28:46 EDT
No EL-5 branch needed since it would conflict with m17n-db in RHEL5.

Please bump release number to 2 when importing.
Comment 25 Jens Petersen 2007-07-24 11:13:37 EDT
cvsadmin done
Comment 26 Jens Petersen 2007-07-24 19:23:46 EDT
I am not really sure about pushing this to FC6 and F7 (at least not yet)
since it will break Indic users updating from current m17n-db.
Comment 27 Jens Petersen 2007-07-24 19:27:34 EDT
Please also use lowercase for the name of the esperanto subpackage to be
consistent.
Comment 28 Parag AN(पराग) 2007-07-24 21:46:10 EDT
Built successfully.
Thanks for review Jens.
Do I need to build m17n-db without m17n-contrib now?
Comment 29 Jens Petersen 2007-07-24 22:05:25 EDT
Yes please update m17n-db otherwise there will be conflicts.
Comment 30 Parag AN(पराग) 2007-07-25 00:04:41 EDT
m17n-db-1.4.0-2.1 is built now.

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