Closed Bug 1443208 Opened 2 years ago Closed 2 years ago

Migrate Gradle invocation in mobile/android/base to GENERATED_FILES

Categories

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

enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1444534
There's a happy looking try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eca3cc5b90083b5cf0aad262c15106bba8efaef.  Manually APK comparisons to follow.
Blocks: 1444546
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!
Attachment #8957730 - Flags: review?(ted) → review+
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.
Attachment #8957731 - Flags: review?(ted) → review+
(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.
Attachment #8960709 - Flags: review?(ted)
Comment on attachment 8960709 [details]
Bug 1443208 - Fix: Allow android-assemble-app.

https://reviewboard.mozilla.org/r/229462/#review235196
Attachment #8960709 - Flags: review?(ted) → review+
Attachment #8960709 - Attachment is obsolete: true
Attachment #8960709 - Flags: review?(jlund)
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.
Pushed by nalexander@mozilla.com:
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
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.
https://hg.mozilla.org/mozilla-central/rev/fe70510127c5
https://hg.mozilla.org/mozilla-central/rev/e001dfa526ad
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1449624
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.