Closed
Bug 1175555
Opened 9 years ago
Closed 9 years ago
Add a build flag to exclude hyphenation dictionaries from Android builds
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(firefox41 affected, firefox44 fixed)
RESOLVED
FIXED
mozilla44
People
(Reporter: rnewman, Assigned: jonalmeida)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/x-review-board-request
|
jonalmeida
:
review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review |
Just like Bug 1063868, but for hyphenation dictionaries. See Bug 1095719 for assessment.
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
Reporter | ||
Comment 3•9 years ago
|
||
Jon, don't forget to attach patches to bugs; Try pushes disappear eventually.
Assignee: nobody → jalmeida
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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?
Flags: needinfo?(rnewman)
Reporter | ||
Comment 6•9 years ago
|
||
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.
Flags: needinfo?(rnewman)
Comment 7•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: needinfo?(jalmeida)
Assignee | ||
Comment 8•9 years ago
|
||
(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?
Flags: needinfo?(jalmeida)
Assignee | ||
Comment 9•9 years ago
|
||
Haven't finished the addon because of my schedule. NI myself so I don't forget.
Flags: needinfo?(jonalmeida942)
Assignee | ||
Comment 10•9 years ago
|
||
Commented on the wrong bug.. but I'm doing this as well.
Flags: needinfo?(jonalmeida942)
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander
Attachment #8675877 -
Flags: review?(nalexander)
Assignee | ||
Comment 14•9 years ago
|
||
This is an old patch that I finally got around to testing if it works.
Assignee | ||
Comment 15•9 years ago
|
||
For enabling the flag.
Attachment #8647616 -
Attachment is obsolete: true
Comment 16•9 years ago
|
||
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?
Attachment #8675877 -
Flags: review?(nalexander)
Comment 17•9 years ago
|
||
(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/.
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
Updated patch for enabling flag.
Attachment #8675879 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8675877 -
Flags: review?(nalexander)
Assignee | ||
Comment 20•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8675877 -
Flags: review?(nalexander) → review+
Comment 21•9 years ago
|
||
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.
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
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
Attachment #8675877 -
Flags: review+ → review?(nalexander)
Assignee | ||
Updated•9 years ago
|
Attachment #8675877 -
Flags: review?(nalexander) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 24•9 years ago
|
||
Keywords: checkin-needed
Comment 25•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 44 → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•