Bug 247227 - (nabi) Review Request: nabi - hangul and hanja X input method
Review Request: nabi - hangul and hanja X input method
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-06 01:27 EDT by subhransu behera
Modified: 2007-11-30 17:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-20 02:06:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
petersen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
nabi.spec-4.patch (2.47 KB, patch)
2007-07-30 03:18 EDT, Jens Petersen
no flags Details | Diff
nabi.desktop (165 bytes, text/plain)
2007-07-30 03:19 EDT, Jens Petersen
no flags Details
nabi.spec-5.patch (1.11 KB, patch)
2007-08-06 22:52 EDT, Jens Petersen
no flags Details | Diff
nabi.spec-6.patch (923 bytes, patch)
2007-08-09 00:03 EDT, Jens Petersen
no flags Details | Diff
nabi-remove-bad-themes (189 bytes, text/plain)
2007-08-16 21:03 EDT, Jens Petersen
no flags Details

  None (edit)
Description subhransu behera 2007-07-06 01:27:54 EDT
Spec URL: 

https://fedoraproject.org/wiki/nabi?action=AttachFile&do=get&target=nabi.spec

SRPM URL: 

https://fedoraproject.org/wiki/nabi?action=AttachFile&do=get&target=nabi-0.17-2.src.rpm

Description: 

Nabi is a Korean word that means butterfly. Nabi (the program) is an easy to use Korean X input method. It allows you to enter phonetic Korean characters (hangul) and pictographic Korean characters (hanja.)

Additional Information: 

This package was there in FC-3.
http://download.fedora.redhat.com/pub/fedora/linux/core/3/SRPMS/nabi-0.14-3.src.rpm

Reference: http://nabi.kldp.net/document_en.html
Comment 1 Mamoru TASAKA 2007-07-06 07:21:26 EDT
Well, you may want to check the spec files of other packages.
For example, there is scim spec:
http://cvs.fedoraproject.org/viewcvs/*checkout*/rpms/scim/devel/scim.spec

Please check the packaging guidelines:
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

* SourceURL must be given with full URL.
* Don't rebuild in mock. At least, libhangul-devel is missing for
  BuildRequires
* Fedora specific compilation flags are not honored. Perhaps
  this can be resolved by using %configure macro.
* Please support parallel make when possible.
* "install-strip" is forbidden. This disables to create debuginfo
  rpm.
* Please keep timestamps. When using "install" command, use it
  with '-p' option.
* Use macros (e.g. /usr/share -> %{_datadir})
* Categories 'Application' 'X-Red-Hat-Base' are deprecated and
  should be removed
* desktop file must be installed by using desktop-file-install
  (BR: desktop-file-utils is needed)
* The binaries which are not under normal users' paths must be
  specified with full path.
* gettext .mo files must be registered in %files entry by using
  %find_lang macro.
* files under %_sysconfdir should be marked as %config(noreplace)
* We recommend %defattr(-,root,root,-)
* Please remove unused macros.
Comment 2 Mamoru TASAKA 2007-07-06 07:34:55 EDT
And:
* %{_sbindir}/alternatives must be added to Requires(....)
Comment 3 Mamoru TASAKA 2007-07-12 01:27:16 EDT
ping?
Comment 4 subhransu behera 2007-07-12 05:38:58 EDT
Hi Mamoru,

Thanks for pointing out the probable mistakes. As nabi-0.18 is released by
upstream I am planning to build the srpm for the same.

 
Comment 5 Jens Petersen 2007-07-20 04:09:15 EDT
Ok, please update with the new .spec file and srpm url when it is ready.
Comment 6 Mamoru TASAKA 2007-07-23 08:52:05 EDT
By the way are there any information when upstream will
release new version?
Comment 7 Jens Petersen 2007-07-23 19:32:58 EDT
It was already released on the 8th, see:

http://kldp.net/frs/?group_id=275
Comment 8 Jens Petersen 2007-07-23 19:59:39 EDT
Subhransu, if you can update your submission fixing the issues raised so far
I am happy to review this.
Comment 9 subhransu behera 2007-07-24 03:15:15 EDT
I am waiting for newer version of libhangul (0.18) to be built.
Comment 10 Jens Petersen 2007-07-24 03:17:18 EDT
Right probably nabi-0.18 requires libhangul-0.0.6 and the current version
in fedora in 0.0.4.  I asked the owner to update it.
Comment 11 subhransu behera 2007-07-26 03:26:26 EDT
Upgraded to 0.18 release.

