692743 - include hyphenation dictionaries in omnijar

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jonathan Kew [:jfkthame]]

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]]

Attachment: 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)]

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]]

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]]

To clarify, you absolutely want to also update removed-files.in. Place the entries in the MOZ_OMNIJAR section.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: 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]

(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]]

(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]]

(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]]

Attachment: v2.1, include hyphenation files in omnijar, and remove the separate files

Restored the required package-manifest.in entries.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 570044 [details] [diff] [review]
v2.1, include hyphenation files in omnijar, and remove the separate files

Looks good.

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/782f5b321370

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/782f5b321370