Closed Bug 1254355 Opened 8 years ago Closed 7 years ago

Move preprocessed targets out of mobile/android/base/Makefile.in

Categories

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

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

Bug 1254355 - Follow-up: Use VPATH relative paths to match GENERATED_FILES targets.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1254355 - Part 2: Generate AndroidManifest.xml using GENERATED_FILES.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1254355 - Post: Make android-* jobs depend on more of the build.
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
We preprocess AndroidManifest.xml.in and some sundry .java.in in mobile/android/base/Makefile.in.  This ticket tracks moving those to moz.build GENERATED_FILES in some way.
This has the slight cost of not displaying all of the -D preprocessor
options in the logs.

Review commit: https://reviewboard.mozilla.org/r/39327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39327/
Attachment #8729271 - Flags: review?(mh+mozilla)
Comment on attachment 8729270 [details]
MozReview Request: Bug 1254355 - Part 1: Make AndroidManifest.xml and fennec_ids.txt GENERATED_FILES. r?glandium

https://reviewboard.mozilla.org/r/39325/#review36105

::: python/mozbuild/mozbuild/action/generate_android.py:24
(Diff revision 1)
> +def _local_defines():

Ugh, this is the worst of both worlds. I'd rather keep these things in Makefile.in than do this. Plus, you're repeating what's already in moz.build here. You can be sure this would get out of sync very quickly.

This ought to be a generic way to replace PP_TARGETS, using the local defines from moz.build. See what we do with generate_symbols_file.py to add the local defines.

Now, the obvious problem with this "plan" is that the Makefile contains DEFINES. But let's see what is there:
- ANDROID_VERSION_CODE which is either coming from MOZ_APP_ANDROID_VERSION_CODE or from executing a script.
- MOZ_ANDROID_SHARED_ID which, it turns out, could just be in moz.build already
- MOZ_BUILDID, which is only used in AppConstants.java.in so, you don't have to care about it for this bug, except it's also used for the alternative form of ANDROID_VERSION_CODE.

Now the question is, why does ANDROID_VERSION_CODE have to have that fallback in Makefile.in?
Attachment #8729270 - Flags: review?(mh+mozilla)
Comment on attachment 8729271 [details]
MozReview Request: Bug 1254355 - Part 2: Make *Constants.java GENERATED_FILES. r?glandium

https://reviewboard.mozilla.org/r/39327/#review36107

::: mobile/android/base/moz.build
(Diff revision 1)
> -for var in ('MOZ_ANDROID_ANR_REPORTER', 'MOZ_LINKER_EXTRACT', 'MOZ_DEBUG',

Err, one step forward, three steps backwards.
Attachment #8729271 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/39325/#review36167

::: python/mozbuild/mozbuild/action/generate_android.py:24
(Diff revision 1)
> +from mozbuild.android_version_code import android_version_code
> +
> +import os
> +
> +
> +def _local_defines():

I think your "worst of both worlds" is addressed by the second commit, which just moves the `DEFINES` stuff out of `moz.build` and into the script.

We have talked many times about a generic `moz.build` `PP_TARGETS`; do we really want this?  It's more generic than `GENERATED_FILES` with a per-use script.

Looking at https://dxr.mozilla.org/mozilla-central/search?q=PP_TARGETS+-path%3Aobj*&redirect=true&case=false I see a few things that can be `FINAL_TARGET_PP_FILES` and then a few things that mostly want `topsrcdir`.  I thought the vision was that such things would get their own per-use script.

I'll remake this patch to add `PREPROCESSED_FILES` to `moz.build` if that's desired.

As for the version code -- most of the time we really need to generate it.  We could make it work just like buildid.h, but it really seems like it *should* be in a `GENERATED_FILES` script -- like this patch does.
glandium: I thought we were explicitly trying to avoid that generic primitive, and to use per-instance `GENERATED_FILES` scripts.  If you feel this should be a generic `PREPROCESSED_FILES` in `moz.build`, I'll do that.

We can make the version code computation just like buildid.h if that's the most sensible.  And I just realized this should depend on buildid.h in any case, which we'd get "for free" if we made this a generic preprocessed declaration and just included "android_version_code.h" in that file.
Flags: needinfo?(mh+mozilla)
You can also have a generic preprocessor GENERATED_FILES script... you just need a way to pass it the local define flags.
Flags: needinfo?(mh+mozilla)
Attachment #8729270 - Attachment is obsolete: true
Attachment #8729271 - Attachment is obsolete: true
Comment on attachment 8858928 [details]
Bug 1254355 - Part 1: Prepare to generalize generate_build_config.

https://reviewboard.mozilla.org/r/130920/#review133524

This seems like a useful refactor. I think I see where you're going with this...
Attachment #8858928 - Flags: review?(gps) → review+
Comment on attachment 8858929 [details]
Bug 1254355 - Part 2: Generate AndroidManifest.xml using GENERATED_FILES.

https://reviewboard.mozilla.org/r/130922/#review133526

Nice cleanup! Amazing how much gunk you were able to delete by cutting out make.
Attachment #8858929 - Flags: review?(gps) → review+
I fixed a small mistake (removed a DEFINES used by PP_TARGET_FILES) and manually tested the compiled AndroidManifest.xml from a green try build and a central build.  Landing!
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1228043614d0
Part 1: Prepare to generalize generate_build_config. r=gps
https://hg.mozilla.org/integration/autoland/rev/a5af7f7132f6
Part 2: Generate AndroidManifest.xml using GENERATED_FILES. r=gps
https://hg.mozilla.org/mozilla-central/rev/1228043614d0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8859649 [details]
Bug 1254355 - Pre: Use #ifdef, not #if, for MOZ_CRASHREPORTER.

https://reviewboard.mozilla.org/r/131664/#review134428

::: mobile/android/base/AndroidManifest.xml.in:267
(Diff revision 1)
>  #include ../services/manifests/FxAccountAndroidManifest_activities.xml.in
>  #ifdef MOZ_ANDROID_SEARCH_ACTIVITY
>  #include ../search/manifests/SearchAndroidManifest_activities.xml.in
>  #endif
>  
> -#if MOZ_CRASHREPORTER
> +#ifdef MOZ_CRASHREPORTER

gps: I made an error hand comparing the manifests, and there was an issue due to the "special" handling of defines; they can be "defined()", or "0", or "1" -- and `MOZ_CRASHREPORTER` is handled differently throughout the tree.  This makes it be "defined()", which agrees with most places in the tree.
Comment on attachment 8859649 [details]
Bug 1254355 - Pre: Use #ifdef, not #if, for MOZ_CRASHREPORTER.

https://reviewboard.mozilla.org/r/131664/#review134970
Attachment #8859649 - Flags: review?(gps) → review+
Comment on attachment 8859650 [details]
Bug 1254355 - Post: Make android-* jobs depend on more of the build.

https://reviewboard.mozilla.org/r/131666/#review134972
Attachment #8859650 - Flags: review?(gps) → review+
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
Attachment #8858928 - Attachment is obsolete: true
Only part 2 was backed out, making for a very confusing landing order.  But hopefully we're okay now!
Flags: needinfo?(nalexander)
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41d155de1019
Pre: Use #ifdef, not #if, for MOZ_CRASHREPORTER. r=gps
https://hg.mozilla.org/integration/autoland/rev/6a0e9a3f2673
Part 2: Generate AndroidManifest.xml using GENERATED_FILES. r=gps
https://hg.mozilla.org/integration/autoland/rev/5cc1ea46d811
Post: Make android-* jobs depend on more of the build. r=gps
Sigh, I busted l10n repacks.  Try push that should fix the issue above.  Will try to get a build peer review for the (trivial) fix and reland without backing out.
Attachment #8859649 - Attachment is obsolete: true
Attachment #8858929 - Attachment is obsolete: true
Attachment #8859650 - Attachment is obsolete: true
Comment on attachment 8860533 [details]
Bug 1254355 - Follow-up: Use full paths to match GENERATED_FILES targets.

https://reviewboard.mozilla.org/r/132550/#review135426

What was actually breaking without the abspath?
Attachment #8860533 - Flags: review+
(In reply to Michael Shal [:mshal] from comment #36)
> Comment on attachment 8860533 [details]
> Bug 1254355 - Follow-up: Use full paths to match GENERATED_FILES targets.
> 
> https://reviewboard.mozilla.org/r/132550/#review135426
> 
> What was actually breaking without the abspath?

l10n repacks.  The gecko-nodeps.ap_ depends on AndroidManifest.xml, which couldn't be found due to the path.  (It would have been a problem for everything depending on AndroidManifest.xml.)  It's just hard to test the l10n stuff locally, and I forgot about the absolute paths/virtual dirs thing in Make.  It's bitten me a lot of times; it's part of the motivation for GENERATED_FILES!
Attachment #8860533 - Flags: review?(gps)
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33d4c8885613
Follow-up: Use full paths to match GENERATED_FILES targets. r=mshal
Callek: I was able to run an Android Nightly (N) build in try but I never got a Nightly Single-locale Repack (N1 .. N5) build in try.  Some of the following produced builds, and some failed in the decision task, but none got my single-locale repack:

hg push-to-try -m "try: -b o -p android-api-15-nightly,android-api-15 -u none -t none --include-nightly"
hg push-to-try -m "try: -b o -p android-api-15-nightly,android-api-15,android -u none -t none --include-nightly"
hg push-to-try -m "try: -b o -p android-api-15 --include-nightly"
hg push-to-try -m "try: -b o -p android-api-15,android-api-15-gradle-dependencies,android-api-15-frontend,android-x86,android-lint --include-nightly"

Can you suggest the correct incantation for N1 builds?  Thanks!
Flags: needinfo?(nalexander) → needinfo?(bugspam.Callek)
(In reply to Nick Alexander :nalexander from comment #42)
> Callek: I was able to run an Android Nightly (N) build in try but I never
> got a Nightly Single-locale Repack (N1 .. N5) build in try.
That's bug 1358865. The failing decision tasks were an infra issue on the weekend.
No, pretty sure this wasn't the bug Aryx mentioned...

There may be another way, but this is what I found today. (I'm sad that stuff changed enough here, but I want to make this work for you today, rather than fixing it systemically first.)

'try: -b o -p linux,linux-nightly,linux64,linux64-nightly,android-api-15,android-api-15-nightly -u none -t none --include-nightly -j android-api-15-nightly'

^ Should do it, IF you apply the following patch as well:

diff --git a/taskcluster/taskgraph/try_option_syntax.py b/taskcluster/taskgraph/try_option_syntax.py
--- a/taskcluster/taskgraph/try_option_syntax.py
+++ b/taskcluster/taskgraph/try_option_syntax.py
@@ -29,16 +29,17 @@ BUILD_KINDS = set([
     'l10n',
     'valgrind',
     'static-analysis',
     'spidermonkey',
 ])
 
 # anything in this list is governed by -j, matching against the `build_platform` attribute
 JOB_KINDS_MATCHING_BUILD_PLATFORM = set([
+    'nightly-l10n',
     'toolchain',
     'android-stuff',
 ])
 
 
 # mapping from shortcut name (usable with -u) to a boolean function identifying
 # matching test names
 def alias_prefix(prefix):
Flags: needinfo?(bugspam.Callek)
Callek: so, progress!  Thanks for all your help so far.  Any idea how I can make the N1 job in https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef230b595751f0ff326b51aa9a534ad69f51723d&selectedJob=93897108 download fennec-...apk rather than target.apk?  This seems like a bug when working on try but perhaps it's not.
Flags: needinfo?(bugspam.Callek)
(In reply to Nick Alexander :nalexander from comment #46)
> Callek: so, progress!  Thanks for all your help so far.  Any idea how I can
> make the N1 job in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ef230b595751f0ff326b51aa9a534ad69f51723d&selectedJob=9
> 3897108 download fennec-...apk rather than target.apk?  This seems like a
> bug when working on try but perhaps it's not.

Bah! That's a legit bug in my configs. 

This isn't actually a problem with file name, but is a problem with the configs. Fixable with the following patch too:

diff --git a/testing/mozharness/configs/single_locale/try_android-api-15.py b/testing/mozharness/configs/single_locale/try_android-api-15.py
--- a/testing/mozharness/configs/single_locale/try_android-api-15.py
+++ b/testing/mozharness/configs/single_locale/try_android-api-15.py
@@ -1,25 +1,27 @@
+import os
+
 BRANCH = "try"
 MOZILLA_DIR = BRANCH
 EN_US_BINARY_URL = "http://archive.mozilla.org/pub/" \
                    "mobile/nightly/latest-mozilla-central-android-api-15/en-US"
 
 config = {
     "branch": "try",
     "log_name": "single_locale",
     "objdir": "obj-l10n",
     "is_automation": True,
     "buildbot_json_path": "buildprops.json",
     "force_clobber": True,
     "clobberer_url": "https://api.pub.build.mozilla.org/clobberer/lastclobber",
     "locales_file": "%s/mobile/locales/l10n-changesets.json" % MOZILLA_DIR,
     "locales_dir": "mobile/android/locales",
     "ignore_locales": ["en-US"],
-    "nightly_build": False,
+    "nightly_build": True,
     'balrog_credentials_file': 'oauth.txt',
     "tools_repo": "https://hg.mozilla.org/build/tools",
     "tooltool_config": {
         "manifest": "mobile/android/config/tooltool-manifests/android/releng.manifest",
         "output_dir": "%(abs_work_dir)s/" + MOZILLA_DIR,
     },
     "exes": {
         'tooltool.py': '/builds/tooltool.py',
@@ -43,17 +45,17 @@ config = {
     "hg_l10n_tag": "default",
     'vcs_share_base': "/builds/hg-shared",
 
     "l10n_dir": "l10n-central",
     "repack_env": {
         # so ugly, bug 951238
         "LD_LIBRARY_PATH": "/lib:/tools/gcc-4.7.2-0moz1/lib:/tools/gcc-4.7.2-0moz1/lib64",
         "MOZ_OBJDIR": "obj-l10n",
-        "EN_US_BINARY_URL": EN_US_BINARY_URL,
+        "EN_US_BINARY_URL": os.environ.get("EN_US_BINARY_URL", EN_US_BINARY_URL),
         "LOCALE_MERGEDIR": "%(abs_merge_dir)s/",
         "MOZ_UPDATE_CHANNEL": "try", # XXX Invalid
     },
     "upload_branch": "%s-android-api-15" % BRANCH,
     "ssh_key_dir": "~/.ssh",
     "merge_locales": True,
     "mozilla_dir": MOZILLA_DIR,
     "mozconfig": "%s/mobile/android/config/mozconfigs/android-api-15/l10n-nightly" % MOZILLA_DIR,
Flags: needinfo?(bugspam.Callek)
I filed that as bug 1355846 last time I pushed to try (for the compare-locales update)
With Callek's help, I've got things green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c30c47a80eac920f2575f1c29f85bb29af8aa23

My memory just failed me, and testing this is subtle: I didn't want abspath, I wanted vdir-relative paths.  Patch inbound.
Attachment #8860533 - Attachment is obsolete: true
Comment on attachment 8861638 [details]
Bug 1254355 - Follow-up: Use VPATH relative paths to match GENERATED_FILES targets.

https://reviewboard.mozilla.org/r/133630/#review136518
Attachment #8861638 - Flags: review?(mshal) → review+
Comment on attachment 8861683 [details]
Bug 1254355 - Part 2: Generate AndroidManifest.xml using GENERATED_FILES.

gps already r+ed this; Mozreview is just terrible at re-landing after backout.
Attachment #8861683 - Flags: review?(gps) → review+
Comment on attachment 8861684 [details]
Bug 1254355 - Post: Make android-* jobs depend on more of the build.

gps already r+ed this; Mozreview is just terrible at re-landing after backout.
Attachment #8861684 - Flags: review?(gps) → review+
mcote: I am trying to autoland commits that have been backed out.  I have grafted the backed out commits back (twice!) and pushed them back for review.  Apparently I didn't push enough for review the second time (I pushed just the commit that changed) and mozreview has forgotten that I have r+ from gps on the two earlier commits.  I set them to r+ in Bugzilla, but mozreview doesn't care.  How do I reland (using autoland, since we're not supposed to push to inbound these days) without wasting the time of my reviewer, who are probably already justifiably annoyed with this ticket?  This process is extremely frustrating!
Flags: needinfo?(mcote)
Chatted with nalexander on IRC.  Gist is that, once landed, MozReview commit series can't really be reused to handle backouts; a new series should be created, either in a new bug or with a new review ID (see http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#bug-references-review-identifiers-and-grouping-commits).
Flags: needinfo?(mcote)
No longer blocks: gradle-automation
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: