Bug 249006 - Review Request: m17n-contrib - Contributed m17n input maps
Summary: Review Request: m17n-contrib - Contributed m17n input maps
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jens Petersen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-07-20 09:56 UTC by Parag AN(पराग)
Modified: 2007-11-30 22:12 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-25 04:04:41 UTC
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 06:44 UTC, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.1.patch (2.38 KB, patch)
2007-07-23 07:57 UTC, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.2.patch (2.52 KB, patch)
2007-07-24 00:49 UTC, Jens Petersen
no flags Details | Diff
m17n-contrib.spec-1.1.3-script-perm.patch (376 bytes, patch)
2007-07-24 07:29 UTC, Jens Petersen
no flags Details | Diff

Description Parag AN(पराग) 2007-07-20 09:56:53 UTC
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 05:53:08 UTC
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 06:44:45 UTC
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 07:08:09 UTC
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 07:57:55 UTC
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 08:44:41 UTC
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 12:23:23 UTC
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 12:47:34 UTC
(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 13:20:17 UTC
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 23:17:58 UTC
> [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-24 00:27:40 UTC
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-24 00:42:10 UTC
Parag, your version is still without my last patch.

Comment 12 Jens Petersen 2007-07-24 00:49:55 UTC
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-24 01:12:14 UTC
(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-24 01:59:47 UTC
(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-24 02:44:57 UTC
> 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-24 02:52:32 UTC
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 06:35:17 UTC
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 06:36:43 UTC
as per comment 6 , I kept tbl2mim.awk script with 755 permissions

Comment 19 Jens Petersen 2007-07-24 07:29:16 UTC
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 07:58:40 UTC
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 08:37:57 UTC
Thanks for review.

Comment 22 Parag AN(पराग) 2007-07-24 08:42:18 UTC
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 08:48:34 UTC
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 13:28:46 UTC
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 15:13:37 UTC
cvsadmin done

Comment 26 Jens Petersen 2007-07-24 23:23:46 UTC
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 23:27:34 UTC
Please also use lowercase for the name of the esperanto subpackage to be
consistent.

Comment 28 Parag AN(पराग) 2007-07-25 01:46:10 UTC
Built successfully.
Thanks for review Jens.
Do I need to build m17n-db without m17n-contrib now?


Comment 29 Jens Petersen 2007-07-25 02:05:25 UTC
Yes please update m17n-db otherwise there will be conflicts.

Comment 30 Parag AN(पराग) 2007-07-25 04:04:41 UTC
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.