Closed Bug 1259555 Opened 4 years ago Closed 4 years ago

Remove INSTALL/PP_TARGETS from mobile/android/*

Categories

(Firefox Build System :: General, defect)

defect
Not set

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.
Depends on: 1252931
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.
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)
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)
Attachment #8734504 - Flags: review?(nalexander) → review+
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.
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.
(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.
Attachment #8734503 - Flags: review?(mh+mozilla)
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.
Alrighty, let's just remove what we can for now. I'll leave the rule to copy javaaddons-test.apk in the Makefile.in.
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/
Attachment #8734503 - Attachment is obsolete: true
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)
Attachment #8734504 - Flags: review?(nalexander) → review+
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 :)
(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.
https://hg.mozilla.org/mozilla-central/rev/e7da19944c6c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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)
Could have also been Bug 1260241, although I tested this locally very thoroughly.  (And wrote docs about how to test it!)
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.
Flags: needinfo?(mshal)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.