Closed Bug 1460600 Opened 6 years ago Closed 6 years ago

Remove --enable-system-hunspell, since it's no longer supported

Categories

(Core :: Spelling checker, defect)

Product:
Core
Component:
Spelling checker
Version:
61 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: andrewSC, Assigned: kmag)

References

Details

Attachments

(1 file)

Bug 1460600: Remove unsupported --enable-system-hunspell flag.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180503152818

Steps to reproduce:

1) Install hunspell and the language specific dictionary (hunspell-en for me).
2) Compile firefox dev edition from the 61.0b3 release tag with "ac_add_options --enable-system-hunspell" in .mozconfig.
3) After compilation and installation, symlink the hunspell directory to /usr/lib/firefox-developer-edition/dictionaries.
4) Confirm permissions are 644 for .dic and .aff files within the dictionaries folder.
5) Run the browser binary from the terminal.
6) Navigate to any site with a multi line text field or use "data:text/html, <html contenteditable>" (without quotes) as an address.
7) Type words correctly and incorrectly spelled.



Actual results:

Observe that correct words are mistakenly flagged as incorrectly spelled. Checking the terminal, notice errors related to being unable to open the .aff and .dic files.

Running `strace -fv /usr/lib/firefox-developer-edition/firefox > debug.out 2>&1` and repeating the actions above shows the open syscall returning ENOENT. Here's a snippet from strace: https://gist.github.com/andrewSC/d3e78b562de346d28f1ce96f29447dbe



Expected results:

The system dictionary should have been opened by the browser and available for use.
The official builds are not affected, so this might be predicated on the --enable-system-hunspell.
I think this is fallout from "Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs."

Maybe the --enable-system-hunspell option should be removed.
Blocks: 1410214
Agreed, we should probably remove that option.

Also, built-in dictionaries are being moved to omnijar, so this symlink hack will stop working soon, anyway.
I wonder if the removal of supporting files in directories has impact on how linux distros work, too.
(In reply to Axel Hecht [:Pike] from comment #4)
> I wonder if the removal of supporting files in directories has impact on how
> linux distros work, too.

We'll still support files in directories, just not from the Firefox app directory. They can still be loaded by setting the spellchecker.dictionary_path pref or DICPATH environment variable.
Status: UNCONFIRMED → NEW
Component: Untriaged → Spelling checker
Ever confirmed: true
Product: Firefox → Core
Summary: Cannot use system hunspell dictionary files → Remove --enable-system-hunspell, since it's no longer supported
Comment on attachment 8974772 [details]
Bug 1460600: Remove unsupported --enable-system-hunspell flag.

https://reviewboard.mozilla.org/r/243178/#review249074

I guess that's fair, although ideally, it would be even better if hunspell could be changed such that we can hook jar: reading and stuff without modifications, which would still allow to use a system hunspell.

::: extensions/spellcheck/hunspell/glue/moz.build:35
(Diff revision 1)
>       'RemoteSpellCheckEngineParent.h',
>  ]
>  
> -# This variable is referenced in configure.in.  Make sure to change that file
> -# too if you need to change this variable.
> +# This variable is referenced in mozilla-config.h.in.  Make sure to change
> +# that file too if you need to change this variable.
>  DEFINES['HUNSPELL_STATIC'] = True

Considering the FileMgr hack, why not do the same kind of thing for hunspell_fopen_hooks.h and hunspell_alloc_hooks.h, removing the need for the mozilla-config.h hack in the first place?
Attachment #8974772 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #7)
> I guess that's fair, although ideally, it would be even better if hunspell
> could be changed such that we can hook jar: reading and stuff without
> modifications, which would still allow to use a system hunspell.

I've been thinking about this, and I still haven't come up with a good way to upstream this.

Just having Hunspell support reading from zip files wouldn't really be enough, since we want to be able to use the jar cache, and particularly the omnijar prefetching.

We might be able to add a hook to replace or augment the FileMgr class, or to pass iterators rather than file paths when loading dictionaries. But I think we'd have to make that a C interface, unless we could guarantee that system Hunspell libraries are always compiled without RTTI. And the mapping might get a bit ugly.

But even leaving all of that aside, the more I think about it, the more I think it's better that we not use system Hunspell, even if it did support reading from jar: URIs. There are some other changes to our bundled Hunspell, too. The most significant is the replacement of the charset converters with stubs that call into our Rust charset converters. The biggest upshot of that, and the removal of the stock FileMgr and HunZip code, is that our bundled Hunspell is much smaller, and maps much less unshared memory in child processes, which is becoming more and more important.

It also means that we're more likely to run into compatibility problems when using system Hunspell. Which may be worth risking if there were some clear advantage to supporting it, but at this point I'm not sure there is.

> ::: extensions/spellcheck/hunspell/glue/moz.build:35
> (Diff revision 1)
> >       'RemoteSpellCheckEngineParent.h',
> >  ]
> >  
> > -# This variable is referenced in configure.in.  Make sure to change that file
> > -# too if you need to change this variable.
> > +# This variable is referenced in mozilla-config.h.in.  Make sure to change
> > +# that file too if you need to change this variable.
> >  DEFINES['HUNSPELL_STATIC'] = True
> 
> Considering the FileMgr hack, why not do the same kind of thing for
> hunspell_fopen_hooks.h and hunspell_alloc_hooks.h, removing the need for the
> mozilla-config.h hack in the first place?

I didn't like this either, and thought about doing something to get rid of it, but it seemed like a separate bug. I was leaning towards just adding a -include to CFLAGS for that directory.
Just to make it clear for distributors, we're now supposed to use the bundled hunspell code/library since we can't use the system lib anymore, but if we still set spellchecker.dictionary_path the system *dictionaries* will be used, right ?
Yes, we set spellchecker.dictionary_path = /usr/share/hunspell and Nightly loads the system dictionaries just fine.
(In reply to Landry Breuil (:gaston) from comment #9)
> Just to make it clear for distributors, we're now supposed to use the
> bundled hunspell code/library since we can't use the system lib anymore, but
> if we still set spellchecker.dictionary_path the system *dictionaries* will
> be used, right ?

Correct. Although I'd generally prefer that you didn't, since scanning for and loading unpacked dictionaries has a non-trivial performance cost.

I also won't guarantee that that will continue to work forever, though I have no immediate plans to remove it. But, in any case, we definitely won't remove support for that without discussion on dev-platform, so you'll have a chance to weigh in if the possibility comes up.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> (In reply to Landry Breuil (:gaston) from comment #9)
> > Just to make it clear for distributors, we're now supposed to use the
> > bundled hunspell code/library since we can't use the system lib anymore, but
> > if we still set spellchecker.dictionary_path the system *dictionaries* will
> > be used, right ?
> 
> Correct. Although I'd generally prefer that you didn't, since scanning for
> and loading unpacked dictionaries has a non-trivial performance cost.

Sure, mozilla makes its own decision to ship localized binaries, but on platforms with package management abilities, the dictionaries are usually shared across apps. And you don't want unix distros to start shipping localized binaries for all locales, that would just waste too much space. I get the performance argument, but we prefer letting the user install the locale/dictionaries he actually uses/needs...
We already allow users to install dictionaries and language packs that aren't present in the build, and Zibi is working on making that process automatic.
https://hg.mozilla.org/mozilla-central/rev/1becc594554c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → kmaglione+bmo
Attachment #8974772 - Flags: review?(ehsan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: