429747 - Fails to build with system hunspell 1.2

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

The change to config/config.mk in bug #412320, that has not been backed out, is provoking subtle problems with system hunspell.

What happens is that mozHunspell.o gets built with (simplified) -I. -I/usr/include/hunspell.

Now, the problem is that hunspell 1.2 defines Hunspell::Hunspell as follows:
  Hunspell(const char * affpath, const char * dpath, const char * key = NULL);

While hunspell 1.1 (included in mozilla tree) defines it as follows:
  Hunspell(const char * affpath, const char * dpath);

The result is that mozHunspell.o needs the Hunspell::Hunspell(const char * affpath, const char * dpath) symbol, and linking libxul with -lhunspell-1.2 fails because it only contains Hunspell::Hunspell(const char * affpath, const char * dpath, const char * key).

Now, simply reverting the change and removing -I$(srcdir) -I. from REQ_INCLUDES might lead embedded hunspell to fail to build  in VPATH setups (though I haven't tested, I guess this would happen because of the #include <hunspell.hxx>), so maybe -I$(srcdir) should only be added if srcdir is not ".".

What do you think?

Comment 1

[Mike Hommey [:glandium]]

(In reply to comment #0)
> Now, simply reverting the change and removing -I$(srcdir) -I. from REQ_INCLUDES
> might lead embedded hunspell to fail to build  in VPATH setups (though I
> haven't tested, I guess this would happen because of the #include
> <hunspell.hxx>), so maybe -I$(srcdir) should only be added if srcdir is not
> ".".

This wouldn't work either, actually... Reordering includes would be the only solution, I guess...

Comment 2

[Reed Loden [:reed]]

Benjamin, does the config/config.mk change in bug 412320 need to be backed out since that bug was WONTFIX'd and backed out?

Comment 3

[Benjamin Smedberg]

No, I left that change in on purpose and want it to remain. The "correct" solution to this problem is to separate the hunspell library from the XPCOM glue:

extensions/spellcheck/hunspell
extensions/spellcheck/build

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

I've created a patch doing it (almost) this way.
I've seperated the hunspell sources to
extensions/spellcheck/hunspell/hunspell and left the XPCOM glue in spellcheck/hunspell/src.
I'm attaching the patch which looks quite large because of the actual file moves.
It works correctly in initial testing.
Not sure if it's quite ready so not requesting review explicitely but anyway I'm interested in feedback.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch containing Makefile changes

my original patch is too big to attach so I splitted into actual changes and a patch containing the file moves.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

Attachment: compressed patch containing file moves

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Wolfgang, if the other patch is just file moves, it's probably easier to attach the commands you used to generate the moves for easier reviewing.

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

in extensions/spellcheck/hunspell:
mkdir hunspell
cd src
mv \
affentry.cpp \
affentry.hxx \
affixmgr.cpp \
affixmgr.hxx \
atypes.hxx \
baseaffix.hxx \
csutil.cpp \
csutil.hxx \
hashmgr.cpp \
hashmgr.hxx \
htypes.hxx \
hunspell.cpp \
hunspell.h \
hunspell.hxx \
langnum.hxx \
phonet.cpp \
phonet.hxx \
README.hunspell \
suggestmgr.cpp \
suggestmgr.hxx \
../hunspell

Comment 9

[Raúl Porcel]

I use this patch:
http://sources.gentoo.org/viewcvs.py/gentoo/src/patchsets/mozilla-firefox/3.0_rc1/100-system-hunspell-corrections.patch?view=markup

Is not mine, though :)

Comment 10

[Mike Hommey [:glandium]]

This is not needed anymore, as a side effect of the internal hunspell now being version 1.2+. What should be the resolution status ? WFM ?

Comment 11

[Reed Loden [:reed]]

Yeah, WFM is fine.