Bug 427824 - Review Request: tmispell-voikko - An Ispell compatible front-end for spell-checking modules
Review Request: tmispell-voikko - An Ispell compatible front-end for spell-ch...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-07 13:20 EST by Ville-Pekka Vainio
Modified: 2008-05-14 15:56 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-14 15:56:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to support Voikko (tmispell) in legacy KSpell (5.54 KB, patch)
2008-01-07 21:32 EST, Kevin Kofler
no flags Details | Diff

  None (edit)
Description Ville-Pekka Vainio 2008-01-07 13:20:25 EST
Spec URL: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
SRPM URL: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.6.3-0.1.fc8.src.rpm
Description:
Tmispell is an Ispell compatible front-end for spell-checking
modules. To do the actual spell-checking for Finnish language it uses
the spell-checking system Voikko.

I've left some comments and some commented-out parts into the SPEC file. They will hopefully help KDE (etc.) integration later on, if someone modifies /usr/bin/ispell to call tmispell when needed.
Comment 1 Kevin Kofler 2008-01-07 21:32:00 EST
Created attachment 291031 [details]
Patch to support Voikko (tmispell) in legacy KSpell

Here's a patch to add support for Voikko to kdelibs3 (legacy KSpell interface).
This patch applies on top of my kspell-hunspell patch (as they touch the same
pieces of code). KDE 4 doesn't need patching because it uses Enchant. The
KSpell2 interface in kdelibs3 should be handled by the kspell2-enchant patch
already.

I can apply this patch to our kdelibs3 if you think it's the best solution.
Comment 2 Ville-Pekka Vainio 2008-01-23 18:25:35 EST
I'd like to mention two known issues for this package, which might come up in
the review:

- The curses interface doesn't seem to handle capital international letters
(like 'Ä') correctly.

- There is a separate copy of the glibmm library in the package. That would
require fixing, but right now upstream doesn't have the time (or possibly the
interest) to do it and I probably wouldn't have the skills...

I hope the package would still be OK for Fedora, since the curses interface is
quite a nice way of using libvoikko and the enchant-voikko subpackage would be
very usable in Gnome and KDE4.
Comment 3 Ville-Pekka Vainio 2008-02-12 12:20:50 EST
New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.1.rc1.fc8.src.rpm

This new 0.7rc1 removes the glibmm copy that was shipped in the package and
tmispell-voikko now uses the glibmm library in the system. I have tested that
this package builds with mock in F-8 and Rawhide. It also works in F-8 for me
(and I see no reason for it not working on Rawhide).

Basically the only issue with this package now should be the curses interface
UTF-8 problem, but it's not severe and if upstream fixes it, I'll update the
Fedora package.

I hope dropping the glibmm copy makes this package a bit less daunting to review ;)
Comment 4 Ville-Pekka Vainio 2008-02-12 19:07:50 EST
New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.2.rc2.fc8.src.rpm

This new version uses ncursesw instead of ncurses, so the UTF-8 problem I first
mentioned in comment #2 is fixed now.
Comment 5 Ville-Pekka Vainio 2008-02-17 07:51:12 EST
Upstream released the first stable version of 0.7.
New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.3.fc8.src.rpm

There should only be minor changes compared to the previous RCs.
Comment 6 Ville-Pekka Vainio 2008-03-24 18:54:43 EDT
I contacted the Fedora Enchant maintainer and he will put Enchant 1.4 into
Fedora 9. As Enchant 1.4 includes the Enchant Voikko provider, I dropped the
enchant-voikko subpackage from tmispell-voikko.

New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.4.fc8.src.rpm
Comment 7 Ville Skyttä 2008-05-11 03:59:11 EDT
- configuring with --disable-dependency-tracking would result in a cleaner build
log and a possible build speedup

- glibmm24-devel pulls in glib2-devel and glib2-devel pulls in pkgconfig so the
build deps on glib2-devel and pkgconfig could be dropped if you like

Both are non-blocker suggestions only, approved.
Comment 8 Ville-Pekka Vainio 2008-05-11 11:13:49 EDT
Thanks for taking the review. I just realized that I can make building the
enchant provider conditional with the '--with' switch of rpmbuild. I've done it
in this new version, so that people who want to build this package for releases
without Enchant 1.4 can also get the Voikko Enchant provider built. Could you
please take another look at the new spec file to make sure everything is ok?

New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.5.fc8.src.rpm
Comment 9 Kevin Kofler 2008-05-11 11:17:28 EDT
So how do we get support for this into KDE (the apps which use the old 
KSpell/K3Spell - stuff using KSpell2/Sonnet already supports it through 
Enchant)? Shall I apply my patch from comment #1?
Comment 10 Ville-Pekka Vainio 2008-05-11 12:36:40 EDT
Kevin: Once this package is in, you could apply the patch. I think the best way
of doing this would be to get the ispell shell script to call tmispell. I
haven't talked to the aspell package maintainer yet, though. But at least in the
mean time that patch would be useful.

It's also worth noting that Fedora 9 doesn't yet have Enchant 1.4, so
KSpell2/Sonnet won't work yet either. I have been talking to the enchant package
maintainer Marc Maurer, he will hopefully put Enchant 1.4 into F9 and F8 as an
update, but it will require some testing first. In case Enchant 1.4 won't go
into F9/F8, I guess I could build the Voikko provider from this package in those
releases as well. We'll see about that when Marc gets some testing done. I'm
also adding him as CC, I hope that's ok.

