Closed
Bug 1259555
Opened 8 years ago
Closed 8 years ago
Remove INSTALL/PP_TARGETS from mobile/android/*
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mshal, Assigned: mshal)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Most of these are straightforward, but installing javaaddons-test.apk requires moving the generated install targets from the misc tier to the tools tier, since the apk isn't generated by the time we build misc.
Assignee | ||
Comment 2•8 years ago
|
||
We need to install generated files later in the build in order to support installing apks. Review commit: https://reviewboard.mozilla.org/r/42301/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42301/
Attachment #8734503 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 3•8 years ago
|
||
The remaining android PP_TARGETS are for l10n or require support for passing flags to the preprocessor (bug 1259530). Review commit: https://reviewboard.mozilla.org/r/42303/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42303/
Attachment #8734504 -
Flags: review?(nalexander)
Updated•8 years ago
|
Attachment #8734504 -
Flags: review?(nalexander) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8734504 [details] MozReview Request: Bug 1259555 - Remove many PP_TARGETS / INSTALL_TARGETS from mobile/android; r?nalexander https://reviewboard.mozilla.org/r/42303/#review38849 If it's green on try, it's good for me. We may need follow-up to get some `mobile/android/base` dependencies correct, depending on whether this changes `NAME` to `$(abspath NAME)` in the produced Makefile.
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/42301/#review38851 ::: python/mozbuild/mozbuild/backend/recursivemake.py:1266 (Diff revision 1) > install_manifest.add_optional_exists(dest) > backend_file.write('%s_FILES += %s\n' % ( > target_var, self._pretty_path(f, backend_file))) > have_objdir_files = True > if have_objdir_files: > - tier = 'export' if obj.install_target == 'dist/include' else 'misc' > + tier = 'export' if obj.install_target == 'dist/include' else 'tools' Can you give me the details on this? Perhaps we should update the APK generation code to be more friendly to the rest of the build system rather than changing this.
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #5) > https://reviewboard.mozilla.org/r/42301/#review38851 > > ::: python/mozbuild/mozbuild/backend/recursivemake.py:1266 > (Diff revision 1) > > install_manifest.add_optional_exists(dest) > > backend_file.write('%s_FILES += %s\n' % ( > > target_var, self._pretty_path(f, backend_file))) > > have_objdir_files = True > > if have_objdir_files: > > - tier = 'export' if obj.install_target == 'dist/include' else 'misc' > > + tier = 'export' if obj.install_target == 'dist/include' else 'tools' > > Can you give me the details on this? Perhaps we should update the APK > generation code to be more friendly to the rest of the build system rather > than changing this. This was to support the javaaddons-test.apk file, which is built during the libs tier: TEST_HARNESS_FILES.testing.mochitest.extensions['roboextender@mozilla.org'].base += [ '!/mobile/android/tests/javaaddons/javaaddons-test.apk', ] This was originally in the Makefile.in as: tools:: $(_TEST_FILES) ... -cp $(DEPTH)/mobile/android/tests/javaaddons/javaaddons-test.apk $(TEST_EXTENSIONS_DIR)/roboextender@mozilla.org/base The tier that this happens was moved from exports to misc for similar reasons in bug 1253430 (we wanted to install things generated during the compile tier). I'm not sure how hard it would be to move apk generation into the compile tier, but that would let us skip this change. Or if there's a way to avoid installing javaaddons-test.apk for roboextender, that would work too.
Updated•8 years ago
|
Attachment #8734503 -
Flags: review?(mh+mozilla)
Comment 7•8 years ago
|
||
Comment on attachment 8734503 [details] MozReview Request: Bug 1259555 - Move install target to tools; r?glandium https://reviewboard.mozilla.org/r/42301/#review39021 We've made efforts to move things out of the tools tier, I'd rather avoid adding other things back there.
Assignee | ||
Comment 8•8 years ago
|
||
Alrighty, let's just remove what we can for now. I'll leave the rule to copy javaaddons-test.apk in the Makefile.in.
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8734504 [details] MozReview Request: Bug 1259555 - Remove many PP_TARGETS / INSTALL_TARGETS from mobile/android; r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42303/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8734503 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8734504 [details] MozReview Request: Bug 1259555 - Remove many PP_TARGETS / INSTALL_TARGETS from mobile/android; r?nalexander Meant to ask for re-review.
Attachment #8734504 -
Flags: review+ → review?(nalexander)
Updated•8 years ago
|
Attachment #8734504 -
Flags: review?(nalexander) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8734504 [details] MozReview Request: Bug 1259555 - Remove many PP_TARGETS / INSTALL_TARGETS from mobile/android; r?nalexander https://reviewboard.mozilla.org/r/42303/#review39361 If it's green on try, it's good for me. Nice cleanup! ::: mobile/android/base/moz.build:982 (Diff revision 2) > FINAL_TARGET_PP_FILES += ['package-name.txt.in'] > > DEFINES['OBJDIR'] = OBJDIR > DEFINES['TOPOBJDIR'] = TOPOBJDIR > + > +OBJDIR_PP_FILES.mobile.android.base += [ Does this work with the hacky `ANDROID_VERSION_CODE` calculation at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#28? I guess it must, or all your builds would fail. Question -- is it possible to set per-file defines with `OBJDIR_PP_FILES`? Looks like no https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#1278. I want this :)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #11) > ::: mobile/android/base/moz.build:982 > (Diff revision 2) > > FINAL_TARGET_PP_FILES += ['package-name.txt.in'] > > > > DEFINES['OBJDIR'] = OBJDIR > > DEFINES['TOPOBJDIR'] = TOPOBJDIR > > + > > +OBJDIR_PP_FILES.mobile.android.base += [ > > Does this work with the hacky `ANDROID_VERSION_CODE` calculation at > https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile. > in#28? I guess it must, or all your builds would fail. Yeah, those DEFINES end up in the objdir Makefile, and are used in the PP_TARGETS rules. > Question -- is it possible to set per-file defines with `OBJDIR_PP_FILES`? > Looks like no > https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/ > backend/recursivemake.py#1278. I want this :) Unfortunately not yet - we have bug 1259530 for this. That would help limit the scope of some of the DEFINES that are used only by the preprocessor, as well as let us port some of the remaining PP_TARGETS that have custom flags. If you have thoughts on this or particular features that you need supported, please add them to that bug.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7da19944c6c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 15•8 years ago
|
||
single-locale nightlies are busted across all locales, my suspicion is that it's this bug that regressed those: make[2]: *** No rule to make target `/builds/slave/m-cen-and-api-15-l10n_1-000000/build/mozilla-central/obj-l10n/mobile/android/base/AndroidManifest.xml', needed by `.aapt.nodeps'. Stop. make[1]: *** [repackage-zip] Error 2 make: *** [repackage-zip-ar] Error 2 Return code: 2 ar failed in make installers-ar! https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c40c0b2f3b4c778af4307e090b4063b63c806cda&filter-job_group_symbol=L10n&filter-job_group_symbol=Update-3&filter-job_group_symbol=Update-1&filter-job_group_symbol=Update-2&exclusion_profile=false&selectedJob=3583909
Flags: needinfo?(mshal)
Comment 16•8 years ago
|
||
Could have also been Bug 1260241, although I tested this locally very thoroughly. (And wrote docs about how to test it!)
Comment 17•8 years ago
|
||
There is pretty intense dependency hacking in the interaction between the packaging code and the .aapt.nodeps rule, but I think it's more likely that the dependency needs to flipped between an absolute path and a relative path around https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#500.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mshal)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•