1175555 - Add a build flag to exclude hyphenation dictionaries from Android builds

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Richard Newman [:rnewman]]

Just like Bug 1063868, but for hyphenation dictionaries. See Bug 1095719 for assessment.

Comment 1

[Nick Alexander :nalexander [he/him]]

Be aware of the aborted work in Bug 1128033, which tried to move the hyphenation dictionary definitions to moz.build.  (It failed due to FINAL_TARGET shenanigans.)

In theory, this ticket is as easy as adding a build flag for excluding (or restricting) the set of hyphenation dictionaries and then using it here: https://dxr.mozilla.org/mozilla-central/source/intl/locales/Makefile.in#7.

In practice, I haven't a clue how this will interact with l10n repacks.  Given that one of the motivations for this ticket is to support smaller APKs for Indic locales, that worries me.

Comment 2

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=117511b0e9ed

Comment 3

[Richard Newman [:rnewman]]

Jon, don't forget to attach patches to bugs; Try pushes disappear eventually.

Comment 4

[Jonathan Almeida [:jonalmeida]]

Attachment: bug_257055_wip.patch

Sounds fair. I'm just making minor changes first to see how broken try becomes. Here's my patch so far: It just uses the MOZ_ANDROID_RESOURCE_CONSTRAINED flag, but I'm looking at other flags in m/a/confvars.sh to see if they're more suitable.

Comment 5

[Jonathan Almeida [:jonalmeida]]

So the tests that fail are because of the missing hyphenation files (which is good) but I haven't figured out how to fix the failing mochitests that test the hyphenation without removing them entirely for now until bug 1095719 is fixed.

In which case, should I leave my patch for review and only land it once I can land the other bug, which would also re-enable the tests?

Comment 6

[Richard Newman [:rnewman]]

I think it's fine to have a patch that causes tests to fail if you disable the feature, so long as the feature remains enabled! This is exactly the situation we're in for fonts and reftests after Bug 1063868.

We will need to solve how to deliver these assets at test time for both of those cases. These are the first steps on a long road.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8647616 [details] [diff] [review]
bug_257055_wip.patch

Instead of using MOZ_ANDROID_RESOURCE_CONSTRAINED we could introduce a new constant like we did for fonts (MOZ_ANDROID_EXCLUDE_FONTS). For fonts this is always disabled for now. This would allow us to ship this flag now and turn it on as soon as we have a solution for downloading the assets.

Comment 8

[Jonathan Almeida [:jonalmeida]]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Comment on attachment 8647616 [details] [diff] [review]
> bug_257055_wip.patch
> 
> Instead of using MOZ_ANDROID_RESOURCE_CONSTRAINED we could introduce a new
> constant like we did for fonts (MOZ_ANDROID_EXCLUDE_FONTS). For fonts this
> is always disabled for now. This would allow us to ship this flag now and
> turn it on as soon as we have a solution for downloading the assets.

This is a good idea; I'll do this today. Although, I thought MOZ_ANDROID_RESOURCE_CONSTRAINED is a catch-all flag for all resources that can be used to remove all resources we don't want to ship and not just fonts and hyphenations. Maybe consider using it again later?

Comment 9

[Jonathan Almeida [:jonalmeida]]

Haven't finished the addon because of my schedule. NI myself so I don't forget.

Comment 10

[Jonathan Almeida [:jonalmeida]]

Commented on the wrong bug.. but I'm doing this as well.

Comment 11

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c6ca275fc2

Comment 12

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e6a54d7fd1d

Comment 13

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Comment 14

[Jonathan Almeida [:jonalmeida]]

This is an old patch that I finally got around to testing if it works.

Comment 15

[Jonathan Almeida [:jonalmeida]]

Attachment: enable_hyphen_flag.patch

For enabling the flag.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

https://reviewboard.mozilla.org/r/22477/#review20035

I think the approach is fine, but there's nothing Android specific about this.  Also, this doesn't exclude the hyphenation code or processing; it's just the details, so perhaps MOZ_EXCLUDE_HYPHENATION_DICTIONARIES?

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #16)
> Comment on attachment 8675877 [details]
> MozReview Request: Bug 1175555 - Build flag to exclude hyphenation
> dictionaries from Android builds r?nalexander
> 
> https://reviewboard.mozilla.org/r/22477/#review20035
> 
> I think the approach is fine, but there's nothing Android specific about
> this.  Also, this doesn't exclude the hyphenation code or processing; it's
> just the details, so perhaps MOZ_EXCLUDE_HYPHENATION_DICTIONARIES?

Gah!  s/details/data files/.

Comment 18

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f9ad77be293

Comment 19

[Jonathan Almeida [:jonalmeida]]

Attachment: enable_hyphen_flag_new.patch

Updated patch for enabling flag.

Comment 20

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

https://reviewboard.mozilla.org/r/22477/#review20417

nits.  Looks good!

::: configure.in:4865
(Diff revision 2)
> +dnl = Whether to exclude hyphenations files in the final APK.

Generalize comment.

::: intl/locales/Makefile.in:8
(Diff revision 2)
> -PATTERN_FILES = $(strip $(wildcard $(srcdir)/*/hyphenation/*.dic))
> +    PATTERN_FILES = $(strip $(wildcard $(srcdir)/*/hyphenation/*.dic))

nit: don't indent, since it's not Makefile.in convention, and you haven't done so elsewhere.

::: intl/locales/Makefile.in:14
(Diff revision 2)
> +endif

nit:
```
endif # MOZ_EXCLUDE_HYPHENATION_DICTIONARIES
```

::: mobile/android/base/AppConstants.java.in:157
(Diff revision 2)
> +     * Whether this APK was built with hyphenations files included

nit: full sentence.

Comment 22

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/22477/#review20417

Fixed nits.

Comment 23

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8a3d24ccd492

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a3d24ccd492