Comment 11 Ville Skyttä 2008-05-11 16:19:56 EDT
(In reply to comment #8)
> Could you
> please take another look at the new spec file to make sure everything is ok?

The enchant-voikko subpackage is missing a %defattr in %files.

When built with enchant support, does something that gets pulled in by library
dependencies own the %{_libdir}/enchant dir?

Are both the versioned and unversioned %{_libdir}/enchant/*.so* needed?

Also, if you like, things could be simplified a bit by replacing the toplevel
%define with "%bcond_with enchant", using %{with enchant} instead of
%{with_enchant}, and removing the %if around the enchant-voikko subpackage
definition (it's unnecessary).
Comment 12 Ville-Pekka Vainio 2008-05-12 09:21:15 EDT
New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.6.fc8.src.rpm

(In reply to comment #11)
> The enchant-voikko subpackage is missing a %defattr in %files.

Fixed.


> When built with enchant support, does something that gets pulled in by library
> dependencies own the %{_libdir}/enchant dir?

Yes, the main enchant package.

> Are both the versioned and unversioned %{_libdir}/enchant/*.so* needed?

I did some testing. Enchant seems to expect libenchant_voikko.so (which is a
symlink to libenchant_voikko.so.1.0.0), if there's only
libenchant_voikko.so.1.0.0 without the symlink, it doesn't work. However, I see
no use for libenchant_voikko.so.1, so that's now removed. I asked in the Voikko
mailing list if it would be ok to just rename libenchant_voikko.so.1.0.0 to
libenchant_voikko.so, but I haven't gotten an answer yet.

Marc: If you happen to read this, any advice?

The enchant-devel package seems to have the .a files for the other providers
(libenchant_aspell.a and libenchant_myspell.a), so I added an
enchant-voikko-devel package, which only has libenchant_voikko.a.

> Also, if you like, things could be simplified a bit by replacing the toplevel
> %define with "%bcond_with enchant", using %{with enchant} instead of
> %{with_enchant}, and removing the %if around the enchant-voikko subpackage
> definition (it's unnecessary).

Thanks for the tip, I've done so.
Comment 13 Ville-Pekka Vainio 2008-05-12 12:56:32 EDT
I've gotten a reply from the Voikko main developer, it should be ok to move
libenchant_voikko.so.1.0.0 to libenchant_voikko.so and it does work, so that's
what I've done in this version.

New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.7.fc8.src.rpm
Comment 14 Ville Skyttä 2008-05-12 13:26:14 EDT
Which applications use/require the static *.a file, and how?  Static libraries
are frowned upon in Fedora and shouldn't be shipped without a good reason.  If
the *.a here needs to be shipped, some work is needed to meet the packaging
guidelines.
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

Something to forward upstream (if it works): if the versioned *.so.* are not
needed, I think the proper way to get rid of them would be to change
libenchant_voikko_la_LDFLAGS to something like "-lvoikko -module -avoid-version
-no-undefined" in enchant/Makefile.am.
Comment 15 Ville-Pekka Vainio 2008-05-12 15:17:50 EDT
I've been under the impression that *.a files go into the -devel packages and
*.la files should not be packaged at all. Also, the guidelines explicitly say
nothing about *.a files, only about *.la files.

In my F8 system, the command "locate *.a|grep ^/usr/lib|wc -l" shows that I have
at least 109 *.a files in the libdir. Piping the grep result to "xargs rpm -qf"
shows, that most of these files (at least 85) indeed come from -devel packages.

As far as I understand, the *.a files can be used to statically link the library
when compiling something under Fedora. I'm not sure if it's actually that useful
in this case, but I added it for consistency, as the other Enchant providers
also have the *.a files shipped in a -devel package.

This is definitely not a blocker for me, if you want me to remove the *.a file,
I will.
Comment 16 Kevin Kofler 2008-05-12 18:30:44 EDT
Static libraries in -devel are deprecated, they should be either dropped or 
shipped in a -static package according to the current guidelines.
Comment 17 Ville-Pekka Vainio 2008-05-13 06:18:51 EDT
(In reply to comment #16)
> Static libraries in -devel are deprecated, they should be either dropped or 
> shipped in a -static package according to the current guidelines.

After re-reading the guidelines, yes, you both are right about this. So I have
now dropped the static library in this version.

New Spec: http://vpv.fedorapeople.org/packages/tmispell-voikko.spec
New SRPM: http://vpv.fedorapeople.org/packages/tmispell-voikko-0.7-0.8.fc8.src.rpm

Comment 18 Ville Skyttä 2008-05-13 12:53:30 EDT
Looks good again now.
Comment 19 Ville-Pekka Vainio 2008-05-13 13:13:33 EDT
Thanks! BTW, in case I need to get this package built with the Enchant Voikko
provider in Fedora, can I pass the '--with enchant' switch to koji somehow?

Then the CVS part:

New Package CVS Request
=======================
Package Name: tmispell-voikko
Short Description: An Ispell compatible front-end for spell-checking modules
Owners: vpv
Branches: F-8 F-9
Cvsextras Commits: yes
Comment 20 Kevin Kofler 2008-05-13 13:15:54 EDT
I don't think you can do that, you'll have to edit the specfile to set the 
flag.
Comment 21 Kevin Fenzi 2008-05-14 11:39:24 EDT
cvs done.
Comment 22 Ville-Pekka Vainio 2008-05-14 15:56:19 EDT
The package is built for releases between F-8 and Rawhide, I'll close this bug.
Thanks!

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