Closed Bug 1128033 Opened 9 years ago Closed 7 years ago

Declare intl/locales/Makefile.in hyphenation data in moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox38+ wontfix, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox38 + wontfix
firefox58 --- fixed

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
(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!
/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)
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+.
Attachment #8557354 - Flags: review?(gps) → review-
(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
https://hg.mozilla.org/mozilla-central/rev/8eef92e67922
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
How could I test if this exact bug is responsible for hyphenation not working in nightly 39 pl (20150224030228)?
(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.
Something went wrong with backout (central and aurora) or it will be fixed in some other way?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
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+
Attachment #8557354 - Attachment is obsolete: true
Attachment #8619261 - Flags: review+
Blocks: 1135858
Can't fix this till we can test l10n repacks on try: bug 848284.
Depends on: 848284
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
Blocks: buildtup
(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.
nalexander pointed out on IRC:
"be sure to verify the set of locales for that hyphenation dictionary change -- they may have  changed."
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.
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.
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.
(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. :)
https://hg.mozilla.org/mozilla-central/rev/a7a05dbcd624
Status: REOPENED → RESOLVED
Closed: 9 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: