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)
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: andrewSC, Assigned: kmag)
References
Details
Attachments
(1 file)
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.
Comment 1•6 years ago
|
||
The official builds are not affected, so this might be predicated on the --enable-system-hunspell.
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
I wonder if the removal of supporting files in directories has impact on how linux distros work, too.
Assignee | ||
Comment 5•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
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 hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment 9•6 years ago
|
||
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 ?
Comment 10•6 years ago
|
||
Yes, we set spellchecker.dictionary_path = /usr/share/hunspell and Nightly loads the system dictionaries just fine.
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Comment 12•6 years ago
|
||
(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...
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1becc594554c1a273535ed175895e801667735e9 Bug 1460600: Remove unsupported --enable-system-hunspell flag. r=glandium
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1becc594554c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Assignee: nobody → kmaglione+bmo
Updated•6 years ago
|
Attachment #8974772 -
Flags: review?(ehsan)
You need to log in
before you can comment on or make changes to this bug.
Description
•