Bug 581619

Summary: Review Request: mlterm - Multi Lingual TERMinal emulator on X
Product: [Fedora] Fedora Reporter: Muayyad Alsadi <alsadi>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: araby.ahmed, fedora-package-review, hicham.haouari, martin.gieseking, notting, pahan
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-12-26 15:19:25 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    
Attachments:
Description Flags
diff to fix the build failure none

Description Muayyad Alsadi 2010-04-12 15:47:48 EDT
Spec URL: http://www.ojuba.org/downloads/misc/SPEC/mlterm.spec
SRPM URL: http://www.ojuba.org/downloads/updates/3/SRPMS/mlterm-3.0.0-1.oj3.src.rpm
Description:
mlterm is a multi-lingual terminal emulator written from
scratch, which supports various character sets and encodings
in the world.  It also supports various unique feature such as
anti-alias using FreeType, multiple windows, scrollbar API,
scroll by mouse wheel, automatic selection of encoding,
and so on. Multiple xims are also supported. 
You can dynamically change various xims.
Comment 1 Muayyad Alsadi 2010-04-12 16:32:03 EDT
there was some missing BuildRequires added to the same SPEC URL above with help from boelkmoeller3 on IRC
Comment 2 Martin Gieseking 2010-04-16 04:10:55 EDT
Hi Muayyad,

