345080 - Allow use of external hunspell library

Status: RESOLVED FIXED

(Core :: Spelling checker, enhancement)

Description

[Mike Hommey [:glandium]]

Everything is in the summary. It would be nice to allow to link against an external library instead of the included one. Patch following

Comment 1

[Mike Hommey [:glandium]]

Attachment: Patch for the issue

Tested on debian with firefox 2.0b1. Works nicely. The only issue is with some dictionaries that ship both en-US and en_US, firefox then displays both, with only one with its full name. But that happens as well when using the embedded myspell and using system dictionaries like it's already done in debian's thunderbird package.

Comment 2

[Mike Hommey [:glandium]]

Attachment: New patch

It's better without configure

Comment 3

[Benjamin Smedberg]

Comment on attachment 229696 [details] [diff] [review]
New patch

Brett, this patch looks ok to me build-wise... do you have objections to supporting system-myspell? Do we have custom myspell patches in the tree?

Comment 4

[Brett Wilson]

(In reply to comment #3)
> (From update of attachment 229696 [details] [diff] [review] [edit])
> Brett, this patch looks ok to me build-wise... do you have objections to
> supporting system-myspell?

I have no opinion. I don't know how the spellchecker is linked, or anything about the myspell stuff: I've been working one level higher than this. Perhaps mscott has an idea?

> Do we have custom myspell patches in the tree?

I do not know.

Comment 5

[Benjamin Smedberg]

Comment on attachment 229696 [details] [diff] [review]
New patch

Off to mscott, then ;-)

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #3)
>  Do we have custom myspell patches in the tree?

We have patches for using the mozilla charset code instead of custom myspell code. But other than that, i don't think we have custom code.

Comment 7

[Scott MacGregor]

