1411951 - Windows langpacks are generated with invalid paths

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[marius.santa]

Attachment: Temp.zip

[Affected versions]:
Firefox 58.0a1 (20171025230440)

[Affected platforms]:
Windows 10 64-bit
MacOS Sierra 10.13
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Install https://addons-dev.allizom.org/en-US/firefox/addon/pl-webext-lang-58-0/
3.Go to "about:config".
4.Set "intl.locale.matchOS = False" and "general.useragent.locale = pl".
5.Restart the browser.


[Expected Results]:
The browser restart with the language changed to polish.

[Actual Results]:
An error window is displayed.


[Regression Range]:
Last good revision: 7cc3f9fd01acb701fec4b872634513c8c59f5f1a
First bad revision: 63618add0894f613af2a5b7939a60c2251443ddf
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7cc3f9fd01acb701fec4b872634513c8c59f5f1a&tochange=63618add0894f613af2a5b7939a60c2251443ddf

[Additional notes]:
Gif presenting the issue is attached.
Error message text is attached.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

When you say "An error window is displayed" do you mean a yellow screen with a red error?

If that's the case, then it's an expected behavior. Basically, nightly code changes every day, and we do not produce nightly langpacks to match those daily changes.

The result is that installing a nightly langpack on a nightly build almost always will give you an outdated localization which in turn triggers the yellow screen.

We're addressing it with a transition to a new localization API that will make us fallback more gracefully.

Please, confirm that that's what you mean, and not what the title states (A crash).

Comment 2

[marius.santa]

Yes it is the yellow screen, and the issue you described as you can see from the attached gif. The problem is that from a users point of view the browser crashed with an error, and the only way to use it, is to create a new clean profile.

Considering I am testing Nightly, can you please provide some steps in order for me to be able to generate the appropriate language packs?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Considering I am testing Nightly, can you please provide some steps in order for me to be able to generate the appropriate language packs?

Sure, but please, be warned, that we do not currently support using nightly with langpacks :)

You need to build Firefox, and then do "./mach build langpack-$loc" where $loc is your locale code (de, pl, it etc.) - pick one of those codes we have l10n for : https://hg.mozilla.org/l10n-central/

The result is an xpi in objdir/dist/platform/xpi/langpack-*-58-*-$loc.xpi

Install this XPI and you'll be able to use a localized build until it updates to a newer nightly and your langpack breaks :(

As I said, there's a major effort to rework how our localization works that in the end will result in us dropping from a missing $loc string to fallback en-US string rather than showing yellow screen of death.

Until this point, we support langpacks only in beta and release. Sorry for that!

I'm going to close this bug as wontfix, although we are fixing it. It's just going to take a long time (at least half a year I expect) before we're there.

If you want to track the progress on the new localization API, see bug 1365426.

Comment 4

[Axel Hecht [:Pike]]

Marius, if that langpack on addons-dev is yours, could you please pull it off the site? Even on a dev instance, I wouldn't want people to find this and then fall into the same rabbit hole as you just did. Thanks.

Comment 5

[Kris Maglione [:kmag]]

We also apparently do nightly langpack builds these days:

https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/win64/xpi/

Comment 6

[marius.santa]

@Axel Hecht I removed the language packs from addons-dev.

@Kris Maglione We used one of the xpi created on the 27th with 58.0a1(20171026221945) witch is from the 26th and we got the same error window but with the message:
<window id="main-window"
^
Is this the same issue?

@Zibi Braniecki we can't make our own custom build it takes programs we don't generally use, lots of time and we get lot of errors, it's not feasible, is there any other way to do this?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

> @Zibi Braniecki we can't make our own custom build it takes programs we don't generally use, lots of time and we get lot of errors, it's not feasible, is there any other way to do this?

I don't know of any other way to get a language pack for nightly than building it.

If you want to test a language pack, you need to:

1) Build Firefox
2) Build language pack for this very version of language pack
3) Install it and test.

Alternatively you can wait for 58 to hit beta when you can test beta releases against beta langpacks without having to build anything manually.

Comment 8

[Kris Maglione [:kmag]]

Sorry, I didn't mean to clear the needinfo before.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> Alternatively you can wait for 58 to hit beta when you can test beta
> releases against beta langpacks without having to build anything manually.

We should really aim to get as much QA as possible done before this hits beta...

