ensure langpacks across platforms are identical.

RESOLVED FIXED in Firefox 60

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: Callek, Assigned: kmoir)

Tracking

Trunk
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

For submission to AMO, as a WebExtension we don't have a process enacted to seperate submissions by OS.

:Pike, is this something you said that was easy to validate on your end?
Flags: needinfo?(l10n)
Product: Core → Firefox Build System
I can validate that they're not.

browser has stuff that's only on for windows,

https://dxr.mozilla.org/mozilla-central/source/browser/locales/jar.mn#30-32

and toolkit has stuff that's on for non-gtk, aka, also on mac,

https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/jar.mn#62-64

We'll need a build system person to see what we can do here w/out breaking repacks by having extra files around. Or we need to whitelist those files from being unreferenced in the regular build.
Flags: needinfo?(l10n)
n-i to kim to find someone to help triage c#1
Flags: needinfo?(kmoir)
I talked to Callek on vidyo and this is my understanding of the requirements for this request

This work is a dependency for a the work required for signing language packs via AMO for releases.

Currently, web extensions submitted to AMO for signing are not separated by OS. 
The plan is to submit language packs directly to AMO for signing as WebExtensions
Currently the language packs are platform specific.
This request is to make them identical across platforms so they can be signed on AMO
Also, you cannot submit a new WebExtension with the same version once one already exists
This work is looking to be done for the 60 release which Callek indicated that should be in the next six weeks.
(Although I look at the release calendar and it's not until the first week of May but perhaps this is to allow for testing of the release promotion process with these changes https://wiki.mozilla.org/RapidRelease/Calendar)

Callek does this capture the requirements?
Flags: needinfo?(kmoir) → needinfo?(bugspam.Callek)
(In reply to Kim Moir [:kmoir] ET from comment #3)
> Callek does this capture the requirements?

I think it does, some slight clarifications:

* This work is desired/required to flip the 'langpacks require signing' bit, which is a desired feature for the 60 Release. various teams (myself included) want this work completed earlier so that there is more wiggle room in the when for that feature flip and more confidence in the automation surrounding it. So while 60 release is the hard deadline, I'm hoping for "no later than EARLY_BETA_OR_EARLIER flag flip date".

* The idea of them being identical is merely for the .xpi's involved in language packs to be the same as produced on windows as produced on mac. Such that we don't have missing files in one vs the other. Its ok if they have jar.mn manifest entries that are platform specific.
Flags: needinfo?(bugspam.Callek)
(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Kim Moir [:kmoir] ET from comment #3)
> * The idea of them being identical is merely for the .xpi's involved in
> language packs to be the same as produced on windows as produced on mac.
> Such that we don't have missing files in one vs the other. Its ok if they
> have jar.mn manifest entries that are platform specific.

Err submit before I completed my thought. "Its ok if they have jar.mn manifest entries that are plaftorm specific, as long as the jar.mn's themselves are identical. [e.g. we can use the flags [1] of an entry to specify os, etc]

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Chrome_Registration#os
So I'm taking a stab at this bug since everyone else on my team has a full roster of work.  So if I run some try runs with language packs that meet this specification - do I have to sign them before they are installed?  If not, how to I install unsigned language packs?  I have done very little l10n work before so I'm not sure how to test, so help would be appreciated.
Flags: needinfo?(bugspam.Callek)
Language packs don't currently need to be signed to be installed, and they can be installed by navigating (in firefox) to a folder where they were produced and clicking on the .xpi and then accepting dialogs.

This bug is about making sure that the build system produces language packs whose file contents are identical on mac as on linux as on windows, so you don't even really need to install them to test that, just diffoscope or unzip + recursive diff, between platforms, from a nightly-try run should help get you data.
Flags: needinfo?(bugspam.Callek)
So what jobs/task are the xpi artifacts included in?  I ran this try run and then added some l10n jobs but they all seem to be dmgs or mar files
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9512a348004b81976588626570434c7765cd6cf
Flags: needinfo?(bugspam.Callek)
Jobs that are part of the `nightly-l10n` kind have artifacts at public/build/{locale}/target.langpack.xpi
Flags: needinfo?(bugspam.Callek)
I have langpacks with the additional files. I'm just doing some diffs and comparing them against existing nightly language packs.  As an aside, they won't install in my existing nightly browser error message is compatibility issues, so trying to figure out the issue there.
Assignee: nobody → kmoir
Posted patch localesbz.patch (obsolete) — Splinter Review
With this patch, the language pack .xpi files that are created have the extra files.  They don't seem to install in my nightly install with a "language pack could not be installed because they aren't compatible with nightly 61.0a1" (on mac)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4ef1c443e5fe16c79f5ca8268c6de8296f3b24
Posted patch localesbz.patch (obsolete) — Splinter Review
I was able to install them now.  The version in my earlier try push was old but I was able to install the language pack from my new push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54abddd6fa7a2bc284f81379e5e0b4551484d0ec


Callek mentioned bug 1416270 as an issue with double zipped windows files
Attachment #8961086 - Attachment is obsolete: true
Attachment #8961442 - Flags: review?(core-build-config-reviews)
mshal looked at this patch and ran another try run with tests which exposed some issues with failed tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b88d4cd26d1d281e53c64bab696db3fc6eaf67&selectedJob=170706590

He looked at the history of the tests and mantaroh originally added tests so the files are os specific but it doesn't look like he works on this area of the code anymore.

i.e. bc4 failed on linux64

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b88d4cd26d1d281e53c64bab696db3fc6eaf67&selectedJob=170706590

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js?q=browser_all_files_referenced.js&redirect_type=direct#61

He said that as a build peer he's not familiar with this code or the reasons for having os specific files included or excluded. Callek could you suggest someone from the l10n side of things that might be able to provide insight and code review?
Flags: needinfo?(bugspam.Callek)
Ahhh thats the test I thought was part of the build but is part of browser-chrome!

TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0

This is the piece that worried me the most, and the one that :Pike above wanted build system knowledge to help with.

Though skimming the test file, I think https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js?q=path%3Abrowser_all_files_referenced.js&redirect_type=single#57 is all we need to change to support this for these files.
Flags: needinfo?(mshal)
Flags: needinfo?(l10n)
Flags: needinfo?(kmoir)
Flags: needinfo?(bugspam.Callek)
Flags: needinfo?(kmoir)
See Also: → 1339424
Another try run to attempt to fix tests but it still has problems
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c782654aeb8dceeb5b219acffcfba2aa7cedd15

trying something new now
(In reply to Justin Wood (:Callek) from comment #14)
> Ahhh thats the test I thought was part of the build but is part of
> browser-chrome!
> 
> TEST-UNEXPECTED-FAIL |
> browser/base/content/test/static/browser_all_files_referenced.js | there
> should be no unreferenced files - Got 1, expected 0
> 
> This is the piece that worried me the most, and the one that :Pike above
> wanted build system knowledge to help with.
> 
> Though skimming the test file, I think
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> static/browser_all_files_referenced.js?q=path%3Abrowser_all_files_referenced.
> js&redirect_type=single#57 is all we need to change to support this for
> these files.

I think it's worth reaching out to the browser group, or whoever owns that test, to see what they recommend.
Flags: needinfo?(mshal)
Posted patch bug1441358.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e209ff010eef40547446ea006107a634782e421
Attachment #8961442 - Attachment is obsolete: true
Attachment #8961442 - Flags: review?(core-build-config-reviews)
Try run is green here with mochitest-bc tests which failed before.  I'm running the other tests as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc72d1b29da30871d6999e171dca956620f822c
Attachment #8963779 - Attachment is obsolete: true
Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Hi mantaroh

We are trying to make the language packs more generic and I understand that you added the original tests - could you provide guidance that I'm on the right path?
Attachment #8964620 - Flags: feedback?(mantaroh)
Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Review of attachment 8964620 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Kim Moir [:kmoir] ET from comment #19)
> Comment on attachment 8964620 [details] [diff] [review]
> bug1441358.patch
> 
> Hi mantaroh
> 
> We are trying to make the language packs more generic and I understand that
> you added the original tests - could you provide guidance that I'm on the
> right path?

I'm not sure about the uiDensity.properties and I have modified 'print dialog' not 'print page setup' when removing these XUL files.This comment is focused on the print page setup dialog.
Currently, Gecko has print page setup dialog XUL files if the platform is windows, So another platform (mac/GTK) doesn't have this XUL file.[1]  So this patch will make gecko be a little big. But I think that this approach is OK since this file(i.e. printPageSetup.dtd" is too small.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/browser/nsIPrintingPromptService.idl
Attachment #8964620 - Flags: feedback?(mantaroh) → feedback+
Flags: needinfo?(l10n)
Axel, can you suggest someone with an l10n background who can review this patch - I talked to the build peers and they are not that familiar with the language packs to provide comment
Flags: needinfo?(l10n)
Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Review of attachment 8964620 [details] [diff] [review]:
-----------------------------------------------------------------

I'll own r+ since I understand the relevant code though I'm not a build peer. :kmoir's comment says the build team wanted to hand off to someone more familiar with l10n.
Attachment #8964620 - Flags: review+
Flags: needinfo?(l10n)
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9589c8e1acbd
ensure langpacks across platforms are identical r=Callek
https://hg.mozilla.org/mozilla-central/rev/9589c8e1acbd
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Approval Request Comment
[Feature/Bug causing the regression]: Language Pack Signing
[User impact if declined]: Unable to automatically sign langpacks in 60, or at least have broken language packs on some platforms.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: For the feature Bug 1441359
[Is the change risky?]: Minimal but not zero
[Why is the change risky/not risky?]: The files touched are used in product and results in a few small files added to the shipped build on all platforms, not just for langpacks
[String changes made/needed]: None
Attachment #8964620 - Flags: approval-mozilla-beta?
Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Fix needed for langpack signing. Approved for 60.0b11.
Attachment #8964620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.