Closed Bug 1365440 Opened 7 years ago Closed 7 years ago

Generate webextension language packs

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 verified, firefox58 verified, firefox59 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified
firefox59 --- verified

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files)

Bug 1365440 - Generate langpacks as webextensions.
59 bytes, text/x-review-board-request
mshal
: review+
Details
language packs.jpg
58.76 KB, image/jpeg
Details 
Based on the conversation from bug 1365031 we want to shift to build language packs as special-type web extensions.
Most of the code seems to be here: http://searchfox.org/mozilla-central/source/toolkit/locales/l10n.mk#173-178

As far as I can decypher that, it does:

1) execute "make libs-pl"
2) echo "Making langpack"
3) execute "../../config/nsinstall -D ../../dist/linux-x86_64/xpi/"
4) execute "obj-x86_64-pc-linux-gnu/_virtualenv/bin/python -m mozbuild.action.preprocessor" which produces install.rdf
5) execute "obj-x86_64-pc-linux-gnu/_virtualenv/bin/python -m mozbuild.action.zip" to zip up the package with "install.rdf chrome browser chrome.manifest"

I'm not sure what step (3) is doing, and I assume that step (1) is placing the `crashreport-override.ini` and `firefox-l10n.js` files in the xpi-stage/langpack-$lang directory which should not be there.

If I understand correctly, the new model will:

1) execute "make libs-pl" but if possible without the files we don't need
2) echo "Making langpack"
3) produce the manifest.json which takes some values from defines.inc from the locale (langpack name, authors etc.)
4) walk through all %.manifest files in the langpack and takes all `locale` entries to place them into the manifest.json
5) remove all %.manifest files
6) zip the result

I separated out step (4) so that it's easier to remove it once we migrate away from ChromeRegistry.

:pike, can you review the plan and if you see it differently suggest yours?
 
I'd also like to understand where should I write the code for (3) and (4). Should it be another `mozbuild.action.langpack-manifest` and `mozbuild.action.langpack-manifest-chrome` actions or some other way?
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> 1) execute "make libs-pl" but if possible without the files we don't need

Can we stop relying on specific make targets?
Re make targets, we discussed the boundary conditions with Callek, and while we have builds on both tc and buildbot, we shouldn't change how automation interacts with the build system.

Which at a high level means that we need to keep building our l10n assets as part of an installers-% make target.

I guess we have a bit of freedom under the hood.

Clearing the needinfo on what the build should do, as long as we don't know what we're actually expected to create.
Flags: needinfo?(l10n)
I had a conversation with :ted and :nalexander on #build today and I think my plan is as follows:

1) Create "mach build langpack-LANG" or "mach langpack LANG" command
2) In it, call libs-% with `WEBEXTENSION_LANGPACK=1` to filter out the things we don't need in the langpack
3) Then call some `mozbuild.action.create_langpack_manifest` which will produce manifest.json using:
3.a) defines.inc from the langpack to get the authors, langpack name etc.
3.b) all *.manifest files in the xpi-stage dir to get the entries
3.c) ab-CD for the locale
4) Then remove all *.manifest files from the xpi-stage dir
5) Then zip it

The content of the webextension package is:

1) all new-style .ftl files in /localization/ directory
2) all old-style .properties/.dtd files in their respective directories
3) searchplugins directory
4) manifest.json similar to the one attached in this bug

Before I start trying to write the patch for it, I'd like to make sure that all stakeholders agree, so NI'in - :pike, :glandium and :ted. I would NI :callek as well, but he's not accepting NI's at the moment. I'll try to sync with him ASAP after he's back on 22nd.

If you guys don't like any of the choices, please, provide the alternative. I'm very new to the whole area and need guidance :)
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
AFAICT, Callek asked us to not change the hooks beetween automation and build for the time being.

I don't see how a mach command falls into those constraints.
Flags: needinfo?(l10n)
Is everyone ok then, if I just modify the current "make langpack-%" command for now until Callek is back?
This is just a POC, but it seems to produce the right kind of zip package. :)
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

Pike, can you look through the patch and let me know if it's the right direction?
Attachment #8870226 - Flags: feedback?(l10n)
Attachment #8870226 - Flags: feedback?(l10n)
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

https://reviewboard.mozilla.org/r/141688/#review149530

::: python/mozbuild/mozbuild/action/langpack_manifest.py:38
(Diff revision 3)
> +#        'MOZ_LANG_TITLE': 'Polski',
> +#        'MOZ_LANGPACK_CREATOR': 'Aviary.pl',
> +#        'MOZ_LANGPACK_CONTRIBUTORS': 'Marek Stepien, Marek Wawoczny'
> +#    }
> +###
> +def parse_defines(paths):

