1443208 - Migrate Gradle invocation in mobile/android/base to GENERATED_FILES

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

Even after Bug 1255924, there's still a bunch of Makefile.in hairiness to invoke Gradle in m/a/b.  It's tied to l10n and specifically the "nodeps pattern" that evolved to support l10n repacks -- see https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/mobile/android/base/Makefile.in#100 and the copious comments around it.

I think that we can remove the "nodeps pattern" entirely now that Bug 1430313 and Bug 1439742 have landed.  And we might be able to stop building during packaging entirely -- see https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/toolkit/mozapps/installer/upload-files-APK.mk#62 and https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/toolkit/mozapps/installer/upload-files-APK.mk#83 -- and instead push those invocations out of |mach package| and into the multi-locale build and single-locale repack steps.  That would speed up local builds and simplify a particularly complex part of Android l10n repacks.

If we can do that, then expressing the gecko.apk generation as a GENERATED_FILES task that invokes Gradle via a Python script becomes really easy.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1443208 - Pre: Add force flag to GENERATED_FILES. .mielczarek

The forces on the system are such that I really need to be able to
FORCE a few RecursiveMake targets in order to make Android and Gradle
use GENERATED_FILES and LOCALIZED_GENERATED_FILES.  Over time, I hope
to avoid FORCE, but that time is not now.

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

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

This small change is actually very significant.  Previously, |mach
package| for mobile/android had two jobs:

1) produce a final APK

2) rebuild parts of the APK that might have been silently modified by
   l10n mechanisms, both from multi-locale builds and single-locale
   repacks

This second part has never been sensible but has been difficult to
alter until recently, since the l10n mechanisms have been out of
mozilla-central and difficult to modify and test.  That's less true
now.

This patch:

a) removes the rebuild parts (the step labeled 2) above (which I
   generally refer to as the "nodeps mechanism")

b) uses the APKs produced by Gradle directly, without the copying
   indirection from m/a/base/Makefile.in

c) does the rebuild for multi-locale builds as an explicit step in the
   appropriate mozharness script

