Closed Bug 1355625 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

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.
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 on attachment 8865975 [details]
Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

https://reviewboard.mozilla.org/r/137570/#review140812
Attachment #8865975 - Flags: review?(s.kaspari) → review+
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
Attachment #8865976 - Flags: review?(s.kaspari) → review+
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?
Attachment #8865974 - Flags: review?(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
Attachment #8865977 - Flags: review?(mshal) → review+
I'd like to re-review the first part, but it should be quick.
Attachment #8865977 - Attachment is obsolete: true
Attachment #8865978 - Attachment is obsolete: true
Depends on: 1255915
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).
Flags: needinfo?(nalexander)
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
```
(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.
Flags: needinfo?(nalexander)
Attachment #8865974 - Attachment is obsolete: true
Attachment #8865974 - Flags: review?(mshal)
Attachment #8865975 - Attachment is obsolete: true
Attachment #8865976 - Attachment is obsolete: true
Comment on attachment 8879626 [details]
Bug 1355625 - Part 2: Merge Android resources before invoking aapt.

Carry-forward r+.
Attachment #8879626 - Flags: review?(s.kaspari) → review+
Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

Carry-forward r+.
Attachment #8879627 - Flags: review?(s.kaspari) → review+
Attachment #8879626 - Attachment is obsolete: true
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.
Attachment #8879625 - Flags: review?(mshal) → review+
Depends on: 1374832
Comment on attachment 8879627 [details]
Bug 1355625 - Part 2: Tweak the Gradle build to agree more with moz.build.

Carry forward r+.
Attachment #8879627 - Flags: review?(s.kaspari) → review+
Attachment #8879627 - Flags: review+ → review?(nalexander)
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.
Attachment #8879627 - Flags: review?(nalexander) → review+
Pushed by nalexander@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b44a1a078e64
https://hg.mozilla.org/mozilla-central/rev/0b7af9b62aff
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1375401
I'm fingering this for Bug 1375401 and friends.  I'll figure out how to flag this for backout ASAP.
Backed out in https://hg.mozilla.org/mozilla-central/rev/61edb6b490719686c8e9f8e3bf4bdd401aa1c2b5
Status: RESOLVED → REOPENED
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---
Assignee: nobody → nalexander
Clearing NI, since I'm not going to get to this anytime soon.  (And we might take a different route to --with-gradle anyway.)
Flags: needinfo?(nalexander)
Now that we're 100% Gradle (https://bugzilla.mozilla.org/show_bug.cgi?id=1414415), this isn't necessary.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.