Closed Bug 1439742 Opened 6 years ago Closed 6 years ago

Use LOCALIZED_{GENERATED_}FILES in mobile/android

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

Right now, some of the hairiest l10n code is in mobile/android/base{/locales}/Makefile.in.  There's a lot not to like here, but this ticket proposes to move the subtle handling of chrome-$(AB_CD) into moz.build to the greatest extent possible.

What's happening here is that the build system is trying to get a localized resource into a directory whose path depends on $(AB_CD).  Due to how l10n builds and repacks work, the build system only learns about $(AB_CD) at build time and NOT at configure time.  That makes all of the straight-forward approaches impossible.

What happens right now is that l10n repacks invoke chrome-$(AB_CD) targets, and the m/a/base/locales/Makefile.in sniffs the appropriate targets from the $(AB_CD) passed through and re-invokes itself in order to define rules with the appropriate resource directory baked in.  It works, but it's hard to understand, and incredibly difficult to modify.

This ticket tracks growing capability in LOCALIZED_GENERATED_FILES to define the special directories, and using it.
I don't know whether to laugh or cry at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f35039ba992544d03063d6ee2f7586adf3f13201.

First, the red are just Python tests that I didn't update.  I'll get to them.

The N builds are looking good!  Everything is in its place for multi-locale builds.

It's the single locale repack builds like N17 that are funny.  The new code does what you'd think -- everything in res/{raw,values}-sl.  The old code puts sl (Slovenian) generated JSON into res/{raw,values} only.  I think that's intentional -- we're trying to force a Slovenian build, regardless of OS locale.  BUT WHY DOES IT DO THAT?  I can't explain it yet.

ted: still worth reviewing, since so much of this is a discussion and requires choices to be justified.
The final commit gives a lot of detail about what's going on, and with it the N and N17 builds at https://treeherder.mozilla.org/#/jobs?repo=try&revision=056ab10143255b0faf574b2dbbff6c318c34e4ae are looking very good!

The final commit is also much easier to reason about than the previously existing code -- look at the error I made, in fact, to see how hard it was to interpret.  (This is also hard to interpret, since the Make targets make _no sense_, but at least it's more clear that something is happening.)

I think I'll probably squash all three "meat" commits into one after review, in order to help bisectors.
Comment on attachment 8952534 [details]
Bug 1439742 - Pre: Remove unused MERGE_FILES and EN_US_OR_L10N_FILE{S}. .mielczarek

https://reviewboard.mozilla.org/r/221754/#review228774

Always nice to remove cruft! I feel like I might have noticed this while doing my `LOCALIZED_FILES` work but forgot to do anything about it.
Attachment #8952534 - Flags: review?(ted) → review+
Comment on attachment 8952535 [details]
Bug 1439742 - Pre: Lift AB_rCD to ambient Make environment. .mielczarek

https://reviewboard.mozilla.org/r/221756/#review228780

A couple of minor comments but otherwise this looks sensible.

::: config/AB_rCD.mk:8
(Diff revision 2)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Turn 'en-US' into '' and general 'ab-CD' into '-ab-rCD' -- note
> +# leading hyphen.  This is for use in Android resource directories, so
> +# that 'res{AB_rCD}' is 'res' for 'en-US' and 'res-en-rGB' for
> +# 'en-GB'.

This format was not familiar to me so I went and looked it up. I think an explanatory link in this comment would be useful, probably to: https://developer.android.com/guide/topics/resources/providing-resources.html#AlternativeResources

::: config/config.mk:342
(Diff revision 2)
>  # Because you might wish to "make locales AB_CD=ab-CD", we don't hardcode
>  # MOZ_UI_LOCALE directly, but use an intermediate variable that can be
>  # overridden by the command line. (Besides, AB_CD is prettier).
>  AB_CD = $(MOZ_UI_LOCALE)
> +
> +ifndef INCLUDED_AB_RCD_MK

You don't seem to set `INCLUDED_AB_RCD_MK` in AB_rCD.mk.

::: mobile/android/base/locales/Makefile.in:30
(Diff revision 2)
>    $(NULL)
>  
>  chrome-%:: AB_CD=$*
>  chrome-%::
>  	@$(MAKE) \
> -	  $(dir-res-values)-$(AB_rCD)/strings.xml \
> +	  ../res/values$(AB_rCD)/strings.xml \

Is there a reason you made *just* this change and not either:
1) leave the `$(dir...)` vars there or
2) remove the `$(dir..)` vars?
Attachment #8952535 - Flags: review?(ted) → review+
Comment on attachment 8952910 [details]
Bug 1439742 - Post: Fix tests. .mielczarek

https://reviewboard.mozilla.org/r/222176/#review228784
Attachment #8952910 - Flags: review?(ted) → review+
Comment on attachment 8952536 [details]
Bug 1439742 - Part 1: Allow {AB_CD} and {AB_rCD} in LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/221758/#review228772

I have a bunch of comments, and a few things that should be fixed before this lands, but overall this looks great.

Thanks so much for writing that short novel of a commit message. I felt like I spent as much time reviewing and commenting on it as I did on the rest of the patch, but only because once I read it and understood what you were doing reviewing the rest of the patch was very straightforward!

::: commit-message-35370:20
(Diff revision 2)
> +In addition, if we use manifests, then we lose the powerful locality
> +of |mach build mobile/android{/base}| re-generating changed
> +locale-dependent resources.  This is similar to how the build system
> +plumbs dist/idl manifest processing throughout the build: we're
> +repairing local workflows after moving work into a global process.
> +For these reasons, this doesn't support {AB_CD} in LOCALIZED_FILES.

This is a great explanation. Thanks for the context!

::: commit-message-35370:31
(Diff revision 2)
> +m/a/base/locales all the way out to
> +mobile/locales/.../region.properties, so the existing code doesn't
> +follow the layout expected between mozilla-central and
> +l10n-central/$(AB_CD).  But that'll impedance will get worse as we
> +improve the build system dependencies, not better, so we should grow
> +support for localized resources that aren't exactly as expected.

I can't say I *love* this but it's a sensible tradeoff for now, especially since you're getting rid of a bunch of other Makefile cruft.

::: commit-message-35370:38
(Diff revision 2)
> +- I chose to follow Python's syntax for string substitutions.  I
> +would have preferred to mark files that should be localized with a
> +leading '%'... but I took that for filesystem absolute paths in
> +moz.build files already.  I also considered @AB_CD@ to echo the
> +preprocessor, but didn't want to open the door to an expecation that
> +_all_ preprocessor DEFINEs will work in the way {AB_CD} does.

I like this. It's succinct and understandable, especially since moz.build files are already Python, and it doesn't confusingly overload a sigil that's already used in Makefiles or whatever.

::: commit-message-35370:48
(Diff revision 2)
> +--fallback flag.  The real reason this is needed is that we're doing
> +work which is more like the work of compare-locales (merging
> +locale-dependent resources) at build-time rather than repack time.  I
> +don't know why that's the case -- probably when we (I) implemented it,
> +compare-locales and the whole l10n process was entirely opaque.  It's
> +not worth changing it now, so we use this --fallback flag approach.

Is it worth filing a follow-up to improve this, or should it wait until we figure out how to remove the make backend from repacks?

::: commit-message-35370:54
(Diff revision 2)
> +
> +- I didn't get to tup support.  This should gently fail without
> +breaking tup builds: any {AB_CD} substitutions just won't be
> +expanded.  I haven't a clue how this should work in tup in the future
> +(or, more generally, how to make any sense of repacks without
> +declaring the full set of expected locales at configure time.)

I think this is OK for now, tup/fastermake don't actually support localized builds or repacks currently. We'll need to do some more work before that's possible.

::: mobile/android/base/locales/generate_suggestedsites.py:59
(Diff revision 2)
>          try:
>              properties.update(path)
> -        except IOError:
> -            # Ignore non-existing files
> +        except IOError as e:
> +            if e.errno == errno.ENOENT:
> +                # Ignore non-existant files.
> -            continue
> +                continue

I think this wants a `raise` now to re-raise non `ENOENT` errors.

::: python/mozbuild/mozbuild/backend/recursivemake.py:531
(Diff revision 2)
>                  tier = 'libs'
>              else:
>                  tier = 'misc'
>              self._no_skip[tier].add(backend_file.relobjdir)
> +
> +            # Localized generated files can use {AB_CD} and {AB_rCD} in their

Please update the docs to match this new feature:
https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#localized-generated-files

::: python/mozbuild/mozbuild/backend/recursivemake.py:539
(Diff revision 2)
> +                substs = {'AB_CD': '$(AB_CD)', 'AB_rCD': '$(AB_rCD)'}
> +            else:
> +                substs = {}
> +            substituted = []
> +
> +            # It would be nice to only write AB_rCD once per backend.mk, but it

