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

RESOLVED WONTFIX

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED WONTFIX
a year ago
4 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
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

a year ago
mozreview-review
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 12

a year ago
mozreview-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 13

a year ago
mozreview-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 14

a year ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8865977 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8865978 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 21

a year ago
mozreview-review-reply
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
```
(Assignee)

Comment 22

a year ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8865974 - Attachment is obsolete: true
Attachment #8865974 - Flags: review?(mshal)
(Assignee)

Updated

a year ago
Attachment #8865975 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8865976 - Attachment is obsolete: true
(Assignee)

Comment 26

a year ago
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+
(Assignee)

Comment 27

a year ago
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8879626 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

a year ago
mozreview-review
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+
(Assignee)

Updated

a year ago
Depends on: 1374832
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a year ago
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+
(Assignee)

Updated

a year ago
Attachment #8879627 - Flags: review+ → review?(nalexander)
(Assignee)

Comment 38

a year ago
mozreview-review
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+

Comment 39

a year ago
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

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b44a1a078e64
https://hg.mozilla.org/mozilla-central/rev/0b7af9b62aff
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1375401
(Assignee)

Comment 41

a year ago
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
status-firefox56: fixed → ---
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---
Assignee: nobody → nalexander
(Assignee)

Comment 43

11 months ago
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)
(Assignee)

Comment 44

4 months ago
Now that we're 100% Gradle (https://bugzilla.mozilla.org/show_bug.cgi?id=1414415), this isn't necessary.
Status: REOPENED → RESOLVED
Last Resolved: a year ago4 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.