Last Comment Bug 692743 - include hyphenation dictionaries in omnijar
: include hyphenation dictionaries in omnijar
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 698451 701428
  Show dependency treegraph
 
Reported: 2011-10-07 02:43 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-01-03 03:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, include hyphenation files in omnijar (650 bytes, patch)
2011-10-07 02:46 PDT, Jonathan Kew (:jfkthame)
mh+mozilla: review+
Details | Diff | Splinter Review
v2, include hyphenation files in omnijar, and remove the separate files (3.54 KB, patch)
2011-10-27 08:25 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
v2.1, include hyphenation files in omnijar, and remove the separate files (2.32 KB, patch)
2011-10-27 11:38 PDT, Jonathan Kew (:jfkthame)
mwu.code: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-10-07 02:43:32 PDT
Now that bug 655337 has landed, we no longer need to provide the bundled hyphenation dictionaries as separate files on disk in order for Firefox to find and use them; they can be loaded directly from omnijar. This will reduce our installed footprint, which is a good thing.
Comment 1 Jonathan Kew (:jfkthame) 2011-10-07 02:46:22 PDT
Created attachment 565472 [details] [diff] [review]
patch, include hyphenation files in omnijar

AFAICS, this is trivial - we can add the hyphenation directory to OMNIJAR_FILES and it should Just Work.
Comment 2 Mark Banner (:standard8, afk until Dec) 2011-10-07 04:03:16 PDT
You might want to update package-manifest.in and removed-files.in as well:

http://mxr.mozilla.org/comm-central/search?string=hyphenation&find=installer&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Comment 3 Mike Hommey [:glandium] 2011-10-27 05:56:58 PDT
Comment on attachment 565472 [details] [diff] [review]
patch, include hyphenation files in omnijar

Sorry I didn't come to this earlier.
Comment 4 Michael Wu [:mwu] 2011-10-27 08:04:33 PDT
To clarify, you absolutely want to also update removed-files.in. Place the entries in the MOZ_OMNIJAR section.
Comment 5 Jonathan Kew (:jfkthame) 2011-10-27 08:25:22 PDT
Created attachment 569981 [details] [diff] [review]
v2, include hyphenation files in omnijar, and remove the separate files

(In reply to Michael Wu [:mwu] from comment #4)
> To clarify, you absolutely want to also update removed-files.in. Place the
> entries in the MOZ_OMNIJAR section.

So like this, yes? Also removed from package-manifest.in as per comment #2.
Comment 6 Dave Garrett 2011-10-27 09:27:30 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Created attachment 569981 [details] [diff] [review] [diff] [details] [review]
> v2, include hyphenation files in omnijar, and remove the separate files

I think you also need to add a line for just "hyphenation/" to removed-files.in to get rid of the then empty directory.
Comment 7 Michael Wu [:mwu] 2011-10-27 10:42:44 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Created attachment 569981 [details] [diff] [review] [diff] [details] [review]
> v2, include hyphenation files in omnijar, and remove the separate files
> 
> (In reply to Michael Wu [:mwu] from comment #4)
> > To clarify, you absolutely want to also update removed-files.in. Place the
> > entries in the MOZ_OMNIJAR section.
> 
> So like this, yes? Also removed from package-manifest.in as per comment #2.

package-manifest.in shouldn't be updated AFAIK. Without it in the package-manifest, the hyphenation files won't get copied to the staging directory, and the omnijar code only takes files out of the staging directory.

(In reply to Dave Garrett from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > Created attachment 569981 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > v2, include hyphenation files in omnijar, and remove the separate files
> 
> I think you also need to add a line for just "hyphenation/" to
> removed-files.in to get rid of the then empty directory.

Yup.
Comment 8 Jonathan Kew (:jfkthame) 2011-10-27 11:12:12 PDT
(In reply to Michael Wu [:mwu] from comment #7)
> package-manifest.in shouldn't be updated AFAIK. Without it in the
> package-manifest, the hyphenation files won't get copied to the staging
> directory, and the omnijar code only takes files out of the staging
> directory.

Right - I had just pushed it to tryserver, which confirmed this by failing the hyphenation tests. Hence dropping that from the patch.
Comment 9 Jonathan Kew (:jfkthame) 2011-10-27 11:38:14 PDT
Created attachment 570044 [details] [diff] [review]
v2.1, include hyphenation files in omnijar, and remove the separate files

Restored the required package-manifest.in entries.
Comment 10 Michael Wu [:mwu] 2011-10-27 11:42:28 PDT
Comment on attachment 570044 [details] [diff] [review]
v2.1, include hyphenation files in omnijar, and remove the separate files

Looks good.
Comment 12 Ed Morley [:emorley] 2011-10-28 04:34:29 PDT
https://hg.mozilla.org/mozilla-central/rev/782f5b321370

Note You need to log in before you can comment on or make changes to this bug.