Upgrade to Hunspell 1.5.4

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

3 years ago
Changes since v1.4.1:

2016-11-18 Dimitrij Mijoski <dmjpp at hotm>:
 * Version 1.5
 * Lot of stability fixes
 * Fixed compilation errors on various systems (Windows, FreeBSD)
 * Small performance improvement compared to 1.4.0
 * API is same as 1.4.

2016-04-29 Caolán McNamara <caolanm at LibO>:
 * deprecate old api and add new one
   old one remains implemented in terms of new one
   and will eventually be removed
 * shrink exposed api down to just hunspell.hxx
 * next major release is likely to require C++11
Assignee

Comment 2

3 years ago
Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

I've updated the version in README.mozilla to 1.5.0 locally, sorry for forgetting to do that before submitting.
Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

>diff --git a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>--- a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>+++ b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>@@ -195,17 +195,17 @@ NS_IMETHODIMP mozHunspell::SetDictionary
>   mDictionary = aDictionary;
>   mAffixFileName = affFileName;
> 
>   mHunspell = new Hunspell(affFileName.get(),
>                          dictFileName.get());
>   if (!mHunspell)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>-  nsDependentCString label(mHunspell->get_dic_encoding());
>+  nsCString label(mHunspell->get_dict_encoding().c_str());

Why don't you use nsAutoCString?

>-nsresult mozHunspell::ConvertCharset(const char16_t* aStr, char ** aDst)
>+nsresult mozHunspell::ConvertCharset(const char16_t* aStr, std::string* aDst)

nit: I'd like you to break the line after |nresult|.


I assume that the changes under extensions/spellcheck/hunspell/src comes from upstream. So, I don't check them, if I should do that, let me know.
Attachment #8812590 - Flags: review?(masayuki) → review+
Assignee

Comment 4

3 years ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> I assume that the changes under extensions/spellcheck/hunspell/src comes
> from upstream. So, I don't check them, if I should do that, let me know.

Correct. You're certainly welcome to if you want, though! :)
Assignee

Comment 5

3 years ago
Tyson, have you been fuzzing Hunspell lately?
Flags: needinfo?(twsmith)
Assignee

Comment 6

3 years ago
This addresses your review comments. Still green on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7248e0dcc8157529e36e66a4a77816451efa9c6&group_state=expanded

While changing the nsCString to nsAutoCString, I noticed a couple instances in mozHunspell.cpp as well. Is there any point in changing those while I'm at it? Looks OK on Try as well.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79569a89fdb7f7257dd34382fed2bc43bf2ff98

Thanks for the quick review!
Attachment #8812590 - Attachment is obsolete: true
Flags: needinfo?(masayuki)
Attachment #8812650 - Flags: review+
The reason why I pointed it is, you changed from nsDependentCString to nsCString. So, up to you for other lines because you don't need to touch them for fixing this bug. Perhaps, we should fix it in another bug if there are a lot of points which should be replaced with nsAuto*String.
Flags: needinfo?(masayuki)

Comment 8

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c7efa29d66
Upgrade to Hunspell 1.5.0. r=masayuki

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25c7efa29d66
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Tyson, have you been fuzzing Hunspell lately?

Not since August. I can likely add it to the continuous fuzzing system this quarter but I want to make sure we are targeting the correct APIs.
Flags: needinfo?(twsmith)
Assignee

Updated

3 years ago
Depends on: 1320746
Assignee

Comment 11

3 years ago
I decided to back this out for causing bug 1320746 and also because it causes some API compatibility issues for systems that use the system Hunspell. Those have been resolved upstream, so I'll try to land the future 1.5.x release that includes the Hungarian fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9444e271b391686b1238a20c20175b8bd7d7b400
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---

Comment 12

3 years ago
backoutbugherder
backout also from central
https://hg.mozilla.org/mozilla-central/rev/9444e271b391

Comment 13

3 years ago
What is the status  for updating to Hunspell version 1.5.4?
Assignee

Comment 14

3 years ago
Haven't had time to work on it yet. Will try as soon as I can.
Assignee

Comment 15

3 years ago
2016-11-30 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.4
	* Fixes the command COMPOUNDSYLLABLE used in Hungarian dictionary.

2016-11-28 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.3
	* Removed a #include from hunspell.hxx that was creating trouble

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.2
	* Reverted full backward compatibility with 1.4 public API, again

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.1
	* Reverted full backward compatibility with 1.4 public API

2016-11-26 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5
	* Fixed the changelog notice bellow about the API. I had a mistake.
Summary: Upgrade to Hunspell 1.5.0 → Upgrade to Hunspell 1.5.4
Assignee

Comment 16

3 years ago
Looks good on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e

avada, can you please test a build from this Try push to confirm that everything works as expected for you?
https://archive.mozilla.org/pub/firefox/try-builds/ryanvm@gmail.com-86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e/

Masayuki-san, did you want to take another look at this before I re-push it (assuming avada's testing goes well)? No changes on the Gecko side compared to the 1.5.0 patch you previously reviewed.
Attachment #8812650 - Attachment is obsolete: true
Flags: needinfo?(masayuki)
Attachment #8817444 - Flags: feedback?(dqeswn)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Masayuki-san, did you want to take another look at this before I re-push it
> (assuming avada's testing goes well)? No changes on the Gecko side compared
> to the 1.5.0 patch you previously reviewed.

No. Up to you.
Flags: needinfo?(masayuki)

Comment 18

3 years ago
Comment on attachment 8817444 [details] [diff] [review]
Update Hunspell to version 1.5.4

Review of attachment 8817444 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. No breakages in spellcheck with the hungarian dictionary.
Attachment #8817444 - Flags: feedback?(dqeswn) → feedback+

Comment 19

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33044cd4c5db
Upgrade to Hunspell 1.5.4. r=masayuki

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33044cd4c5db
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Updated

3 years ago
Blocks: 1326277
Assignee

Updated

3 years ago
Blocks: 691240
This requires a version check to be added to pkg_check_modules in moz.configure, because this breaks building with system version 1.4.
Seconded, i had a build failure w/ 53.0b1 using --with-system-hunspell - our system version is 1.3.2... (yeah ancient i know)
You need to log in before you can comment on or make changes to this bug.