Closed
Bug 1355625
Opened 8 years ago
Closed 7 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)
Firefox Build System
Android Studio and Gradle Integration
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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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+
Comment 15•8 years ago
|
||
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•7 years ago
|
Attachment #8865977 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8865978 -
Attachment is obsolete: true
Assignee | ||
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
Attachment #8865974 -
Attachment is obsolete: true
Attachment #8865974 -
Flags: review?(mshal)
Assignee | ||
Updated•7 years ago
|
Attachment #8865975 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8865976 -
Attachment is obsolete: true
Assignee | ||
Comment 26•7 years 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•7 years 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•7 years ago
|
Attachment #8879626 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years 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 | ||
Comment 33•7 years ago
|
||
Assignee | ||
Comment 34•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years 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•7 years ago
|
Attachment #8879627 -
Flags: review+ → review?(nalexander)
Assignee | ||
Comment 38•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b44a1a078e64
https://hg.mozilla.org/mozilla-central/rev/0b7af9b62aff
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee | ||
Comment 41•7 years ago
|
||
I'm fingering this for Bug 1375401 and friends. I'll figure out how to flag this for backout ASAP.
Status: RESOLVED → REOPENED
status-firefox56:
fixed → ---
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---
Updated•7 years ago
|
Assignee: nobody → nalexander
Assignee | ||
Comment 43•7 years 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•7 years ago
|
||
Now that we're 100% Gradle (https://bugzilla.mozilla.org/show_bug.cgi?id=1414415), this isn't necessary.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → WONTFIX
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•