Bugzilla will be upgraded to version 5.0. The upgrade date is tentatively scheduled for 2 December 2018, pending final testing and feedback.
Bug 958881 - Review Request: ibus-cangjie - IBus engine to input Cangjie and Quick
Review Request: ibus-cangjie - IBus engine to input Cangjie and Quick
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On: 954042
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-02 11:08 EDT by Mathieu Bridon
Modified: 2013-09-24 23:56 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-05-07 11:07:55 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mathieu Bridon 2013-05-02 11:08:32 EDT
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 15:06:23 EDT
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 00:29:12 EDT
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 01:13:19 EDT
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 11:01:58 EDT
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 11:54:03 EDT
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 12:24:49 EDT
(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 13:42:18 EDT
Git done (by process-git-requests).
Comment 8 Mathieu Bridon 2013-05-07 11:07:55 EDT
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.