This should use mozbuild.preprocessor instead, or something in mozbuild.action.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:70
(Diff revision 3)
> +#    ')
> +#    s == 'Marek Wawoczny, Marek Stepien'
> +###
> +def convert_contributors(str):
> +    str = str.replace('<em:contributor>', '')
> +    tokens = str.split('</em:contributor>')

This looks like it'd be simpler to use a regular expression here

::: python/mozbuild/mozbuild/action/langpack_manifest.py:107
(Diff revision 3)
> +#            'locale': 'pl',
> +#            'path': 'chrome/pl/locale/pl/autoconfig/'
> +#        },
> +#    ]
> +###
> +def parse_manifest(path, base_path, chrome_entries):

Probably want to use mozpack.chrome.manifest instead.

::: python/mozbuild/mozbuild/action/zip.py:43
(Diff revision 3)
>              for p, f in finder.find(path):
> +                if args.X is None:
> +                    jarrer.add(p, f)
> +                else:
> +                    ext = os.path.splitext(p)[1]
> +                    if ext != args.X:

This should use mozpack.path.match, I think.
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

Hard to say if this is a f+ yet, there's a ton of details I have about the code, and the big picture is more one of addons and build to comment on.
Attachment #8870226 - Flags: feedback?(l10n)
I'm struggling to find an .inc parser in mozilla-central that I could use in this patch instead of my POC parser.

:mshal - catlee said that you may know?
Flags: needinfo?(mshal)
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

https://reviewboard.mozilla.org/r/141688/#review149530

> This should use mozbuild.preprocessor instead, or something in mozbuild.action.

I could not find a .inc file parser that would give me entries out of it.

> This looks like it'd be simpler to use a regular expression here

I couldn't find a good regexp that would simplify this logic. If you have an idea, please, share :)
Updated the patch to Pike's feedback leaving two items open for the reviewer.

:ted agreed to review it for me :)
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
oh, one thing I did not touch yet is how to facilitate the switch.

:aswan is working on consuming the new langpacks in bug 1365709 so I assume that soon Gecko will be able to handle them, but I also assume that we'll want to, at least for a while, be able to produce both kinds of langpacks so the new logic should get a new command name?

I'll leave it to the reviewer to suggest the approach.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> I'm struggling to find an .inc parser in mozilla-central that I could use in
> this patch instead of my POC parser.
> 
> :mshal - catlee said that you may know?

You may be able to use preprocessor.py, though it may be a bit hacky since you'd be going through its data structures. Something like:

from mozbuild.preprocessor import Preprocessor
pp = Preprocessor()
pp.do_include('./browser/locales/en-US/defines.inc')
print("MOZ_LANGPACK_CREATOR: %s" % pp.context['MOZ_LANGPACK_CREATOR'])

The context has a few items in it by default that you may need to ignore (FILE, LINE, etc)

Is it no longer possible to #include defines.inc in a template file? If you could then you can use the preprocessor directly and let it do the replacing.
Flags: needinfo?(mshal)
I updated the patch to use all the python packages that you suggested.
I also removed the commented out pieces from browser build system and moved the command to new make command "new-langpack-%" to prevent the new code from breaking the old code.

:ted is on PTO now and suggested :mshal who's on PTO as well till Tuesday. I'll try to lure :mshal to review this one when he's back :)
Attachment #8870226 - Flags: review?(ted) → review?(mshal)
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

https://reviewboard.mozilla.org/r/141688/#review161432

I don't think there is anything major here, but I'd like to re-review real quick after it's updated. I flagged a few things that flake8 caught, but there are a few other minor things that it catches as well if you're interested in fixing them.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:1
(Diff revision 7)
> +###

nit: You should stick an MPL license header up here.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:48
(Diff revision 7)
> +#        'MOZ_LANGPACK_CONTRIBUTORS': 'Marek Stepien, Marek Wawoczny'
> +#    }
> +###
> +def parse_defines(paths):
> +    pp = Preprocessor()
> +    for path in paths.split(','):

I think this would be simpler if paths was a list so we don't have to split a string here.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:111
(Diff revision 7)
> +#        },
> +#    ]
> +###
> +def parse_chrome_manifest(path, base_path, chrome_entries):
> +    for entry in parse_manifest(None, path):
> +        pass

Stray 'pass' line.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:118
(Diff revision 7)
> +            parse_chrome_manifest(
> +                os.path.join(os.path.dirname(path), entry.relpath),
> +                base_path,
> +                chrome_entries
> +            )
> +            pass

Stray 'pass' again.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:145
(Diff revision 7)
> +                'type': 'resource',
> +                'alias': entry.name,
> +                'path': entry.target
> +            })
> +        else:
> +            raise Error('Unknown type %s' % entry[0])

(found by flake8): Error -> Exception