Spec URL: http://oriya.sourceforge.net/nabi.spec

SRPM URL: http://oriya.sourceforge.net/nabi-0.18-1.fc7.src.rpm
Comment 12 Jens Petersen 2007-07-30 03:18:17 EDT
Created attachment 160216 [details]
nabi.spec-4.patch

some more fixes - please check I didn't break anything. :)
Comment 13 Jens Petersen 2007-07-30 03:19:07 EDT
Created attachment 160217 [details]
nabi.desktop
Comment 14 Mamoru TASAKA 2007-07-30 10:06:42 EDT
(In reply to comment #11)
> Upgraded to 0.18 release.
> Spec URL: http://oriya.sourceforge.net/nabi.spec

(In reply to comment #12)
> Created an attachment (id=160216) [edit]
> nabi.spec-4.patch
> 
> some more fixes - please check I didn't break anything. :)

(I only glanced at both spec files)
More cleanup:
* Please use full path for the commands which are not under
  normal users' paths.

* The following is smarter
-----------------------------------------------------------
desktop-file-install --dir=${RPM_BUILD_ROOT}%{_datadir}/applications/ \
      %SOURCE1
-----------------------------------------------------------
! Note: desktop-file-install automatically fix the following,
        however Categories must be seperated by semicolon: i.e.
-----------------------------------------------------------
Categories=System;
-----------------------------------------------------------

* What is the use of the line:
-----------------------------------------------------------
%attr(755,root,root)
-----------------------------------------------------------

? Question
  Which package should own %_sysconfdir/X11/xinit/xinput.d ?
Comment 15 Jens Petersen 2007-08-06 01:58:01 EDT
(In reply to comment #14)
>   Which package should own %_sysconfdir/X11/xinit/xinput.d ?

I think it should be owned by im-chooser now.
Comment 16 Jens Petersen 2007-08-06 02:13:39 EDT
So actually it should be the nabi config file listed there not the directory.
Comment 17 subhransu behera 2007-08-06 03:57:23 EDT
Upgraded with required fixes.

Spec URL: http://oriya.sourceforge.net/0.18-3/nabi.spec

SRPM URL: http://oriya.sourceforge.net/0.18-3/nabi-0.18-3.fc7.src.rpm
Comment 18 Jens Petersen 2007-08-06 20:24:13 EDT
(In reply to comment #15)
> >   Which package should own %_sysconfdir/X11/xinit/xinput.d ?
> I think it should be owned by im-chooser now.

BTW Tagoh already kindly fixed this in bug 250960. :)
Comment 19 Jens Petersen 2007-08-06 22:52:34 EDT
Created attachment 160792 [details]
nabi.spec-5.patch

This should hopefully address the rest of the points raised by Tasaka-san.
Please check them and update package.
Comment 20 subhransu behera 2007-08-07 03:17:58 EDT
Package Updated.

Spec URL: http://oriya.sourceforge.net/0.18-4/nabi.spec

SRPM URL: http://oriya.sourceforge.net/0.18-4/nabi-0.18-4.fc7.src.rpm
Comment 21 Mamoru TASAKA 2007-08-07 07:52:20 EDT
* rpmlint
  - rpmlint complains about:
-----------------------------------------------
W: nabi invalid-license GPL
W: nabi rpm-buildroot-usage %prep rm -rf $RPM_BUILD_ROOT
-----------------------------------------------
    - License tag policy is updated so please change license
      tag accordingly.
      http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
      http://fedoraproject.org/wiki/Licensing
      As far as I checked the source code, this is GPLv2+.
    - "rm -rf $RPM_BUILD_ROOT" is not needed for %prep
Comment 22 Mamoru TASAKA 2007-08-07 07:52:50 EDT
Other things seems okay for me.
Comment 23 Jens Petersen 2007-08-07 22:04:28 EDT
Thanks, Tasaka-san, for your careful checking: seems the new version of
rpmlint is more strict.

Ok, Subhransu, if you can do one more update I'd like to move into the final
formal next.
Comment 24 subhransu behera 2007-08-08 02:41:38 EDT
Package Updated.

Spec URL: http://oriya.sourceforge.net/0.18-5/nabi.spec

SRPM URL: http://oriya.sourceforge.net/0.18-5/nabi-0.18-5.fc7.src.rpm
Comment 25 Mamoru TASAKA 2007-08-08 12:19:34 EDT
Seems okay to me.
Comment 26 Jens Petersen 2007-08-08 23:59:59 EDT
Here is my review:

Good:
+ rpmlint clean
+ package follows naming and packaging guidelines
+ specifies GPL version and includes COPYING
+ spec file is legible
+ source is pristine
0d0fba8851a1ac367d7b52840e6bef3e  nabi-0.18.tar.gz
+ builds in mock and runs correctly
+ buildreqs listed
+ uses %find_lang
+ file and dir ownership looks correct
+ consistent macro usage
+ does not contain devel files
+ scriplets reasonable

Need attention:
- actually thinking more, I realised that the desktop is not needed
  since we don't normally run input methods from the desktop menu -
  they should be configured with im-chooser instead.
- minor, but I think ChangeLog can be dropped from %doc

Apart from that all MUST items from ReviewGuidelines are satisfied. :)
Comment 27 Jens Petersen 2007-08-09 00:03:05 EDT
Created attachment 160954 [details]
nabi.spec-6.patch

this patch fixes the above:

- remove unnecessary desktop file
- tidy doc filelist
Comment 28 Jens Petersen 2007-08-09 19:47:34 EDT
I contacted upstream about the questionable themes which we are excluding from
the package.  I hope they can be removed in a future release.
Comment 29 subhransu behera 2007-08-10 02:24:18 EDT
Package Updated.

Spec URL: http://oriya.sourceforge.net/0.18-6/nabi.spec

SRPM URL: http://oriya.sourceforge.net/0.18-6/nabi-0.18-6.fc8.src.rpm
Comment 30 Jens Petersen 2007-08-16 20:42:33 EDT
I suggest preparing a new tarball with the unappropriate images removed.
Upstream has agreed to remove them in the next release.

The new tarball should have a distinct name from the upstream one:
say something like nabi-0.18_fedora.tar.gz.

Probably need to patch some of the makefiles too for the removed themes.
Comment 31 Jens Petersen 2007-08-16 21:03:46 EDT
Created attachment 161708 [details]
nabi-remove-bad-themes

A small shellscript does the theme removal.
Comment 32 Mamoru TASAKA 2007-08-24 05:51:00 EDT
Well, what is the status of this bug?
Comment 33 subhransu behera 2007-08-24 06:04:15 EDT
Hi Mamoru San,

We're waiting for upstream to remove the unappropriate images till the 
end of the this month.

Or else, we can go ahead as per as the suggestion of Jens.
Comment 34 Mamoru TASAKA 2007-08-24 06:41:42 EDT
Okay, thank you for imformation.
Comment 35 Jens Petersen 2007-09-03 20:21:56 EDT
Since it is taking a bit longer for the upstream release than expected let's
go ahead with just removing the questionable themes from the tarball as per
<http://fedoraproject.org/wiki/Packaging/SourceURL#head-b46af5567f2338faafa13aff9855caa3c91f2c8c>.
Comment 36 subhransu behera 2007-09-04 02:39:12 EDT
Package updated with version: 0.18-7

Spec URL: http://oriya.sourceforge.net/0.18-7/nabi.spec

SRPM URL: http://oriya.sourceforge.net/0.18-7/nabi-0.18-7.fc8.src.rpm
Comment 37 Jens Petersen 2007-09-05 03:12:07 EDT
Here is the source diff output for the record:
$ diff -ur nabi-0.18 nabi-0.18_fedora
Only in nabi-0.18/themes: MSWindows
Only in nabi-0.18/themes: MSWindowsXP
Only in nabi-0.18/themes: Mac

All issues have been addressed and the package looks good to me.

Thanks Subhransu for your patience and work on this.
I am happy to approve the package. :)

BTW note if you want to build the package for F7 then it will have to
be with the older release since f7 libhangul is still 0.0.4 which is not
new enough to build the latest nabi.

APPROVED.
Comment 38 Jens Petersen 2007-09-05 03:42:36 EDT
It would help if you could do one so-called package "pre-review"
and I am happy to sponsor you for cvsextras. :-)
Comment 39 subhransu behera 2007-09-07 02:21:33 EDT
New Package CVS Request
=======================
Package Name: nabi
Short Description: Hangul X Input Method 
Owners: sbehera
Branches: F-7
InitialCC: petersen
Cvsextras Commits: Yes
Comment 40 Kevin Fenzi 2007-09-07 14:37:46 EDT
cvs done.
Comment 41 Parag AN(पराग) 2007-09-20 02:06:23 EDT
Closing this review as package already got built successfully on koji.

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