1355625 - Make moz.build org.mozilla.gecko.R match Gradle org.mozilla.gecko.R

Status: RESOLVED WONTFIX

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

Description

[Nick Alexander :nalexander [he/him]]

At this time, I don't believe that the Firefox for Android team will have the QA time to verify that switching the product from moz.build to Gradle does not have unforeseen impacts in the wild.  In response, I'm driving the two build systems to be bit-for-bit identical, thereby avoiding the need for a costly QA process to quantify unknowns and estimate risks.

The major part of making the two APK outputs bit-for-bit identical is to make org.mozilla.gecko.R identical in the two builds.  It turns out this isn't as hard as one might think.

The Gradle build does two things:

1) it copies all the relevant resources into a single merged directory;
2) it merges and orders all of the values*/*xml files.

To copy this, we need to do some work before running aapt.  Luckily, moz.build has grown features that make this tractable.  This ticket tracks:

0) invoking a wrapper around aapt using moz.build's GENERATED_FILES mechanism;
1) making the wrapper do 1) above;
2) making the wrapper do 2) above;
3) verifying the outputs agree.

Bug 1355616 lays the groundwork for 1) here, making the merging simpler.  It turns out that 2) isn't that complicated.

Comment 1

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96be201ea9b

Comment 2

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=533f7bcf7a53

Comment 3

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8786d59f441e

Comment 4

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7ade38fe50

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 1: Invoke aapt using py_action.

This is partial work, which I will fold down before landing.  This
adds a py_action invocation wrapping aapt, and lets me compare the
output easily.  With this, the R.txt produced by the aapt invocation
is the same as the R.txt produced by the py_action invocation.

The reason to do this is to be able to process the various inputs
before aapt gets to them.  The rest of this ticket will implement a
hacky implementation of the Gradle build system's resource merging
algorithm; once we have the moz.build and Gradle resources identical,
we'll be one big step closer to producing bit-identical builds and
flipping the switch in favour of Gradle.

Originally I wrote this to use GENERATED_FILES, but it produced a
world of pain.  Since Android's aapt tool is fundamentally directory
oriented, not file oriented, it required adding support for FORCE to
GENERATED_FILES and required directory crawling and FileAvoidWrite in
the wrapper.  After getting that working I was eventually stymied by
the arcane requirements of the Android re-packaging system, which
interacts with the l10n system.  I would have required support for
building GENERATED_FILES in the libs tier rather than the misc tier.
After that realization I gave up and turned to py_action: the
dependencies on branding are just too entangled with l10n to use
GENERATED_FILES.

And, in the not-so-distant future, all of this moz.build and
Makefile.in chicanery will be deleted in favour of invoking Gradle at
the appropriate points!

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

This is a restricted version of the Android Gradle build tool resource
merging algorithm.  The goal is for the R.java produced by moz.build
to have exactly the same resource ID map as the R.java produced by Gradle.

This implementation does two things:

1) Copy all resources into a single directory.  There's the potential
for conflicts based on order here; Android's resource merging
algorithm is intended to support product flavors and build types; it's
non-trivial.  Our moz.build "algorithm" is based on the order the
directories are provided to aapt, and to the best of my knowledge
Fennec does not depend on this ordering; it's trivial.

2) Merge all XML values into a single values.xml (depending on
resource configurations).  This is the crucial piece: we need to
collect and order the resulting XML nodes exactly as the Gradle
resource merging implementation does.  Luckily, it's not hard!

In both these places there's are two wrinkles around versioning.  The
first is that the Gradle resource merging algorithm assigns versions
to unversioned resources depending on what Android SDK version added
the features that the resource is filtered on.  For example, "xlarge"
was added in v4 and "ldrtl" was added in v13, so "drawable-xlarge" is
really "drawable-xlarge-v4" and "drawable-ldrtl" is really
"drawable-ldrtl-v13".  The second wrinkle is that some versions of a
resource may be unreachable: this happens if, for example, a v4 and a
v11 version is provided.  Since the minimum SDK is (currently) v15,
the v11 version can always be used, meaning that the v14 version will
never be used and can be culled.

With this work, the R.txt produced by moz.build is identical to the
R.txt produced by Gradle, paving the way to get other key parts of the
build -- including the compiled Java .class files -- identical!

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

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 3: Tweak the Gradle build to agree more with moz.build.

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

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 4: TO BE FOLDED - Use the Python aapt invocation.

This will be folded into the first patch.

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

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - DO NOT LAND - Add |mach compare-mach-and-gradle| to compare builds.

I developed and used these scripts to compare the Gradle and
moz.build build outputs to each other.

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

Comment 10

[Nick Alexander :nalexander [he/him]]

mshal: flagging you for the little build system tweak since originally I wrote this to use GENERATED_FILES and you implemented the multi-output support for that.  See the commit comment explaining why GENERATED_FILES can't be used here :(

I've structured this to support testing during development; if you could just review the combined Makefile.in changes and whatever Python you care to review that would be great.  I'll work with sebastian on the Android details while he's here in YVR.

Thanks!

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865975 [details]
Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

https://reviewboard.mozilla.org/r/137570/#review140812

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865976 [details]
Bug 1355625 - Part 3: Tweak the Gradle build to agree more with moz.build.

https://reviewboard.mozilla.org/r/137572/#review140814

::: mobile/android/app/build.gradle:144
(Diff revision 1)
>                  srcDir "${topsrcdir}/mobile/android/base/java"
>                  srcDir "${topsrcdir}/mobile/android/search/java"
>                  srcDir "${topsrcdir}/mobile/android/services/src/main/java"
>  
> +                // These aren't included in moz.build builds, for reasons unknown.
> +                exclude "org/mozilla/gecko/dlc/CleanupAction.java"

bug 1363520

::: mobile/android/app/build.gradle:145
(Diff revision 1)
>                  srcDir "${topsrcdir}/mobile/android/search/java"
>                  srcDir "${topsrcdir}/mobile/android/services/src/main/java"
>  
> +                // These aren't included in moz.build builds, for reasons unknown.
> +                exclude "org/mozilla/gecko/dlc/CleanupAction.java"
> +                exclude "org/mozilla/gecko/tabs/TabsLayoutRecyclerAdapter.java"

bug 1363551

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8865974 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

https://reviewboard.mozilla.org/r/137568/#review141272

::: mobile/android/base/Makefile.in:445
(Diff revision 1)
>  		-J $(4) \
>  		--output-text-symbols $(5) \
>  		--ignore-assets "$$(ANDROID_AAPT_IGNORE)"
> +	mkdir -p $(subst /,-2/,$(4))
> +	mkdir -p aapt
> +	$$(call py_action,aapt_package,-f -m \

Since you're not able to use GENERATED_FILES, you'll need to manually add the dependency on aapt_package.py. Right now the aapt command does not re-execute if aapt_package.py changes.

::: python/mozbuild/mozbuild/action/aapt_package.py:29
(Diff revision 1)
> +import subprocess
> +import sys
> +
> +import buildconfig
> +from mozbuild.preprocessor import Preprocessor
> +from mozbuild.util import ensureParentDir 

There are a few unused imports here (OrderedDict, Preprocessor, ensureParentDir)

::: python/mozbuild/mozbuild/action/aapt_package.py:92
(Diff revision 1)
> +        buildconfig.substs['AAPT'],
> +        'package',
> +    ] + \
> +    (['-f'] if args.f else []) + \
> +    [
> +        '-m',

How come you always pass in -m here? Should it depend on args.m?

Comment 14

[Michael Shal [:mshal]]

Comment on attachment 8865977 [details]
Bug 1355625 - Part 4: TO BE FOLDED - Use the Python aapt invocation.

https://reviewboard.mozilla.org/r/137574/#review141280

Comment 15

[Michael Shal [:mshal]]

I'd like to re-review the first part, but it should be quick.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8865974 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

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

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8865975 [details]
Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

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

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8865976 [details]
Bug 1355625 - Part 3: Tweak the Gradle build to agree more with moz.build.

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

Comment 19

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5d8b8a922f

Comment 20

[Michael Shal [:mshal]]

I think my comments in #c13 still need to be addressed, though I can't tell if mozreview is just being wonky (mozreview still shows one open issue and the r? cleared, and the diff between version 1 and version 2 of the patch just seems to show unrelated differences from bug 1365089).

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8865974 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

https://reviewboard.mozilla.org/r/137568/#review141272

> Since you're not able to use GENERATED_FILES, you'll need to manually add the dependency on aapt_package.py. Right now the aapt command does not re-execute if aapt_package.py changes.

I did this -- it's in the second patch now.

> There are a few unused imports here (OrderedDict, Preprocessor, ensureParentDir)

Ran it through flake8/pyflakes a few times; ignored only some indentation warnings.  I see a trailing space that I'll purge eventually.

> How come you always pass in -m here? Should it depend on args.m?

It's a flag to `aapt` that we always want:
```
-m  make package directories under location specified by -J
```

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Shal [:mshal] from comment #20)
> I think my comments in #c13 still need to be addressed, though I can't tell
> if mozreview is just being wonky (mozreview still shows one open issue and
> the r? cleared, and the diff between version 1 and version 2 of the patch
> just seems to show unrelated differences from bug 1365089).

mshal -- sorry, I replied to your comments but forgot to hit publish.  I think I addressed your concerns, sorry if I didn't.  I'll set the r? flags again, 'cuz I do think it's updated correctly.

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 1: Invoke aapt using py_action.

This is partial work, which I will fold down before landing.  This
adds a py_action invocation wrapping aapt, and lets me compare the
output easily.  With this, the R.txt produced by the aapt invocation
is the same as the R.txt produced by the py_action invocation.

The reason to do this is to be able to process the various inputs
before aapt gets to them.  The rest of this ticket will implement a
hacky implementation of the Gradle build system's resource merging
algorithm; once we have the moz.build and Gradle resources identical,
we'll be one big step closer to producing bit-identical builds and
flipping the switch in favour of Gradle.

Originally I wrote this to use GENERATED_FILES, but it produced a
world of pain.  Since Android's aapt tool is fundamentally directory
oriented, not file oriented, it required adding support for FORCE to
GENERATED_FILES and required directory crawling and FileAvoidWrite in
the wrapper.  After getting that working I was eventually stymied by
the arcane requirements of the Android re-packaging system, which
interacts with the l10n system.  I would have required support for
building GENERATED_FILES in the libs tier rather than the misc tier.
After that realization I gave up and turned to py_action: the
dependencies on branding are just too entangled with l10n to use
GENERATED_FILES.

And, in the not-so-distant future, all of this moz.build and
Makefile.in chicanery will be deleted in favour of invoking Gradle at
the appropriate points!

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

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

This is a restricted version of the Android Gradle build tool resource
merging algorithm.  The goal is for the R.java produced by moz.build
to have exactly the same resource ID map as the R.java produced by Gradle.

This implementation does two things:

1) Copy all resources into a single directory.  There's the potential
for conflicts based on order here; Android's resource merging
algorithm is intended to support product flavors and build types; it's
non-trivial.  Our moz.build "algorithm" is based on the order the
directories are provided to aapt, and to the best of my knowledge
Fennec does not depend on this ordering; it's trivial.

2) Merge all XML values into a single values.xml (depending on
resource configurations).  This is the crucial piece: we need to
collect and order the resulting XML nodes exactly as the Gradle
resource merging implementation does.  Luckily, it's not hard!

In both these places there's are two wrinkles around versioning.  The
first is that the Gradle resource merging algorithm assigns versions
to unversioned resources depending on what Android SDK version added
the features that the resource is filtered on.  For example, "xlarge"
was added in v4 and "ldrtl" was added in v13, so "drawable-xlarge" is
really "drawable-xlarge-v4" and "drawable-ldrtl" is really
"drawable-ldrtl-v13".  The second wrinkle is that some versions of a
resource may be unreachable: this happens if, for example, a v4 and a
v11 version is provided.  Since the minimum SDK is (currently) v15,
the v11 version can always be used, meaning that the v14 version will
never be used and can be culled.

With this work, the R.txt produced by moz.build is identical to the
R.txt produced by Gradle, paving the way to get other key parts of the
build -- including the compiled Java .class files -- identical!

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

Comment 25

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

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

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879626 [details]
Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

Carry-forward r+.

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

Carry-forward r+.

Comment 28

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879625 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

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

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

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

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879625 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

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

Comment 31

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

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

Comment 32

[Michael Shal [:mshal]]

Comment on attachment 8879625 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

https://reviewboard.mozilla.org/r/150970/#review155758

::: mobile/android/base/Makefile.in:454
(Diff revision 3)
>  # removes the target file if any recipe command fails.
>  
>  define aapt_command
> -$(1): $$(call mkdir_deps,$(filter-out ./,$(dir $(3) $(4) $(5)))) $(2)
> +$(1): $(topsrcdir)/python/mozbuild/mozbuild/action/aapt_package.py $$(call mkdir_deps,$(filter-out ./,$(dir $(3) $(4) $(5)))) $(2)
>  	@$$(TOUCH) $$@
> -	$$(AAPT) package -f -m \
> +	$$(call py_action,aapt_package,-f -m \

I believe this -m can be removed.

::: python/mozbuild/mozbuild/action/aapt_package.py:48
(Diff revision 3)
> +
> +    # These serve to order build targets; they're otherwise ignored.
> +    parser.add_argument('ignored_inputs', nargs='*')
> +    parser.add_argument('-f', action='store_true', default=False,
> +                        help='force overwrite of existing files')
> +    parser.add_argument('-m', action='store_true', default=False,

This argument appears to be unused, since we always pass in -m below.

Comment 33

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8e32f46eeb1

Comment 34

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97a56abdb67

Comment 35

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879625 [details]
Bug 1355625 - Part 1: Invoke aapt using py_action.

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

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

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

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

Carry forward r+.

Comment 38

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

https://reviewboard.mozilla.org/r/150974/#review155942

Carry forward r+ from sebastian.  I hate MozReview.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b44a1a078e64
Part 1: Invoke aapt using py_action. r=mshal
https://hg.mozilla.org/integration/autoland/rev/0b7af9b62aff
Part 2: Tweak the Gradle build to agree more with moz.build. r=nalexander

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b44a1a078e64
https://hg.mozilla.org/mozilla-central/rev/0b7af9b62aff

Comment 41

[Nick Alexander :nalexander [he/him]]

I'm fingering this for Bug 1375401 and friends.  I'll figure out how to flag this for backout ASAP.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/61edb6b490719686c8e9f8e3bf4bdd401aa1c2b5

Comment 43

[Nick Alexander :nalexander [he/him]]

Clearing NI, since I'm not going to get to this anytime soon.  (And we might take a different route to --with-gradle anyway.)

Comment 44

[Nick Alexander :nalexander [he/him]]

Now that we're 100% Gradle (https://bugzilla.mozilla.org/show_bug.cgi?id=1414415), this isn't necessary.