Lucky for you someone already hit this same problem and provided a solution. :) You can just use `backend_file.write_once` and it'll do the right thing.

::: python/mozbuild/mozbuild/backend/recursivemake.py:550
(Diff revision 2)
> +                try:
> +                    substituted.append(o.format(**substs))
> +                except KeyError as e:
> +                    raise ValueError('%s not in %s is not a valid substitution in %s'
> +                                     % (e.args[0], ', '.join(sorted(substs.keys())), o))
> +            obj.outputs = tuple(substituted)

A little nitpicky, but I would prefer if you used a new variable here instead of reassigning `obj.outputs`.

::: python/mozbuild/mozbuild/test/backend/data/localized-files-AB_CD/moz.build:5
(Diff revision 2)
>  # -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
>  # Any copyright is dedicated to the Public Domain.
>  # http://creativecommons.org/publicdomain/zero/1.0/
>  
> -LOCALIZED_FILES += [
> +LOCALIZED_FILES.x['{AB_CD}'] += [

You added test data for both `localized-files-AB_CD` and `localized-generated-files-AB_CD` but it looks like you only wrote a test for the latter, unless I missed something. Is this testdata leftover from some earlier work?

::: python/mozbuild/mozbuild/test/frontend/data/localized-files-no-en-us/moz.build:8
(Diff revision 2)
>  # http://creativecommons.org/publicdomain/zero/1.0/
>  
>  LOCALIZED_FILES.foo += [
>      'en-US/bar.ini',
>      'foo.js',
> +    'inner/locales/en-US/bar.ini',

Thanks for making sure this gets tested!

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:1375
(Diff revision 2)
>              for f in files:
>                  self.assertTrue(unicode(f) in expected)
>  
>      def test_localized_files_no_en_us(self):
>          """Test that LOCALIZED_FILES errors if a path does not start with
> -        `en-US/`."""
> +        `en-US/` or contain `/locales/en-US/`."""

This is mentioned in the docs for `LOCALIZED_FILES`, so please update those to match your changes:
https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#localized-files
Attachment #8952536 - Flags: review?(ted) → review+
Comment on attachment 8952537 [details]
Bug 1439742 - Allow {AB_CD} and {AB_rCD} in LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/221760/#review228816

::: commit-message-9542b:5
(Diff revision 2)
> +Bug 1439742 - Part 2: Use LOCALIZED_GENERATED_FILES for strings.xml. r=ted.mielczarek
> +
> +This can't be a LOCALIZED_PP_FILES, since we need to customize the
> +output location based on AB_rCD, and since we need a little more
> +flexibility than PP_FILES gives for our inputs.

Considering that I only wound up using `LOCALIZED_PP_FILES` in one single place and it turns out to not be flexible enough to do most other things I wonder if we shouldn't just remove it and change that one place to use `LOCALIZED_GENERATED_FILES`?

::: mobile/android/base/locales/generate_strings_xml.py:25
(Diff revision 2)
> +    defines = {}
> +    defines['ANDROID_PACKAGE_NAME'] = CONFIG['ANDROID_PACKAGE_NAME']
> +    defines['MOZ_APP_DISPLAYNAME'] = CONFIG['MOZ_APP_DISPLAYNAME']
> +    # Includes.
> +    defines['STRINGSPATH'] = android_strings_dtd
> +    defines['SYNCSTRINGSPATH'] = sync_strings_dtd

I would probably write this as one multi-line dict literal (or `dict(foo=bar, ...)`), but I'm not really hung up on it.

::: mobile/android/base/locales/moz.build:39
(Diff revision 2)
>      '--fallback',
>      # The `locales/en-US` in this path will not be rewritten.
>      TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'
>  ]
> +
> +LOCALIZED_GENERATED_FILES += ['../res/values{AB_rCD}/strings.xml']

Do we have any tests to verify that generating a file into a path that goes up a level works properly? That feels like the kind of thing that could break in subtle ways. (I wouldn't block landing this patch on that, just something to think about.)
Attachment #8952537 - Flags: review?(ted) → review+
Comment on attachment 8952890 [details]
Bug 1439742 - Part 3: Restore single-locale repack behaviour. .mielczarek

https://reviewboard.mozilla.org/r/222122/#review228770

::: mobile/android/base/locales/moz.build:19
(Diff revision 2)
> +#
> +# Therefore, every localized generated file needs to be written into both the
> +# default resources (res/{values,raw}) and the locale-specific resources
> +# (res/{values,raw}-AB-rCD), depending on Make target magic controlled in
> +# mobile/android/base (for regular builds) and in mobile/android/locales (for
> +# multi-locale builds and single-locale repacks).

To clarify: the `{AB_rCD}` variant will not wind up being generated in an en-US build, correct? (An extra comment to that effect might be helpful here.)

::: mobile/android/base/locales/moz.build:35
(Diff revision 2)
> -]
> +    ]
> -browsersearch.flags += ['--verbose']
> +    browsersearch.flags += ['--verbose']
> -browsersearch.flags += [
> +    browsersearch.flags += [
> -    '--fallback',
> +        '--fallback',
> -    # The `locales/en-US` in this path will not be rewritten.
> +        # The `locales/en-US` in this path will not be rewritten.
> -    TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'
> +        TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'

Any reason not to simply combine these two assignments into one multi-line list? (same thing in suggestedsites below)
Attachment #8952890 - Flags: review?(ted) → review+
Blocks: 1441305
Comment on attachment 8952535 [details]
Bug 1439742 - Pre: Lift AB_rCD to ambient Make environment. .mielczarek

https://reviewboard.mozilla.org/r/221756/#review228780

> This format was not familiar to me so I went and looked it up. I think an explanatory link in this comment would be useful, probably to: https://developer.android.com/guide/topics/resources/providing-resources.html#AlternativeResources

I've boned up the comment, and reference a ticket tracking the work to migrate to BCP-47: https://bugzilla.mozilla.org/show_bug.cgi?id=1441305.

> You don't seem to set `INCLUDED_AB_RCD_MK` in AB_rCD.mk.

I realized it wasn't needed.  I'm dropping the guard entirely, and will use the `backend_file.write_once` thing too -- thanks for that!

> Is there a reason you made *just* this change and not either:
> 1) leave the `$(dir...)` vars there or
> 2) remove the `$(dir..)` vars?

This was just in staging the patch.  The final result is sensible, so I'm not going to worry about this -- lots will get squashed for landing.  Sorry that this wasn't clean.
Comment on attachment 8952536 [details]
Bug 1439742 - Part 1: Allow {AB_CD} and {AB_rCD} in LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/221758/#review228772

> Is it worth filing a follow-up to improve this, or should it wait until we figure out how to remove the make backend from repacks?

This isn't really RecursiveMake specific, it has to do with using properties of the file name rather than independent metadata to determine if it should be localized.  It's probably worth solving that problem more generally if it crops up again, but since this is unusual functionality (it's `compare-locales`-like) I don't think we need to make it better anytime soon.

> A little nitpicky, but I would prefer if you used a new variable here instead of reassigning `obj.outputs`.

I've done this.

> You added test data for both `localized-files-AB_CD` and `localized-generated-files-AB_CD` but it looks like you only wrote a test for the latter, unless I missed something. Is this testdata leftover from some earlier work?

Yes, it was.  Deleted.

> This is mentioned in the docs for `LOCALIZED_FILES`, so please update those to match your changes:
> https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#localized-files

Done!
Comment on attachment 8952537 [details]
Bug 1439742 - Allow {AB_CD} and {AB_rCD} in LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/221760/#review229384

::: commit-message-9542b:5
(Diff revision 2)
> +Bug 1439742 - Part 2: Use LOCALIZED_GENERATED_FILES for strings.xml. r=ted.mielczarek
> +
> +This can't be a LOCALIZED_PP_FILES, since we need to customize the
> +output location based on AB_rCD, and since we need a little more
> +flexibility than PP_FILES gives for our inputs.

I don't feel strongly.  Follow-up, for sure.  I think we should push the l10n-in-moz.build programme further before removing the tools initially built.

::: mobile/android/base/locales/generate_strings_xml.py:25
(Diff revision 2)
> +    defines = {}
> +    defines['ANDROID_PACKAGE_NAME'] = CONFIG['ANDROID_PACKAGE_NAME']
> +    defines['MOZ_APP_DISPLAYNAME'] = CONFIG['MOZ_APP_DISPLAYNAME']
> +    # Includes.
> +    defines['STRINGSPATH'] = android_strings_dtd
> +    defines['SYNCSTRINGSPATH'] = sync_strings_dtd

I'm going to leave it and revisit in future changes.  I generally like `dict(foo=bar, ...)` myself as well.

::: mobile/android/base/locales/moz.build:39
(Diff revision 2)
>      '--fallback',
>      # The `locales/en-US` in this path will not be rewritten.
>      TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'
>  ]
> +
> +LOCALIZED_GENERATED_FILES += ['../res/values{AB_rCD}/strings.xml']

No, and in fact it doesn't really work properly: the .deps file ends up out of MDDEPDIR (.deps).  It still works because the .deps file is explicitly added to EXTRA_MDDEPEND_FILES.  This is abused elsewhere in the build system, and I don't know if it's tracked anywhere.
Comment on attachment 8952890 [details]
Bug 1439742 - Part 3: Restore single-locale repack behaviour. .mielczarek

https://reviewboard.mozilla.org/r/222122/#review229388

::: mobile/android/base/locales/moz.build:19
(Diff revision 2)
> +#
> +# Therefore, every localized generated file needs to be written into both the
> +# default resources (res/{values,raw}) and the locale-specific resources
> +# (res/{values,raw}-AB-rCD), depending on Make target magic controlled in
> +# mobile/android/base (for regular builds) and in mobile/android/locales (for
> +# multi-locale builds and single-locale repacks).

Correct.  I added a note about this.

::: mobile/android/base/locales/moz.build:35
(Diff revision 2)
> -]
> +    ]
> -browsersearch.flags += ['--verbose']
> +    browsersearch.flags += ['--verbose']
> -browsersearch.flags += [
> +    browsersearch.flags += [
> -    '--fallback',
> +        '--fallback',
> -    # The `locales/en-US` in this path will not be rewritten.
> +        # The `locales/en-US` in this path will not be rewritten.
> -    TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'
> +        TOPSRCDIR + '/mobile/locales/en-US/chrome/region.properties'

Done.
Attachment #8952536 - Attachment is obsolete: true
Attachment #8952890 - Attachment is obsolete: true
Attachment #8952910 - Attachment is obsolete: true
This is looking healthy.  I tested by manually inspecting the diff.* files produced by

```
#!/bin/bash

rm -rf compare-l10n
mkdir compare-l10n
pushd compare-l10n

# From https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=580d833df9c44acec686a9fb88b5f27e9d29f68d

# Ns
curl -L -o central.en-US.apk https://queue.taskcluster.net/v1/task/HnsnE_tLTGeT5JpxWyFEwg/runs/0/artifacts/public/build/en-US/target.apk
curl -L -o central.multi.apk https://queue.taskcluster.net/v1/task/HnsnE_tLTGeT5JpxWyFEwg/runs/0/artifacts/public/build/target.apk
# Ns17
curl -L -o central.sl.apk    https://queue.taskcluster.net/v1/task/IM-lrMJ0SmS7svLAq2WT-g/runs/0/artifacts/public/build/sl/target.apk

# From https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f5a0dadea36e5b8bc69a27f7786d556060eb4e0

# N
curl -L -o try.en-US.apk https://queue.taskcluster.net/v1/task/XsCpM5ZMRpuxOIhiTyRxyA/runs/0/artifacts/public/build/en-US/target.apk
curl -L -o try.multi.apk https://queue.taskcluster.net/v1/task/XsCpM5ZMRpuxOIhiTyRxyA/runs/0/artifacts/public/build/target.apk
# Ns17
curl -L -o try.sl.apk    https://queue.taskcluster.net/v1/task/MWhNaBBZSQqwKBRCzxM4YQ/runs/1/artifacts/public/build/sl/target.apk

apktool d -f central.en-US.apk
apktool d -f central.multi.apk
apktool d -f central.sl.apk
apktool d -f try.en-US.apk
apktool d -f try.multi.apk
apktool d -f try.sl.apk

diff -qr central.en-US try.en-US > diff.en-US
diff -qr central.multi try.multi > diff.multi
diff -qr central.sl try.sl > diff.sl

popd

```

and there are no resource differences. \o/
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d304b444c23d
Pre: Remove unused MERGE_FILES and EN_US_OR_L10N_FILE{S}. r=ted.mielczarek
https://hg.mozilla.org/integration/autoland/rev/1817640e5a09
Pre: Lift AB_rCD to ambient Make environment. r=ted.mielczarek
https://hg.mozilla.org/integration/autoland/rev/546fb266153e
Allow {AB_CD} and {AB_rCD} in LOCALIZED_GENERATED_FILES. r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/d304b444c23d
https://hg.mozilla.org/mozilla-central/rev/1817640e5a09
https://hg.mozilla.org/mozilla-central/rev/546fb266153e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Core → Firefox Build System
Blocks: 1444534
Blocks: 1622065
You need to log in before you can comment on or make changes to this bug.