there are many things to be fixed (see rpmlint output below). Please have a look at the packaging guidelines (https://fedoraproject.org/wiki/Packaging/Guidelines) and polish the spec file accordingly. Also, the above srpm and spec files are out of sync. 


$ rpmlint /var/lib/mock/fedora-12-x86_64/result/*.rpm
mlterm.src: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti
mlterm.src: W: spelling-error %description -l en_US multi -> mulch, mufti
mlterm.src: W: spelling-error %description -l en_US encodings -> encoding, encoding s, encodes
mlterm.src: W: spelling-error %description -l en_US scrollbar -> scroll bar, scroll-bar, scroll
mlterm.src: W: spelling-error %description -l en_US xims -> xis, aims, rims
mlterm.src: W: no-version-in-last-changelog
mlterm.src: W: invalid-license Modified BSD-style license
mlterm.src:24: W: setup-not-quiet
mlterm.src:9: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 4)
mlterm.x86_64: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti
mlterm.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
mlterm.x86_64: W: spelling-error %description -l en_US encodings -> encoding, encoding s, encodes
mlterm.x86_64: W: spelling-error %description -l en_US scrollbar -> scroll bar, scroll-bar, scroll
mlterm.x86_64: W: spelling-error %description -l en_US xims -> xis, aims, rims
mlterm.x86_64: W: no-version-in-last-changelog
mlterm.x86_64: W: invalid-license Modified BSD-style license
mlterm.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/mlconfig ['/usr/lib64', '/usr/local/lib']
mlterm.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/mlterm ['/usr/lib64', '/usr/local/lib']
mlterm.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/mlterm-menu ['/usr/lib64']
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/color
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/vfont
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libmkf.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mkf/libmkf_8bits.a
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/taafont
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libmkf.so
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mkf/libmkf_jajp.a
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/key
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/xim
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libsample.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libkik.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libnext.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mkf/libmkf_zh.a
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/aafont
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libim-kbd.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mkf/libmkf_kokr.a
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/main
mlterm.x86_64: E: postin-without-ldconfig /usr/lib64/libmkf.so.14.0.0
mlterm.x86_64: E: postun-without-ldconfig /usr/lib64/libmkf.so.14.0.0
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libathena.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libmotif.a
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/font
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/menu
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/termcap
mlterm.x86_64: W: file-not-utf8 /usr/share/doc/mlterm-3.0.0/ja/README.aafont
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/vaafont
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libkik.so
mlterm.x86_64: E: postin-without-ldconfig /usr/lib64/libkik.so.11.0.0
mlterm.x86_64: E: postun-without-ldconfig /usr/lib64/libkik.so.11.0.0
mlterm.x86_64: W: non-conffile-in-etc /etc/mlterm/tfont
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libsample2.a
mlterm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/mlterm/libmozmodern.a
mlterm.x86_64: W: file-not-in-%lang /usr/share/locale/ar/LC_MESSAGES/mlconfig.mo
mlterm.x86_64: W: file-not-in-%lang /usr/share/locale/de/LC_MESSAGES/mlconfig.mo
mlterm.x86_64: W: file-not-in-%lang /usr/share/locale/ja/LC_MESSAGES/mlconfig.mo
mlterm.x86_64: W: file-not-in-%lang /usr/share/locale/vi/LC_MESSAGES/mlconfig.mo
mlterm.x86_64: W: file-not-in-%lang /usr/share/locale/zh_TW/LC_MESSAGES/mlconfig.mo
mlterm-debuginfo.x86_64: W: no-version-in-last-changelog
mlterm-debuginfo.x86_64: W: invalid-license Modified BSD-style license
3 packages and 0 specfiles checked; 7 errors, 51 warnings.
Comment 3 Muayyad Alsadi 2010-04-16 09:16:24 EDT
I have updated the spec file (same URL)

what todo with those ?

mlterm.i586: W: devel-file-in-non-devel-package /usr/lib/libkik.so
mlterm.i586: W: devel-file-in-non-devel-package /usr/lib/libmkf.so

they are .so why rpmlint consider them -devel like a and la ?

regarding this

mlterm.i586: W: file-not-in-%lang /usr/share/locale/ar/LC_MESSAGES/mlconfig.mo

what does this mean and how to fix it ?

regarding license what is the name should I use instead of "Modified BSD-style license"

regarding spelling I think the rest (like xims, the plural of XIM) are terms and they need to be accepted as-is

regarding having .spec in sync with .src.rpm I'll do that ASAP,
after we fix those issues.
Comment 4 Martin Gieseking 2010-04-16 10:50:54 EDT
First of all, please bump the release number every time you provide an updated spec file. Otherwise it's hard to distinguish between the various versions.

Replace "%define pixmapdir /usr/share/pixmaps" with "%global pixmapdir %{_datadir}/pixmaps"

Fix Source0 according to http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

The allowed license tags are listed here: http://fedoraproject.org/wiki/Licensing. The file LICENSE contains the BSD license text (3 clause variant), so the corresponding license tag should be BSD. However, folder "inputmethod" contains several files licensed under LGPLv2+ and MIT. Thus, the resulting License for the package is "BSD and LGPLv2+ and MIT" if everything goes to one package.

Instead of calling configure directly, you should use the macro %configure which automatically applies the correct directories. In this case you can drop the CFLAGS and --FOOdir=%{_FOOdir} lines.

The locale files must be properly added using %find_lang. See http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files for further information

The .desktop file must be installed with desktop-file-install or desktop-file-validate (see http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files)

Use the code snippets from http://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets#GTK.2B_icon_cache for updating the GTK+ icon cache

Change %defattr(-,root,root) to %defattr(-,root,root,-)

If the bundled libraries libkik and libmkf are only relevant for the mlterm binaries, and not used for further third-party packages, they can stay in the base package. In this case you should remove the development files libkik.so and libmkf.so. However, it might be better to put the two libraries in separate subpackages.
Comment 5 Muayyad Alsadi 2010-04-19 11:30:02 EDT
updating accordingly

> The locale files must be properly added using %find_lang.
I used
%find_lang mlconfig

for some why mlterm gettext name is mlconfig.
anyway, I test mlterm in a locale env and it worked
Comment 6 Muayyad Alsadi 2010-04-21 10:14:27 EDT
up!

same URL above
Comment 7 Ahmed M. Araby 2010-04-29 14:03:34 EDT
works fine for me
Comment 8 Muayyad Alsadi 2010-05-24 17:13:55 EDT
up!
Comment 9 Hicham HAOUARI 2010-06-02 18:57:02 EDT
build failure on f-14 : 

http://koji.fedoraproject.org/koji/getfile?taskID=2226143&name=build.log
Comment 10 Hicham HAOUARI 2010-06-02 19:46:00 EDT
Created attachment 419211 [details]
diff to fix the build failure
Comment 11 Hicham HAOUARI 2010-06-02 21:08:03 EDT
F-14 : http://koji.fedoraproject.org/koji/taskinfo?taskID=2226234
Comment 12 Muayyad Alsadi 2010-06-03 11:42:20 EDT
the latest .src.rpm with the suggested patch
http://www.ojuba.org/downloads/updates/testing/4/SRPMS/mlterm-3.0.0-3.oj4.src.rpm

to test it, I've built it on F13

http://www.ojuba.org/downloads/updates/testing/4/x86_64/mlterm-3.0.0-3.oj4.x86_64.rpm
Comment 13 Hicham HAOUARI 2010-06-08 07:49:40 EDT
1)Please provide a direct link to the spec everytime you update the package.

2)The %{_libexecdir}/* should be replaced IMHO with the corresponding files separately.

3)You forgot to update the changelog.

4)BuildRequires : gtk+-devel, better put gtk2-devel

5)Patch should be upstreamed ( put a link to the corresponding upstream bugzilla bug report )

6)As mentioned above, change "sed -i -re 's/#ifdef  HAVE_ISASTREAM/#ifdef HAVE_STROPTS_H/' kiklib/src/kik_pty_streams.c" to a patch and upstream it. Same for fribidi-config

7)The desktop file comment should be changed IMHO :
"Use the command line with Arabic support" to "Use the command line with multilingual support", and the arabic comment also.

8)It is up to you, but the pixmapdir macro is really unneeded
Comment 14 Muayyad Alsadi 2010-06-08 10:17:54 EDT
> 1) Please provide a direct link to the spec everytime you update the package.

no problem, I'll do that

> 2)The %{_libexecdir}/* should be replaced IMHO with the corresponding files separately.

is there a reason for that ? they are files that might change from version to another

> 3)You forgot to update the changelog.
ok

> 4)BuildRequires : gtk+-devel, better put gtk2-devel

we can't do that, the config window of mlterm is a gtk1 application

> 5)Patch should be upstreamed ( put a link to the corresponding upstream
bugzilla bug report )

if you are referring to your patch in #11 then it just -lx11 to build option, it could be missed because of many reasons, and BTW it used to build in F11 and F12 without this patch, so if the upstream is using fedora<13 he won't be convinced that there is any problem


> 6)As mentioned above, change "sed -i -re 's/#ifdef  HAVE_ISASTREAM/#ifdef
HAVE_STROPTS_H/' kiklib/src/kik_pty_streams.c" to a patch and upstream it. Same
for fribidi-config

the fribidi-config patch is no longer needed that's why it's commented (it was needed because the upstream used to use fribidi-config instead of pkg-config which is in fedora)

and the sed change is needed to build it, try building without it.
the function it uses "isastream" is from stropts.h I guess both are not in fedora so HAVE_STROPTS_H is equivalent to HAVE_ISASTREAM

http://www.opengroup.org/onlinepubs/009695399/functions/isastream.html
Comment 15 Muayyad Alsadi 2010-06-08 10:19:49 EDT
> 7)The desktop file comment should be changed IMHO : "Use the command line with Arabic support" to "Use the command line with multilingual support", and the arabic comment also.

actually (from my fedora IRC talks with users) it's intended for Arabic users, since Chinese and Japanese prefer another terminal emulator.
Comment 16 Hicham HAOUARI 2010-06-10 09:38:00 EDT
(In reply to comment #14)
> > 1) Please provide a direct link to the spec everytime you update the package.
> 
> no problem, I'll do that
> 
> > 2)The %{_libexecdir}/* should be replaced IMHO with the corresponding files separately.
> 
> is there a reason for that ? they are files that might change from version to
> another
that discards the whole point of explicitely specifying files in the %files section, please fix that
> 
> > 3)You forgot to update the changelog.
> ok
> 
> > 4)BuildRequires : gtk+-devel, better put gtk2-devel
> 
> we can't do that, the config window of mlterm is a gtk1 application
> 

The build i have done on comment 14 is not using gtk1

> > 5)Patch should be upstreamed ( put a link to the corresponding upstream
> bugzilla bug report )
> 
> if you are referring to your patch in #11 then it just -lx11 to build option,
> it could be missed because of many reasons, and BTW it used to build in F11 and
> F12 without this patch, so if the upstream is using fedora<13 he won't be
> convinced that there is any problem

All sane upstreams projects will understand the importance of the change in Fedora policy about dso linking

> 
> 
> > 6)As mentioned above, change "sed -i -re 's/#ifdef  HAVE_ISASTREAM/#ifdef
> HAVE_STROPTS_H/' kiklib/src/kik_pty_streams.c" to a patch and upstream it. Same
> for fribidi-config
> 
> the fribidi-config patch is no longer needed that's why it's commented (it was
> needed because the upstream used to use fribidi-config instead of pkg-config
> which is in fedora)
> 
> and the sed change is needed to build it, try building without it.
> the function it uses "isastream" is from stropts.h I guess both are not in
> fedora so HAVE_STROPTS_H is equivalent to HAVE_ISASTREAM
> 
> http://www.opengroup.org/onlinepubs/009695399/functions/isastream.html    
Then the code should check for either one of them.
Comment 17 Hicham HAOUARI 2010-06-10 09:38:59 EDT
By the way, why did you prefer imlib over gdk-pixbuf ?
Comment 18 Hicham HAOUARI 2010-06-12 08:01:09 EDT
09) pixmaps are not installed correctly, they should be placed in the hicolor icon theme corresponding folder
10) timestamps are not preserved
11) missing Requires : hicolor-icon-theme
12) gtk2-devel depends on libpng-devel, so libpng-devel in BR is unneeded
Comment 19 Hicham HAOUARI 2010-06-12 08:03:25 EDT
13) for the HAVE_ISASTREAM symbol, pass it in CFLAGS, that is better than sedding IMHO
Comment 20 Jason Tibbitts 2010-11-17 21:00:50 EST
I don't see any response to the above four comments.