(In reply to marius.santa from comment #6)
> @Kris Maglione We used one of the xpi created on the 27th with
> 58.0a1(20171026221945) witch is from the 26th and we got the same error
> window but with the message:
> <window id="main-window"
> ^
> Is this the same issue?

No, this is a different issue. It looks like the paths in the manifest.json files for the windows langpacks are mangled, and have backslashes as path separators in some places:

        "onboarding": "browser\\features\\[e-mail]\\pl/locale/pl/", 

Zibi, can you look into this? This is probably a release blocker.

Comment 9

[Kris Maglione [:kmag]]

Note that the OS-X langpacks should work on Windows (I tested, and they do work for me on Linux, at least), so testing with those may be a good workaround until the Windows langpacks are fixed:

https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/mac/xpi/

(Also, AMO actually only signs and distributes Windows langpacks, on the basis that they should be the same for all platforms, and there isn't any install manifest data to distinguish them, so it shouldn't really matter which we test with. But the Windows builds really need to be fixed, if only for the sake of AMO.)

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

> We should really aim to get as much QA as possible done before this hits beta...

Ok, then I'd say what we need is:

1) Particular release of Firefox Nightly (if possible to the hg revision)
2) A langpack built off of that revision

Then we can test it.

> Zibi, can you look into this? This is probably a release blocker.

Yeah, will look into this!

But I'm wondering - what's the value of producing langpacks per platform, if all platforms share the same langpack? It seems to me that we could just take, say, linux langpacks and release them.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> But I'm wondering - what's the value of producing langpacks per platform, if
> all platforms share the same langpack? It seems to me that we could just
> take, say, linux langpacks and release them.

We could, but that doesn't justify building broken Windows langpacks ;)

I'm all for not building separate langpacks for each platform, but that's a separate bug, really.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

Review commit: https://reviewboard.mozilla.org/r/194334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194334/

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review199286

::: python/mozbuild/mozbuild/action/langpack_manifest.py:21
(Diff revision 1)
>  import json
>  import io
>  import datetime
>  import requests
>  import mozversioncontrol
> +import posixpath

I suggest to use mozpack.path instead. Does the same thing, but probably created for the purpose of "paths in our packages", https://firefox-source-docs.mozilla.org/python/mozpack.html#module-mozpack.path.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194334/diff/1-2/

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194334/diff/2-3/

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you Axel! I was looking for such function in the regular os.path module and was surprised not to find it. This is much better!

Comment 17

[marius.santa]

I can confirm that on Mac OS 10.13 and Ubuntu 16.04 32-bit using Nightly 58.0a1 (20171029220112) language pack activation works fine.

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review200504

::: python/mozbuild/mozbuild/action/langpack_manifest.py:21
(Diff revision 3)
>  import json
>  import io
>  import datetime
>  import requests
>  import mozversioncontrol
> +import mozpack.path

Everywhere else we do 'import mozpack.path as mozpath'. We have bug 1399430 to make that more sane, but in the meantime it probably makes sense to be consistent with existing scripts.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:271
(Diff revision 3)
>                  os.path.join(os.path.dirname(path), entry.relpath),
>                  base_path,
>                  chrome_entries
>              )
>          elif isinstance(entry, ManifestLocale):
> -            chrome_entries.append({
> +            entry_path = os.path.join(

I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

> I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()

I tried that, but that seemed to require me to move all os.path.join to mozpath.join including ones the recursive ones that are used to actually load files from the disk.
Otherwise I end up with weird "browser\\locales/browser/en-US/file.dtd"

It may be that our setup allows me to use posix paths to load files in python, but I'm not sure if that's better than just normalizing the path before injecting it into the manifest.

What's your preference?

Comment 20

[Michael Shal [:mshal]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> > I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()
> 
> I tried that, but that seemed to require me to move all os.path.join to
> mozpath.join including ones the recursive ones that are used to actually
> load files from the disk.
> Otherwise I end up with weird "browser\\locales/browser/en-US/file.dtd"
> 
> It may be that our setup allows me to use posix paths to load files in
> python, but I'm not sure if that's better than just normalizing the path
> before injecting it into the manifest.
> 
> What's your preference?

Ok, just use normpath as you had then and switch the import so it's mozpath.normpath()

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194334/diff/3-4/

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review201122

r+ with the function name fixed. As it is it would get an AttributeError.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:283
(Diff revision 4)
> +            chrome_entries.append({
> +                'type': 'locale',
> +                'alias': entry.name,
> +                'locale': entry.id,
> +                'platforms': convert_entry_flags_to_platform_codes(entry.flags),
> +                'path': mozpath.path.normsep(entry_path)

This should just be 'mozpath.normsep', not 'mozpath.path.normsep'.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194334/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4fb78381ea0
Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json. r=mshal

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a4fb78381ea0

Comment 26

[marius.santa]

Attachment: language packs windows.jpg

This issue is verified as fixed on Firefox 58.0a1 (20171106100122) under Wind 10 64-bit.

Post-fix screenshot attached.