1441358 - ensure langpacks across platforms are identical.

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Justin Wood (:Callek)]

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?

Comment 1

[Axel Hecht [:Pike]]

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.

Comment 2

[Justin Wood (:Callek)]

n-i to kim to find someone to help triage c#1

Comment 3

[Kim Moir [:kmoir] ET]

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?

Comment 4

[Justin Wood (: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.

Comment 5

[Justin Wood (: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

Comment 6

[Kim Moir [:kmoir] ET]

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.

Comment 7

[Justin Wood (: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.

Comment 8

[Kim Moir [:kmoir] ET]

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

Comment 9

[Justin Wood (:Callek)]

Jobs that are part of the `nightly-l10n` kind have artifacts at public/build/{locale}/target.langpack.xpi

Comment 10

[Kim Moir [:kmoir] ET]

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.

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: localesbz.patch

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

Comment 12

[Kim Moir [:kmoir] ET]

Attachment: localesbz.patch

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

Comment 13

[Kim Moir [:kmoir] ET]

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?

Comment 14

[Justin Wood (: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.

Comment 15

[Kim Moir [:kmoir] ET]

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

Comment 16

[Michael Shal [:mshal]]

(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.

Comment 17

[Kim Moir [:kmoir] ET]

Attachment: bug1441358.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e209ff010eef40547446ea006107a634782e421

Comment 18

[Kim Moir [:kmoir] ET]

Attachment: bug1441358.patch

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

Comment 19

[Kim Moir [:kmoir] ET]

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?

Comment 20

[Mantaroh Yoshinaga[: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

Comment 21

[Kim Moir [:kmoir] ET]

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

Comment 22

[Justin Wood (:Callek)]

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9589c8e1acbd
ensure langpacks across platforms are identical r=Callek

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9589c8e1acbd

Comment 25

[Justin Wood (:Callek)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8964620 [details] [diff] [review]
bug1441358.patch

Fix needed for langpack signing. Approved for 60.0b11.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/305aefdbaf13