d) does the rebuild for each single-locale repack as another step in
   the existing `installers-%` target in m/a/locales/Makefile.in (it's
   not easy to remove this from the Makefile, since the repackage is
   invoked immediately after (it's the `repackage-zip-$*` target))

The new m/a/gradle.py file will grow additional tasks in tickets to
follow, hence the lock file and pre-factored form.

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

Comment 3

[Nick Alexander :nalexander [he/him]]

There's a happy looking try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eca3cc5b90083b5cf0aad262c15106bba8efaef.  Manually APK comparisons to follow.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8957730 [details]
Bug 1443208 - Pre: Add force flag to GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/226686/#review232832

::: python/mozbuild/mozbuild/backend/recursivemake.py:596
(Diff revision 1)
>             backend=' backend.mk' if obj.flags else '',
>             # Locale repacks repack multiple locales from a single configured objdir,
>             # so standard mtime dependencies won't work properly when the build is re-run
>             # with a different locale as input. IS_LANGUAGE_REPACK will reliably be set
>             # in this situation, so simply force the generation to run in that case.
> -           repack_force=' $(if $(IS_LANGUAGE_REPACK),FORCE)' if obj.localized else '',
> +           force=' FORCE' if obj.force else (' $(if $(IS_LANGUAGE_REPACK),FORCE)' if obj.localized else ''),

This is complicated enough that I might lift this out into a separate conditional. (I find nested inline conditionals like this pretty hard to understand.)

::: python/mozbuild/mozbuild/frontend/context.py:1323
(Diff revision 1)
>          passed as extra arguments following the inputs.
> +
> +        When the ``force`` attribute is present, the file is generated every
> +        build, regardless of whether it is stale.  This is special to the
> +        RecursiveMake backend and intended for special situations only (e.g.,
> +        localization).  Please consult a build peer before using ``force``.

Thanks for documenting this!

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/226688/#review232844

Looking better and better!

FYI this patch seems to have some extraneous copies of files under `python/mozbuild/mozbuild/test/backend/data`. Perhaps rebase mixup cruft?

::: mobile/android/base/Makefile.in:46
(Diff revision 1)
>  	  res/values$(AB_rCD)/strings.xml \
>  	  res/raw$(AB_rCD)/suggestedsites.json \
>  	  res/raw$(AB_rCD)/browsersearch.json \
>  	  AB_CD=$*
>  
> -gradle_dir := $(topobjdir)/gradle/build/mobile/android
> +GeneratedJNIWrappers.cpp GeneratedJNIWrappers.h GeneratedJNINatives.h : android_apks

At some point we're going to want to figure out how to express dependencies like this in moz.build files, because tup builds won't work without them.

::: mobile/android/gradle.py:22
(Diff revision 1)
> +    # Building the same Gradle root project with multiple concurrent processes
> +    # is not well supported, so we use a simple lock file to serialize build
> +    # steps.
> +    lock_path = '{}/gradle/mach_android.lockfile'.format(buildconfig.topobjdir)
> +    ensureParentDir(lock_path)
> +    lock_instance = lock_file(lock_path)

This sure would be nicer if the lockfile was a context manager.

::: mobile/android/gradle.py:25
(Diff revision 1)
> +    lock_path = '{}/gradle/mach_android.lockfile'.format(buildconfig.topobjdir)
> +    ensureParentDir(lock_path)
> +    lock_instance = lock_file(lock_path)
> +    try:
> +        cmd = [
> +            '{topsrcdir}/mach'.format(topsrcdir=buildconfig.topsrcdir),

I would probably prefer to write this as `mozpath.join(buildconfig.topsrcdir, 'mach')`.

::: mobile/android/gradle.py:30
(Diff revision 1)
> +            '{topsrcdir}/mach'.format(topsrcdir=buildconfig.topsrcdir),
> +            'android',
> +            verb,
> +        ]
> +        cmd.extend(args)
> +        return subprocess.check_call(cmd)

Given that `check_call` only ever returns zero (and raises for failure), returning its value doesn't seem very useful.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #3)
> There's a happy looking try build at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9eca3cc5b90083b5cf0aad262c15106bba8efaef.  Manually
> APK comparisons to follow.

Sadly, my manual APK comparisons show that this isn't working in the wild.  More to follow.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1443208 - Fix: Allow android-assemble-app.

The previous commit works perfectly... except the mozharness action
only happens if you specify it in many places.  Here's the rest of the
places, so that it actually gets applied.

I took the time to inline a variable that can no longer be
meaningfully changed.

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

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957730 [details]
Bug 1443208 - Pre: Add force flag to GENERATED_FILES. .mielczarek

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

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

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

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8960709 [details]
Bug 1443208 - Fix: Allow android-assemble-app.

https://reviewboard.mozilla.org/r/229462/#review235196

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

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

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/226688/#review232844

> At some point we're going to want to figure out how to express dependencies like this in moz.build files, because tup builds won't work without them.

I concur, but this is just a stepping stone on the road to having everything be a `GENERATED_FILES` entry, which at least moves the problem into the build backend generation.

> This sure would be nicer if the lockfile was a context manager.

I concur, but it's follow-up.

> Given that `check_call` only ever returns zero (and raises for failure), returning its value doesn't seem very useful.

I've replaced this to do the `check_call` and then return 0.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957730 [details]
Bug 1443208 - Pre: Add force flag to GENERATED_FILES. .mielczarek

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

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

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

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957730 [details]
Bug 1443208 - Pre: Add force flag to GENERATED_FILES. .mielczarek

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

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8957731 [details]
Bug 1443208 - Express Fennec APK with GENERATED_FILES. .mielczarek

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe70510127c5
Pre: Add force flag to GENERATED_FILES. r=ted.mielczarek
https://hg.mozilla.org/integration/autoland/rev/e001dfa526ad
Express Fennec APK with GENERATED_FILES. r=ted.mielczarek

Comment 18

[Nick Alexander :nalexander [he/him]]

Ah!  It looks like that my manual checking missed that the default strings in a single-locale repack are now en-US, rather htan the locale's strings.  These repacks are not high-value (I'm not sure they even have a tier label), so I'm not going to back out.  I'll address this in follow-up, or as part of the series I'm working on to finish this "build mobile/android in moz.build with GENERATED_FILES" programme in Bug 1444546.

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/fe70510127c5
https://hg.mozilla.org/mozilla-central/rev/e001dfa526ad