(In reply to comment #3)
> (From update of attachment 229696 [details] [diff] [review] [edit])
> Do we have custom myspell patches in the tree?

Here's a summary of the changes we made to myspell when it came over from Open Office: 

http://lxr.mozilla.org/seamonkey/source/extensions/spellcheck/myspell/src/README.mozilla

Comment 8

[Brett Wilson]

I don't understand why we need this patch. I see no benefit, and we're more likely to have compatibility problems.

Comment 9

[Mike Hommey [:glandium]]

It doesn't have a benefit for you, but it has for distributors and for users. No need to ship a myspell with every product from mozilla.org, on top of openoffice and all others software using it. You know, the old thing about shared libraries...

Comment 10

[timeless]

there will eventually only be one gecko library, so the argument about shipping is mostly not going to fly.

that said, this looks dead in the water given that mscott hasn't looked at it in 2 months.

Comment 11

[Scott MacGregor]

Comment on attachment 229696 [details] [diff] [review]
New patch

We're moving to hunspell anyway so it doesn't make sense to try to do this now.

I'm also worried about compatibility issues this might open up especially since we patch myspell as it is (although the patching we do may be purely build related).

Comment 12

[Gergan Penkov]

(In reply to comment #11)
> (From update of attachment 229696 [details] [diff] [review])
> We're moving to hunspell anyway so it doesn't make sense to try to do this now.
> 
> I'm also worried about compatibility issues this might open up especially since
> we patch myspell as it is (although the patching we do may be purely build
> related).
> 
firefox trunk works without problems built against system hunspell-1.1.5
hunspell is backwards compatible with myspell and unless you are going to break API/ABI with upstream there should be no problems with allowing the distributors to build against system myspell/hunspell

Comment 13

[Alexander Sack]

From what I read in this bug, you don't want to add system myspell support since you are switching to hunspell anyway ... so would you be willing to accept a dedicated system hunspell patch instead?

Comment 14

[Reed Loden [:reed]]

Mike, could you supply a new patch for hunspell instead?

Comment 15

[Mike Hommey [:glandium]]

Attachment: Patch for native hunspell

New patch for hunspell

Comment 16

[Mike Hommey [:glandium]]

Scott, could you review this patch ?

Comment 17

[Alexey Gladkov]

Attachment: hunspell with libxul

Little addon to able build libxul with system hunspell.

Comment 18

[Mike Hommey [:glandium]]

Attachment: New version of the patch

This instance of the patch includes bits from attachment #302393 [details] [diff] [review], and also adds some necessary changes to avoid a failure to build when using hidden visibility. The #include change is necessary to avoid using the hunspell.hxx file in the hunspell/src directory instead of the system wrapper.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Actually working patch

The previous patch lacked the necessary bit in config/Makefile.in to have the system_wrapper created.

Comment 20

[Benjamin Smedberg]

Comment on attachment 302429 [details] [diff] [review]
Actually working patch

Looks ok to me... we should run this against the tryserver to make sure it works across the board.

Comment 21

[Alexey Gladkov]

It seems Thunderbird does not build with external hunspell.

Comment 22

[Mike Hommey [:glandium]]

(In reply to comment #21)
> It seems Thunderbird does not build with external hunspell.

Could you copy/paste the build error message ?

Comment 23

[Reed Loden [:reed]]

Comment on attachment 302429 [details] [diff] [review]
Actually working patch

Allow Linux distributions to use system Hunspell.

Comment 24

[Alexey Gladkov]

(In reply to comment #22)
> (In reply to comment #21)
> > It seems Thunderbird does not build with external hunspell.
> 
> Could you copy/paste the build error message ?
> 

c++  -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -pedantic -pipe -Wall -O2 -march=i586 -mtune=i686 -fPIC -DPIC -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O2 -fPIC -shared -Wl,-z,defs -Wl,-h,libspellchecker.so -o libspellchecker.so  mozSpellCheckerFactory.o mozSpellChecker.o mozPersonalDictionary.o mozEnglishWordUtils.o mozGenericWordUtils.o mozSpellI18NManager.o mozInlineSpellChecker.o mozInlineSpellWordUtil.o     -lpthread  -Wl,-rpath-link,../../../dist/bin  -Wl,--whole-archive ../hunspell/src/libhunspell_s.a -Wl,--no-whole-archive -L../../../dist/bin -L../../../dist/lib -L../../../dist/bin -lxpcom -lxpcom_core  -L../../../dist/bin -L/usr/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../../../dist/lib/libunicharutil_s.a  -Wl,--version-script -Wl,../../../build/unix/gnu-ld-scripts/components-version-script -Wl,-Bsymbolic -ldl -lm    
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::Check(unsigned short const*, int*)':
mozHunspell.cpp:(.text+0x509): undefined reference to `Hunspell::spell(char const*, int*, char**)'
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::~mozHunspell()':
mozHunspell.cpp:(.text+0x5ac): undefined reference to `Hunspell::~Hunspell()'
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::~mozHunspell()':
mozHunspell.cpp:(.text+0x65c): undefined reference to `Hunspell::~Hunspell()'
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::~mozHunspell()':
mozHunspell.cpp:(.text+0x706): undefined reference to `Hunspell::~Hunspell()'
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::SetDictionary(unsigned short const*)':
mozHunspell.cpp:(.text+0x98b): undefined reference to `Hunspell::~Hunspell()'
mozHunspell.cpp:(.text+0x9e1): undefined reference to `Hunspell::Hunspell(char const*, char const*)'
mozHunspell.cpp:(.text+0xa73): undefined reference to `Hunspell::get_dic_encoding()'
mozHunspell.cpp:(.text+0xac6): undefined reference to `Hunspell::get_dic_encoding()'
../hunspell/src/libhunspell_s.a(mozHunspell.o): In function `mozHunspell::Suggest(unsigned short const*, unsigned short***, unsigned int*)':
mozHunspell.cpp:(.text+0xce5): undefined reference to `Hunspell::suggest(char***, char const*)'
collect2: ld returned 1 exit status
gmake[4]: *** [libspellchecker.so] Error 1
gmake[4]: Leaving directory `/tmp/mozilla/extensions/spellcheck/src'
gmake[3]: *** [libs] Error 2
gmake[3]: Leaving directory `/tmp/mozilla/extensions/spellcheck'
gmake[2]: *** [libs_tier_toolkit] Error 2
gmake[2]: Leaving directory `/tmp/mozilla'
gmake[1]: *** [tier_toolkit] Error 2
gmake[1]: Leaving directory `/tmp/mozilla'
make: *** [default] Error 2
make: Leaving directory `/tmp/mozilla'

Comment 25

[Mike Hommey [:glandium]]

Can you try this additional patch ?

--- a/extensions/spellcheck/src/Makefile.in
+++ b/extensions/spellcheck/src/Makefile.in
@@ -90,6 +90,12 @@ EXTRA_DSO_LDOPTS = \
                $(MOZ_UNICHARUTIL_LIBS) \
                $(NULL)
 
+ifdef FORCE_STATIC_LIB
+ifdef MOZ_NATIVE_HUNSPELL
+EXTRA_DSO_LDOPTS += $(MOZ_HUNSPELL_LIBS)
+endif
+endif
+
 include $(topsrcdir)/config/rules.mk
 
 LOCAL_INCLUDES += \

(Not sure FORCE_STATIC_LIB is actually appropriate)

Comment 26

[Mike Hommey [:glandium]]

oops, the first ifdef should read ifndef.

Comment 27

[Alexey Gladkov]

Yes. It solves the problem. I think, that FORCE_STATIC_LIB it is not necessary here.

Comment 28

[Alexey Gladkov]

I think, that it is better to make so:

--- mozilla/extensions/spellcheck/src/Makefile.in 
+++ mozilla/extensions/spellcheck/src/Makefile.in 
@@ -87,8 +87,9 @@ endif 
 EXTRA_DSO_LDOPTS = \ 
                $(LIBS_DIR) \ 
                $(MOZ_COMPONENT_LIBS) \ 
                $(MOZ_UNICHARUTIL_LIBS) \ 
+               $(MOZ_HUNSPELL_LIBS) \ 
                $(NULL) 
  
 include $(topsrcdir)/config/rules.mk 
  

Comment 29

[Reed Loden [:reed]]

Mike, can you put together an updated patch for check-in? I can run it on the try server to make sure it works ok.

Comment 30

[Mike Hommey [:glandium]]

Attachment: Updated patch

I removed a useless AC_SUBST (PKC_CHECK_MODULES does it) and added the missing bits for component build (I used a ifdef MOZ_HUNSPELL_NATIVE, just in case MOZ_HUNSPELL_LIBS would take a value even when MOZ_HUNSPELL_NATIVE is not defined at some time in the future ; better safe than sorry)

Comment 31

[Mike Hommey [:glandium]]

Attachment: Updated patch

Actually, forget it about MOZ_HUNSPELL_LIBS and MOZ_NATIVE_HUNSPELL...

Comment 32

[Reed Loden [:reed]]

Comment on attachment 303218 [details] [diff] [review]
Updated patch

I'll run this through the try server to make sure it works, but since you already have approval, no need to ask for it again for such a small change that I would have to do anyway as a bustage fix if I had landed the original patch.

Comment 33

[Reed Loden [:reed]]

Hmm, you must be doing this from some non-pristine tree, as your config/Makefile.in changes don't apply correctly (due to system-bz2 being added). I can work around that, so it's not that big of a deal, but just thought I would mention it.

I don't see where MOZ_HUNSPELL_LIBS and MOZ_HUNSPELL_CFLAGS get set at all to something actually useful, so I'm unsure how this patch will even work, or am I missing something?

Comment 34

[Mike Hommey [:glandium]]

PKG_CHECK_MODULES(MOZ_HUNSPELL, hunspell) sets them. And does the AC_SUBST.

Comment 35

[Mike Hommey [:glandium]]

(In reply to comment #33)
> Hmm, you must be doing this from some non-pristine tree, as your
> config/Makefile.in changes don't apply correctly (due to system-bz2 being
> added). I can work around that, so it's not that big of a deal, but just
> thought I would mention it.

Oops sorry for this.

Comment 36

[Reed Loden [:reed]]

(In reply to comment #34)
> PKG_CHECK_MODULES(MOZ_HUNSPELL, hunspell) sets them. And does the AC_SUBST.

Ok, thanks! That answers my question. I'll land this for you as soon as the tree is green, which may end up being tomorrow.

Comment 37

[Reed Loden [:reed]]

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1931; previous revision: 1.1930
done
Checking in config/Makefile.in;
/cvsroot/mozilla/config/Makefile.in,v  <--  Makefile.in
new revision: 3.145; previous revision: 3.144
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 3.448; previous revision: 3.447
done
Checking in config/system-headers;
/cvsroot/mozilla/config/system-headers,v  <--  system-headers
new revision: 3.36; previous revision: 3.35
done
Checking in extensions/spellcheck/hunspell/src/Makefile.in;
/cvsroot/mozilla/extensions/spellcheck/hunspell/src/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/spellcheck/hunspell/src/mozHunspell.h;
/cvsroot/mozilla/extensions/spellcheck/hunspell/src/mozHunspell.h,v  <--  mozHunspell.h
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/spellcheck/src/Makefile.in;
/cvsroot/mozilla/extensions/spellcheck/src/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/library/libxul-rules.mk;
/cvsroot/mozilla/toolkit/library/libxul-rules.mk,v  <--  libxul-rules.mk
new revision: 1.22; previous revision: 1.21
done