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.

And:
* %{_sbindir}/alternatives must be added to Requires(....)

ping?

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.

 

Ok, please update with the new .spec file and srpm url when it is ready.

By the way are there any information when upstream will
release new version?

It was already released on the 8th, see:

http://kldp.net/frs/?group_id=275

Subhransu, if you can update your submission fixing the issues raised so far
I am happy to review this.

I am waiting for newer version of libhangul (0.18) to be built.

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.

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

Created attachment 160216 [details]
nabi.spec-4.patch

some more fixes - please check I didn't break anything. :)

Created attachment 160217 [details]
nabi.desktop

(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 ?

(In reply to comment #14)
>   Which package should own %_sysconfdir/X11/xinit/xinput.d ?

I think it should be owned by im-chooser now.

So actually it should be the nabi config file listed there not the directory.

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

(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. :)

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.

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

* 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

Other things seems okay for me.

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.

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

Seems okay to me.

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. :)

Created attachment 160954 [details]
nabi.spec-6.patch

this patch fixes the above:

- remove unnecessary desktop file
- tidy doc filelist

I contacted upstream about the questionable themes which we are excluding from
the package.  I hope they can be removed in a future release.

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

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.

Created attachment 161708 [details]
nabi-remove-bad-themes

A small shellscript does the theme removal.

Well, what is the status of this bug?

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.

Okay, thank you for imformation.

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>.

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

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.

It would help if you could do one so-called package "pre-review"
and I am happy to sponsor you for cvsextras. :-)

New Package CVS Request
=======================
Package Name: nabi
Short Description: Hangul X Input Method 
Owners: sbehera
Branches: F-7
InitialCC: petersen
Cvsextras Commits: Yes

cvs done.

Closing this review as package already got built successfully on koji.