Closed
Bug 1128033
Opened 10 years ago
Closed 7 years ago
Declare intl/locales/Makefile.in hyphenation data in moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox38+ wontfix, firefox58 fixed)
RESOLVED
FIXED
mozilla58
People
(Reporter: nalexander, Assigned: nalexander)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
We have have a libs:: custom install rule, with a wildcard thrown in [1]. We could add a new HYPHENATION_FILES (like RESOURCE_FILES) in moz.build, or we could just move the rule to misc::, or...
https://dxr.mozilla.org/mozilla-central/source/intl/locales/Makefile.in#10
Comment 1•10 years ago
|
||
Like FINAL_TARGET_FILES, you mean ?
<https://dxr.mozilla.org/comm-central/source/mozilla/python/mozbuild/mozbuild/frontend/context.py#520>
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Joshua Cranmer [:jcranmer] from comment #1)
> Like FINAL_TARGET_FILES, you mean ?
> <https://dxr.mozilla.org/comm-central/source/mozilla/python/mozbuild/
> mozbuild/frontend/context.py#520>
I do! I was not aware that would work, but I see that package-manifest.in is the bit that pushes into hyphenation, not the moz.build target. Very good!
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
/r/3205 - Bug 1128033 - Install hyphenation files with FINAL_TARGET_FILES. r=gps
Pull down this commit:
hg pull review -r ae9a8f8498d9b70f3a9a103fb544ae49e6551719
Attachment #8557354 -
Flags: review?(gps)
Comment 5•10 years ago
|
||
https://reviewboard.mozilla.org/r/3205/#review2687
r-. FINAL_TARGET, l10n, and moz.build do not mix, sadly. I'm pretty sure this will break l10n repacks. If you can prove me wrong, you get r+.
Updated•10 years ago
|
Attachment #8557354 -
Flags: review?(gps) → review-
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5)
> https://reviewboard.mozilla.org/r/3205/#review2687
>
> r-. FINAL_TARGET, l10n, and moz.build do not mix, sadly. I'm pretty sure
> this will break l10n repacks. If you can prove me wrong, you get r+.
gps and I discussed this in person and concluded that FINAL_TARGET in moz.build should be fine. Let's find out!
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 9•10 years ago
|
||
How could I test if this exact bug is responsible for hyphenation not working in nightly 39 pl (20150224030228)?
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Stefan Plewako [:stef] from comment #9)
> How could I test if this exact bug is responsible for hyphenation not
> working in nightly 39 pl (20150224030228)?
It's not really up for debate. I inspected the logs at
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-20-03-02-02-mozilla-central-l10n/mozilla-central-linux-l10n-nightly-fr-bm91-build1-build92.txt.gz
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-21-03-02-08-mozilla-central-l10n/mozilla-central-linux-l10n-nightly-fr-bm70-build1-build261.txt.gz
and you can see that intl/locales is not installing hyphenation data. It's exactly as gps thought -- l10n packaging is super special. Backing out.
Comment 11•10 years ago
|
||
Something went wrong with backout (central and aurora) or it will be fixed in some other way?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8557354 [details]
MozReview Request: bz://1128033/nalexander
This actually got r+ from gps in person to land.
Approval Request Comment
[Feature/regressing bug #]: Bug 1135858.
[User impact if declined]: bad hyphenation in all locale repacks.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: very low.
[String/UUID change made/needed]: none.
Attachment #8557354 -
Flags: review-
Attachment #8557354 -
Flags: review+
Attachment #8557354 -
Flags: approval-mozilla-aurora?
Merge of the backout https://hg.mozilla.org/mozilla-central/rev/c7f0a0caee42
Comment 15•10 years ago
|
||
Comment on attachment 8557354 [details]
MozReview Request: bz://1128033/nalexander
yes, let's backout from 38 aurora - please adjust the status flag back to fixed when this is completed.
Attachment #8557354 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
tracking-firefox38:
--- → +
Comment 16•10 years ago
|
||
Target Milestone: mozilla38 → ---
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8557354 -
Attachment is obsolete: true
Attachment #8619261 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
Can't fix this till we can test l10n repacks on try: bug 848284.
Depends on: 848284
Comment 20•7 years ago
|
||
I think bug 1392256 removed this directory from l10n repack consideration entirely, and we also have gained the ability to run l10n repacks on try now anyway, so we should try this again!
Depends on: 1392256
Comment 21•7 years ago
|
||
Comment 22•7 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7edf1d9d2e4cc502ee54804a30e97499be394732
This should run nightly l10n repacks for Linux on this push.
I did another try push with just linux64 builds just for sanity's sake:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f8fb2e832edcec76cbdd8784f112017a95eee3
If these are all green I'm going to re-land this patch.
Comment 23•7 years ago
|
||
nalexander pointed out on IRC:
"be sure to verify the set of locales for that hyphenation dictionary change -- they may have changed."
Comment 24•7 years ago
|
||
Everything on try was green, although given comment 10 I guess this wouldn't have actually broken the repack step so much as just made the resulting build be missing some bits.
Comment 25•7 years ago
|
||
I inspected the repack logs for the try builds in comment 21 and 22.
The ones for 21 repacked the build with the changes, the ones for 22 repacked the latest nightly from ftp (w/out the changes in this bug in the build, but in the source dir)
Though, yes, l10n repacks shouldn't care about the hyphenation dicts at all anymore.
Comment 26•7 years ago
|
||
I verified that the list of locales with .dic files in intl/locales still matches the list of locales in the moz.build file in this patch.
I also grabbed an arbitrary repack build package from try:
https://queue.taskcluster.net/v1/task/dKo4MbnTQo2hJR6mzKCfRQ/runs/0/artifacts/public/build/af/target.tar.bz2
and verified that its omni.ja contains the full list of .dic files in the proper paths.
Comment 27•7 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #25)
> I inspected the repack logs for the try builds in comment 21 and 22.
>
> The ones for 21 repacked the build with the changes, the ones for 22
> repacked the latest nightly from ftp (w/out the changes in this bug in the
> build, but in the source dir)
Right, I think this is just the way our repack tasks work currently.
> Though, yes, l10n repacks shouldn't care about the hyphenation dicts at all
> anymore.
That was my belief, I just wanted to prove it. :)
Comment 28•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a05dbcd62416887e56fd4dc93862d14e5e3a04
Bug 1128033 - Install hyphenation files with FINAL_TARGET_FILES. r=gps
Comment 29•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 10 years ago → 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•