Bug 958881 - Review Request: ibus-cangjie - IBus engine to input Cangjie and Quick
Summary: Review Request: ibus-cangjie - IBus engine to input Cangjie and Quick
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 954042
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-05-02 15:08 UTC by Mathieu Bridon
Modified: 2013-09-25 03:56 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-05-07 15:07:55 UTC
Type: ---
Embargoed:
panemade: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Mathieu Bridon 2013-05-02 15:08:32 UTC
Spec URL: http://bochecha.fedorapeople.org/packages/ibus-cangjie.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/ibus-cangjie-1.0-1.fc19.src.rpm

Description:
IBus engine for users of the Cangjie and Quick input methods.

It is primarily intended to Hong Kong people who want to input Traditional
Chinese, as they are (by far) the majority of Cangjie and Quick users.

However, it should work for others as well (e.g to input Simplified Chinese).

Fedora Account System Username: bochecha

Comment 1 Adam Williamson 2013-05-03 19:06:23 UTC
Not assigned to me, but I gave it a quick once-over anyway, I don't see anything at all wrong, passes all requirements for me. It might be useful to explain the use of %_prefix/lib - I'm sure there's a good reason for it, but it's the kind of thing that gets flagged (rpmlint complains). Other than that, looks good. I did a successful Rawhide scratch build, but F19 fails because python3-cangjie is not yet available; the update is still in updates-testing, I see.

Comment 2 Mathieu Bridon 2013-05-04 04:29:12 UTC
Thanks Adam for looking at it.

(In reply to comment #1)
> Not assigned to me, but I gave it a quick once-over anyway, I don't see
> anything at all wrong, passes all requirements for me. It might be useful to
> explain the use of %_prefix/lib - I'm sure there's a good reason for it, but
> it's the kind of thing that gets flagged (rpmlint complains).

This package is noarch, and as such is exempt from multilib, so %{_prefix}/lib is allowed as per:
https://fedoraproject.org/wiki/Packaging:Guidelines#Multilib_Exempt_Locations

Maybe I could add a comment in the spec file?

> Other than
> that, looks good. I did a successful Rawhide scratch build, but F19 fails
> because python3-cangjie is not yet available; the update is still in
> updates-testing, I see.

I could add a buildroot override if it helps. I'll need one anyway to build it once approved, so I don't mind creating it a bit in advance. :)

Comment 3 Parag AN(पराग) 2013-05-06 05:13:19 UTC
Review:

+ koji scratch build for f20 -> http://koji.fedoraproject.org/koji/taskinfo?taskID=5333725

+ rpmlint on rpms gave
ibus-cangjie.noarch: W: no-manual-page-for-binary ibus-setup-cangjie
ibus-cangjie.src:55: E: hardcoded-library-path in %{_prefix}/lib/%{name}
2 packages and 0 specfiles checked; 1 errors, 1 warnings.


+ Source verified with upstream as
http://ibus-cangjie.opensource.hk/downloads/ibus-cangjie/ibus-cangjie-1.0.tar.xz :
  CHECKSUM(SHA256) this package     : b337004848513edc035bae7babc5ec4fe25d9fcff463001fcebfbe7467a16764
  CHECKSUM(SHA256) upstream package : b337004848513edc035bae7babc5ec4fe25d9fcff463001fcebfbe7467a16764



Suggestions:
1) Desktop files should be installed as per given in 
https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

2) Good to add some comment in spec for rpmlint error of hardcoded-library-path

Comment 4 Mathieu Bridon 2013-05-06 15:01:58 UTC
Thanks for the comments Parag, here's a new submission.

Spec URL: http://bochecha.fedorapeople.org/packages/ibus-cangjie.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/ibus-cangjie-1.0-2.fc20.src.rpm

(In reply to comment #3)
> Suggestions:
> 1) Desktop files should be installed as per given in 
> https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-
> install_usage

Good catch, I added the desktop-file-validate thing for both desktop files.

But now I wonder, do you know of a way to have the Autotools stuff use desktop-file-install when installing the desktop files? (I'm upstream, so if that's easily doable, I'd rather get it done upstream)

> 2) Good to add some comment in spec for rpmlint error of
> hardcoded-library-path

Yeah, Adam asked for this as well above. I just added a quick note in the %files section.

Comment 5 Parag AN(पराग) 2013-05-06 15:54:03 UTC
1) I don't think you need to add any comments for desktop file installation like you added
"# Upstream doesn't validate their desktop files"
This is a standard packaging policy to install desktop files.

2) Autotools stuff just installs generated desktop file into path /usr/share/applications. I have not seen yet any project using either desktop-file-validate or some way to validate desktop file before getting installed.


3) New updated SRPM looks good.

One more thing I want to suggest is to use following way for make install line as this will preserve the timestamps of files from upstream tarball.

make install DESTDIR=%{buildroot} INSTALL="install -p"

APPROVED this package.

Comment 6 Mathieu Bridon 2013-05-06 16:24:49 UTC
(In reply to comment #5)
> 1) I don't think you need to add any comments for desktop file installation
> like you added
> "# Upstream doesn't validate their desktop files"
> This is a standard packaging policy to install desktop files.

I know, I left it mostly for me, as a reminder that I should try to find if there's a way I can do that upstream, so other distros also benefit. :)

> 2) Autotools stuff just installs generated desktop file into path
> /usr/share/applications. I have not seen yet any project using either
> desktop-file-validate or some way to validate desktop file before getting
> installed.

Yeah, me neither. :-/

I just thought I'd ask.

> 3) New updated SRPM looks good.
> 
> One more thing I want to suggest is to use following way for make install
> line as this will preserve the timestamps of files from upstream tarball.
> 
> make install DESTDIR=%{buildroot} INSTALL="install -p"

Oh, forgot about that one too. I'll fix it when I import the package!

> APPROVED this package.

Thanks a lot for the review Parag, this one and the python3-cangjie one. :)

New Package SCM Request
=======================
Package Name: ibus-cangjie
Short Description: IBus engine to input Cangjie and Quick
Owners: bochecha
Branches: f18 f19
InitialCC:

Comment 7 Gwyn Ciesla 2013-05-06 17:42:18 UTC
Git done (by process-git-requests).

Comment 8 Mathieu Bridon 2013-05-07 15:07:55 UTC
Thanks for the Git process, Jon.

Package built in all the requested branches, closing.


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