::: python/mozbuild/mozbuild/action/langpack_manifest.py:207
(Diff revision 7)
> +        'version': appver,
> +        'languages': locales,
> +        'author': '%s (contributors: %s)' % (defines['MOZ_LANGPACK_CREATOR'], contributors),
> +        'chrome_entries': [
> +        ]
> +    };

(found by flake8): extraneous semicolon

::: python/mozbuild/mozbuild/action/langpack_manifest.py:231
(Diff revision 7)
> +                entry['type'],
> +                entry['alias'],
> +                entry['path']
> +            )
> +        else:
> +            raise Error('Unknown type %s' % entry['type'])

(found by flake8): Error -> Exception

::: python/mozbuild/mozbuild/action/langpack_manifest.py:240
(Diff revision 7)
> +def main(args):
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('--locales', help='List of language codes provided by the langpack')
> +    parser.add_argument('--appid', help='ID of the application the langpack is for')
> +    parser.add_argument('--appver', help='Version of the application the langpack is for')
> +    parser.add_argument('--defines', help='List of defines files to load data from')

This should be made an actual list rather than a comma-separated string. (default=[], nargs='+' probably).

::: python/mozbuild/mozbuild/action/langpack_manifest.py:241
(Diff revision 7)
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('--locales', help='List of language codes provided by the langpack')
> +    parser.add_argument('--appid', help='ID of the application the langpack is for')
> +    parser.add_argument('--appver', help='Version of the application the langpack is for')
> +    parser.add_argument('--defines', help='List of defines files to load data from')
> +    parser.add_argument('input', help='Langpack directory.')

With --defines as a list, you might need to change this to --input so that the Makefile can do:

--defines $(NEW_APP_DEFINES) --input $(DIST)/xpi-stage/locale-$(AB_CD)

to separate the defines from the input file.

::: python/mozbuild/mozbuild/action/zip.py:27
(Diff revision 7)
>      parser.add_argument("-C", metavar='DIR', default=".",
>                          help="Change to given directory before considering "
>                          "other paths")
>      parser.add_argument("--strip", action='store_true',
>                          help="Strip executables")
> +    parser.add_argument("-X", metavar='EXCLUDE', default=None,

Can we make this '-x' instead of '-X' so it matches the zip(1) command?

::: python/mozbuild/mozbuild/action/zip.py:27
(Diff revision 7)
>      parser.add_argument("-C", metavar='DIR', default=".",
>                          help="Change to given directory before considering "
>                          "other paths")
>      parser.add_argument("--strip", action='store_true',
>                          help="Strip executables")
> +    parser.add_argument("-X", metavar='EXCLUDE', default=None,

I think this should be a default=[] and nargs="+" option, so that we can just do '-x "**/*.manifest" "**/*.js" ...' in the Makefile instead of splitting on spaces manually.

::: python/mozbuild/mozbuild/action/zip.py:40
(Diff revision 7)
>  
>      with errors.accumulate():
>          finder = FileFinder(args.C, find_executables=args.strip)
>          for path in args.input:
>              for p, f in finder.find(path):
> +                if args.X is None:

With args.x defaulting to [], we can remove this if/else block and just do:

if not any([match(p, exclude) for exclude in args.x]):
   jarrer.add(p, f)

::: toolkit/locales/l10n.mk:161
(Diff revision 7)
>  	@$(MAKE) repackage-zip AB_CD=$* ZIP_IN='$(ZIP_IN)'
>  
>  APP_DEFINES = $(firstword $(wildcard $(LOCALE_SRCDIR)/defines.inc) \
>                            $(srcdir)/en-US/defines.inc)
> +
> +NEW_APP_DEFINES = $(TK_DEFINES),$(firstword $(wildcard $(LOCALE_SRCDIR)/defines.inc) \

Eventually APP_DEFINES will be removed, right? Otherwise we should use $(TK_DEFINES),$(APP_DEFINES) here I think.
Attachment #8870226 - Flags: review?(mshal)
Comment on attachment 8870226 [details]
Bug 1365440 - Generate langpacks as webextensions.

https://reviewboard.mozilla.org/r/141688/#review166974

Looks good! Thanks for making all those updates.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:41
(Diff revision 10)
> +
> +###
> +# Parses multiple defines files into a single key-value pair object.
> +#
> +# Args:
> +#    paths (str) - a comma separated list of paths to defines files

I guess now this is 'paths (list)' instead of (str)?
Attachment #8870226 - Flags: review?(mshal) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bb27a8d3618
Generate langpacks as webextensions. r=mshal
https://hg.mozilla.org/mozilla-central/rev/7bb27a8d3618
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1390461
Attached image language packs.jpg — 
This feature is verified as fixed on Firefox 57.0 (20171112125346), Firefox 58.0b3 (20171113130450) and 59.0a1 (20171113220112) under Wind 10 64-bit and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: --- → verified
status-firefox59: --- → verified
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: