Bug 488010 - Review Request: ibus-table-cangjie - Cang Jie input method for ibus-table.
Summary: Review Request: ibus-table-cangjie - Cang Jie input method for ibus-table.
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Jens Petersen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 484650 488168 488172 488173 488175
TreeView+ depends on / blocked
 
Reported: 2009-03-02 05:56 UTC by Caius Chance
Modified: 2010-01-13 05:02 UTC (History)
7 users (show)

Fixed In Version:
Clone Of: 484650
Environment:
Last Closed: 2009-03-16 02:21:42 UTC
Type: ---
Embargoed:
petersen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
ibus-table-cangjie.spec-1.patch (757 bytes, patch)
2009-03-06 01:18 UTC, Jens Petersen
no flags Details | Diff
ibus-table-cangjie.spec-2.patch (1.82 KB, patch)
2009-03-06 02:43 UTC, Jens Petersen
no flags Details | Diff
ibus-table-cangjie.spec-3.patch (1.83 KB, patch)
2009-03-12 07:23 UTC, Jens Petersen
no flags Details | Diff
ibus-table-cangjie.spec-4.patch (1.08 KB, patch)
2009-03-12 08:26 UTC, Jens Petersen
no flags Details | Diff

Description Caius Chance 2009-03-02 05:56:52 UTC
A split from ibus-table-chinese, which was sub-package of ibus-table.

Uploading srpm and spec.

Comment 2 Caius Chance 2009-03-03 00:08:57 UTC
Changed Requires: from ibus to ibus-table.
Same URL of comment #1.

Comment 3 Jens Petersen 2009-03-03 00:23:49 UTC
First question that comes to mind is why are the db files generated at install
time rather than build-time?

Peng Huang, is that to save payload space?

Comment 4 Peng Huang 2009-03-03 02:45:07 UTC
The db files are generated at buid-time but without index. It is for reducing the size of package files. The commands in %post is for creating the index. I think it is OK.

Comment 5 Caius Chance 2009-03-03 04:47:21 UTC
Spec URL: http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie-1.1.0.20090220-1.fc11.src.rpm
SRPM URL: http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie.spec
Description: A split from ibus-table-chinese, which was sub-package of ibus-table.

Comment 6 Parag AN(पराग) 2009-03-04 09:30:58 UTC
build failed-> http://koji.fedoraproject.org/koji/taskinfo?taskID=1219412
also I see you included COPYING with text of GPLv3 and license tag in spec is GPLv2.

Comment 7 Caius Chance 2009-03-05 23:59:46 UTC
Added "BuildRequires" automake.
Updated license # to GPLv3.
Rebuilt.

Spec URL:
http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie-1.1.0.20090220-2.fc11.src.rpm

SRPM URL: http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie.spec

Comment 8 Caius Chance 2009-03-06 00:11:11 UTC
FYI, coment# 7 is built successfully with mock on 6 MAR 08 rawhide.

Comment 9 Jens Petersen 2009-03-06 01:18:28 UTC
Created attachment 334251 [details]
ibus-table-cangjie.spec-1.patch

- no need to rebootstrap
- silence setup

Still does not build on i386 since ibus-table.noarch contains /usr/lib64/pkgconfig/ibus-table.pc!

Comment 10 Jens Petersen 2009-03-06 02:43:05 UTC
Created attachment 334255 [details]
ibus-table-cangjie.spec-2.patch

This now builds correctly after I fixed pkgconfig in ibus-table to be noarch,
and even installs without errors. ;o)

However after installing and restarting ibus I couldn't find the Cangjie tables in the ibus menu.

Comment 11 Jens Petersen 2009-03-06 02:45:50 UTC
rpmlint says:

ibus-table-cangjie.noarch: E: zero-length /usr/share/doc/ibus-table-cangjie-1.1.0.20090220/AUTHORS
ibus-table-cangjie.noarch: W: summary-ended-with-dot Cang Jie input methods for ibus-table.

Please fix those.

Comment 13 Caius Chance 2009-03-06 04:07:00 UTC
Applied patch in comment #10.
Updated AUTHORS.
Fixed summary-ended-with-dot.


Spec URL:
http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie-1.1.0.20090220-3.fc11.src.rpm

SRPM URL: http://fedorapeople.org/~cchance/packaging/ibus-table-cangjie.spec

Comment 14 Caius Chance 2009-03-06 04:08:47 UTC
Tested on latest rawhide w/ koji ibus-table-1.1.0.20090220-5.fc11.noarch, cangjie 3 and 5 are in ibus-setup to be enabled.

Comment 16 Jens Petersen 2009-03-06 05:26:16 UTC
spec and srpm links are reversed above again

srpm is missing: you still need a patch?

Comment 18 Jens Petersen 2009-03-09 01:55:30 UTC
If the cangjie tables are from the public domain, then the license should reflect that.

I think we need a new upstream package with the correct license.

Comment 20 Jens Petersen 2009-03-09 06:50:12 UTC
It is possible to request a new upstream release or do a snapshot to fix the license issue:
I don't patching the license file is a good idea.

Comment 21 Caius Chance 2009-03-09 07:28:45 UTC
Sure. Will do.

Comment 23 Jens Petersen 2009-03-10 03:15:39 UTC
%post no longer needed?

Is it better to separate Quick and Cangjie or to keep them together?

Comment 24 Caius Chance 2009-03-10 04:08:07 UTC
Files generated in %post (.db files) are actually created during running of autogen.sh.

I have modified the upstream configure.ac to even get quick 3 and 5 generated at once, too.

If you think that is a better solution for separating quick and cangjie into different packages, I would not mind.

However, this involves creation of "quick" tarball on upstream sources system, modification of current cangjie tarball, package creation of "quick" and its review, etc.

Personally I prefer to do it after package review of all other 5 ibus-table-* done.

Comment 25 Jens Petersen 2009-03-10 10:53:45 UTC
(In reply to comment #24)
> Files generated in %post (.db files) are actually created during running of
> autogen.sh.

Ok I will check but what about comment 4: shouldn't the indices be added at installed time?

> Personally I prefer to do it after package review of all other 5 ibus-table-*
> done.  

Yep, adding ibus-table-quick later is fine: well whatever upstream decides - but sounds like it makes sense to separate the quicks into a separate package. :)

Comment 26 Caius Chance 2009-03-11 11:34:38 UTC
(In reply to comment #25)
> Yep, adding ibus-table-quick later is fine: well whatever upstream decides -
> but sounds like it makes sense to separate the quicks into a separate package.
> :)  

Upstream has granted this decision make to me. :)

Comment 28 Jens Petersen 2009-03-12 07:23:00 UTC
Created attachment 334902 [details]
ibus-table-cangjie.spec-3.patch

With this it builds again in mock and some macro use fixes.

(Just change %bcond_without to %bcond_with to turn off bootstrapping.)

Comment 30 Jens Petersen 2009-03-12 08:26:39 UTC
Created attachment 334904 [details]
ibus-table-cangjie.spec-4.patch

This patch should do the --no-create-index properly: with my inspection NO_INDEX is (now?) a no-op.

Better would be to change this upstream to be a configure option (--without-index).

Comment 31 Jens Petersen 2009-03-12 08:28:12 UTC
I guess now %post may change the checksum of the db files at install - so probably they should be labelled such in %files.

Comment 32 Jens Petersen 2009-03-12 08:32:49 UTC
Also if COPYING only applies to icons I think it would be better to move it to the icons directory upstream
otherwise it is a bit misleading or labelled as such at least.

Comment 33 Jens Petersen 2009-03-12 08:35:21 UTC
Here is the review:

 +:ok, =:needs attention, -:needs fixing,  NA: not applicable

MUST Items:
[+] MUST: rpmlint output
[+] MUST: Package Naming Guidelines
[+] MUST: spec file name must match base package %{name}
[+] MUST: Packaging Guidelines.
[+] MUST: Licensing Guidelines
[+] MUST: License field in the package spec file must match actual license.
[+] MUST: include license files in %doc if available in source
[+] MUST: The spec file must be written in American English and be legible.
[+] MUST: source md5sum matches upstream release

21b4c23cd7c700330d7006d8c38f3b2e  ibus-table-cangjie-1.1.0.20090309.tar.gz

[+] MUST: must successfully compile and build into binary rpms on one main arch
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissable content.
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[?] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.

Any Chinese translation available?

[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.

However words/minute feature should be disabled.

[+] SHOULD: If scriptlets are used, those scriptlets must be sane.


Apart from comment 31, I think the package looks ok now.

Comment 34 Mamoru TASAKA 2009-03-12 09:02:07 UTC
I think Requires(post) should be used on this srpm to
ensure the correct rpm order.

Comment 35 Caius Chance 2009-03-13 01:35:46 UTC
http://cchance.fedorapeople.org/packaging/ibus-table-cangjie.spec
http://cchance.fedorapeople.org/packaging/ibus-table-cangjie-1.1.0.20090309-5.fc11.src.rpm 

- check md5 for .db files
- move COPYING to icons/
- requires ibus-table for post-install

Comment 36 Jens Petersen 2009-03-13 02:25:27 UTC
Looks good to me now, thanks

Package is APPROVED.

Comment 37 Caius Chance 2009-03-13 04:12:19 UTC
Package Change Request
=======================
Package Name: ibus-table-cangjie
Short Description: Cangjie and Quick input method for ibus-table.
Owners: cchance
Branches: N/A (devel)
InitialCC:
Cvsextras Commits: yes

Comment 39 Jens Petersen 2009-03-16 00:46:30 UTC
cvs admin done; for reference:

[[
Package Name: ibus-table-cangjie
Short Description: Cangjie input method tables for ibus-table
Owners: cchance
Branches: devel
InitialCC: i18n-team
]]

Comment 40 Caius Chance 2009-03-16 02:21:42 UTC
Built to rawhide:

http://koji.fedoraproject.org/koji/buildinfo?buildID=94216

Comment 41 Caius Chance 2009-05-20 00:43:20 UTC
Package Change Request
======================
Package Name: ibus-table-cangjie
New Branches: F-10
Owners: cchance

Comment 42 Kevin Fenzi 2009-05-20 05:37:08 UTC
cvs done.


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